-
Notifications
You must be signed in to change notification settings - Fork 389
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 nine more widgets #2161
Conversation
021f70a
to
1651152
Compare
Created DOCS-6650 for 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.
Confirmed with Dashboard team that Event Stream and Log Stream are now supported under the List widget, Event Timeline is supported under Timeseries widget. And Scatterplot should be Scatter Plot if matching to the UI.
ed71ceb
to
cdb0fa4
Compare
cdb0fa4
to
f529fff
Compare
Type: schema.TypeList, | ||
Optional: true, | ||
MaxItems: 1, | ||
Description: "The definition for an Log Stream 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.
nitpick(typo):
Description: "The definition for an Log Stream widget.", | |
Description: "The definition for a Log Stream widget.", |
for _, v := range []string{"formula"} { | ||
// Properties listed above are defined as single, but API Spec expects a plural name | ||
if widgetDefRequest[v] != nil { | ||
widgetDefRequest[v+"s"] = widgetDefRequest[v] | ||
delete(widgetDefRequest, v) | ||
} | ||
} |
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.
suggestion: I think the intent is that we'll have more than just one value here to pluralize in the future, but it might be safer for future developers to be more explicit here (example below), so that something like query->querys doesn't happen (i.e. we'll likely need some kinda lookup of singular->plural here at some point)
for _, v := range []string{"formula"} { | |
// Properties listed above are defined as single, but API Spec expects a plural name | |
if widgetDefRequest[v] != nil { | |
widgetDefRequest[v+"s"] = widgetDefRequest[v] | |
delete(widgetDefRequest, v) | |
} | |
} | |
// "formulas" is the expected form per the spec | |
if widgetDefRequest["formula"] != nil { | |
widgetDefRequest["formulas"] = widgetDefRequest[v] | |
delete(widgetDefRequest, "formula") | |
} |
delete(widgetDefRequest, v) | ||
} | ||
} | ||
if widgetType != "topology_map" && widgetDefRequest["query"] != nil { |
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.
question: Why is topology_map
specifically excluded here?
The following types have a query
option in their requests
:
[
"distribution",
"funnel",
"geomap",
"list_stream",
"slo_list",
"topology_map"
]
I think I might have asked this before in a previous review but can't seem to remember the reason
return nil, diags | ||
} | ||
// Distribution/change/heatmap widgets have a "requests" field, while API Spec has a "request" field | ||
// Here we set the "requests" field and remove "request" |
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.
suggestion(minor):
// Here we set the "requests" field and remove "request" |
for _, v := range []string{"event", "custom_link", "input"} { | ||
// Properties listed above are defined as single, but API Spec expects a plural name | ||
if widgetDef[v] != nil { | ||
widgetDef[v+"s"] = widgetDef[v] | ||
delete(widgetDef, v) | ||
} | ||
} |
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.
suggestion(non-blocking): Similar to a suggestion above, we may want to consider remapping properties w/ some kinda map here, rather than relying the on the "s"-suffix relation
for _, v := range []string{"custom_links", "inputs", "events"} { | ||
// Properties listed above are defined as plural for the API Spec and need to be converted back to single values for TF | ||
if widgetDef[v] != nil { | ||
widgetDef[v[:len(v)-1]] = widgetDef[v] | ||
delete(widgetDef, v) | ||
} | ||
} |
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.
suggestion(non-blocking): Similar to a suggestion above, we may want to consider remapping properties w/ some kinda map here, rather than relying the on the "s"-suffix relation
This pull request implements the second part of the support for the powerpack resource in Terraform.
Previous PR:
Changes
How to test: