Skip to content
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 new monitor summary widget parameters #396

Merged

Conversation

chrismdd
Copy link
Contributor

This PR adds two new, optional parameters for the monitor summary (manage status) widget: summary_type, and show_last_triggered: https://docs.datadoghq.com/graphing/widgets/monitor_summary/#api.

go-datadog-api was updated here: zorkian/go-datadog-api#290

It also updates the acceptance tests for screenboards and dashboards, both of which pass with these changes.

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Looks mostly good, just a couple small comments.

@@ -2335,6 +2335,10 @@ func getManageStatusDefinitionSchema() map[string]*schema.Schema {
Type: schema.TypeString,
Required: true,
},
"summary_type": {
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also set a validation here to ensure its one of the three types - https://www.terraform.io/docs/extend/schemas/schema-behaviors.html#validatefunc or will these change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I added a description and validation. Please let me know if I should move the validation function somewhere or otherwise alter it (I don't use Go much at the moment).

@chrismdd
Copy link
Contributor Author

@jirikuncar @nmuesch thanks for reviewing. I don't have write access to this repo, so could you please merge this PR for me?

Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tests are passing. Merging, thanks for the contribution!

@bkabrda bkabrda merged commit c8c1848 into DataDog:master Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants