-
Notifications
You must be signed in to change notification settings - Fork 387
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
[datadog_powerpacks] Implement support for basic widgets #2157
Conversation
Created DOCS-6533 for the Docs Team editorial review. |
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.
First pass on the data/tests looks great! 🙌
I had one question on potentially tweaking one of the tests for consideration
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.
lgtm wrt @datadog/dashboards-backend side of things 🙌
docs/resources/powerpack.md
Outdated
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 don't know how much control you have over this page, I noticed the note about the schema being generated by tfplugindocs, but some of the descriptions for the widgets don't match the UI:
- Ln 71 Service Summary widget
- Ln 74 Pie Chart widget
- Ln 76 Top List widget
- Ln 78 I'm not sure about the Trace Service widget, is this supposed to be the Profiler Flame graph widget?
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.
@estherk15 I believe I can manually edit the values after they're autogenerated without any issues. @skarimo is that correct? I'll make the adjustments.
Ln 71 servicemap -> this matches the Service Map widget type, no? (src)
Ln 74 sunburst -> this matches Pie Chart, will fix. [src]
Ln 76 toplist -> this matches Top List, will fix. [src]
Ln 78 trace_service -> this matches Service Summary widget type. [src]
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.
You are correct. Looks like we can use display name in the description as well to clear things up but lets do that in a separate pr as this re-uses schemas from the dashboard resource and we would be modifying that as well with these changes. The descriptions used in the docs come from its schema, for example: https://github.com/DataDog/terraform-provider-datadog/blob/master/datadog/resource_datadog_dashboard.go#L930
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.
Thanks for making the changes! Left a few minor nits but otherwise this looks great!
apiInstances := providerConf.DatadogApiInstances | ||
auth := providerConf.Auth | ||
|
||
err := utils.Retry(2, 10, func() error { |
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.
Could we move the retry into the for loop. I fixed this up earlier this week here for the other resources: #2166
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.
Done 👍
x := dimensions["x"].(int64) | ||
y := dimensions["y"].(int64) | ||
widgetLayout = datadogV2.NewPowerpackInnerWidgetLayout(height, width, x, y) | ||
} else { |
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.
} else { | |
} else if strings.HasSuffix(widgetType, "_definition") { |
e79cf4c
to
81a4e87
Compare
Co-authored-by: skarimo <[email protected]>
This pull request implements the first part of the support for the powerpack resource in Terraform.
Unfortunately, we can't immediately add all widgets since we do not validate powerpacks widgets in the public API spec. As a result, we have to manually do some validation for each new widget type. I initially tried to add all widgets, but the PR got unreasonably large, so I opted to split it out into several parts. This first part is the simplest widgets, which don't have any nested configurations. Widgets with nested configurations require extra validation which I will add on in the following PRs.
Most of the changes in this PR are cassettes and I opted to create a separate test file for each new widget.
Changes
How to test: