-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dashboard resource #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Left some inline comments
@@ -0,0 +1,39 @@ | |||
# Datadog::Dashboards::Dashboard | |||
|
|||
Congratulations on starting development! Next steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace this file with an example stack? For ex - https://github.com/DataDog/datadog-cloudformation-resources/blob/master/datadog-integrations-aws-handler/README.md
@@ -0,0 +1,2 @@ | |||
cloudformation-cli-python-lib==2.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets update these to just require the common package at 0.0.1
LOG.info("Starting %s Create Handler", TYPE_NAME) | ||
model = request.desiredResourceState | ||
|
||
json_payload = json.loads(model.DashboardDefinition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should wrap this in a try/catch and return a failed ProgressEvent if its invalid json
with v1_client( | ||
model.DatadogCredentials.ApiKey, | ||
model.DatadogCredentials.ApplicationKey, | ||
model.DatadogCredentials.ApiURL or "https://api.datadoghq.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model.DatadogCredentials.ApiURL or "https://api.datadoghq.com", | |
model.DatadogCredentials.ApiURL, |
Can we try without, the common package should now handle this
TELEMETRY_TYPE_NAME, | ||
__version__, | ||
) as api_client: | ||
dashboard = validate_and_convert_types( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind adding a comment about this one? With like whats expected in this signature and thats its doing the serialization? Also, can this raise an exception if the dashboard is invalid? If so we should do the same progressEvent exception
try: | ||
dash = api_instance.get_dashboard(dashboard_id) | ||
json_dict = ApiClient.sanitize_for_serialization(dash) | ||
model.DashboardDefinition = json.dumps(json_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth catching this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, dumping should not raise
@@ -0,0 +1,79 @@ | |||
{ | |||
"typeName": "Datadog::Dashboards::Dashboard", | |||
"description": "Datadog Dashboard", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also trying to standardize adding the version number of the resource in this field (see PR #97) If that gets approved, can we add the version here
SSIA