-
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 dashboard list resource #296
Conversation
@@ -32,19 +32,20 @@ func Provider() terraform.ResourceProvider { | |||
}, | |||
|
|||
ResourcesMap: map[string]*schema.Resource{ | |||
"datadog_downtime": resourceDatadogDowntime(), |
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.
Alphabetized while I was here...
This PR requires - https://github.com/terraform-providers/terraform-provider-datadog/pull/297 to be merged before CI will pass. |
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 overall. I do have some questions below tho.
id, _ := strconv.Atoi(d.Id()) | ||
// Deleting the overall List will also take care of deleting its sub elements | ||
// Deletion of individual dash items happens in the Update method | ||
// Note this doesn't delete the actual dashboards, just removes them from the deleted list |
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 understand this note
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.
Just mentioning that you don't have to delete each individual dashboard from the list, deleting the list itself is enough.
"dash_id": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
Description: "The list of dashboard IDs to add", |
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 this a list of ids or just one id?
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.
Just one ID
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 recall that trying to make this a list made the provider code really complex. But I see I forgot to update the description, will fix that 😄
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. Assuming this as been tested tho.
Adds a new dashboard list resource.