-
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
Add support for metadata block in dashboard widgets #278
Conversation
Merge upstream repo
Merge upstream
Sync with upstream
Merge upstream
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.
Since #277 was approved I'm reviewing this based on this diff - enbashi/terraform-provider-datadog@rami/add_style...enbashi:rami/add_metadata
Overall looks good. Thanks for this! Left a couple minor comments/questions.
- `line_width` - (Optional) Width of line displayed. Available values are: `normal`, `thick`, or `thin`. | ||
- `metadata` - (Optional). Used to define expression aliases. Multiple nested blocks are allowed with the following structure: | ||
- `expression` - (Required) | ||
- `alias_name` - (Optional) |
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 there a reason one would add the expression but not an alias for it?
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.
probably not a typical use case, but the UI allows it, and so does the the API and Go client.
Schema: map[string]*schema.Schema{ | ||
"expression": { | ||
Type: schema.TypeString, | ||
Optional: true, |
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.
Should this be required (the docs changes say this field is required)
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're absolutely correct. It is a required. Thanks for catching this.
Expression: datadog.String(metadata["expression"].(string)), | ||
} | ||
// AliasName | ||
if v, ok := metadata["alias_name"].(string); ok && len(v) != 0 { |
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.
does this need the same String
call as the one above?
Add support for style block in dashboard widgets (DataDog#277)
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.
Looks good to me. Thanks for the iteration!
Add support for
metadata
block in dashboard widgets. Fixes https://github.com/terraform-providers/terraform-provider-datadog/issues/271. To go after https://github.com/terraform-providers/terraform-provider-datadog/pull/277.