-
Notifications
You must be signed in to change notification settings - Fork 265
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 resource for monitor alerts #679
Add resource for monitor alerts #679
Conversation
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.
@andrewsomething @danaelhe My first goal is to be able to create a monitor alert with the compiled module, and then work more on the acceptance/unit testing.
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.
This is looking great! Offered some pointers inline including for the expandAlerts
.
d6be4e7
to
1b376db
Compare
I enabled the new resource in provider.go, and you can now run the acceptance tests (I'll add more). This one fails. I doubt I'll have much time to work on this during next week.
|
It actually creates an alert for a droplet, with one of the acceptance tests. There is a stack trace that's somewhat tricky for me to debug. Help would be appreciated. |
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.
@atombrella When I ran the tests, I received the following panic:
panic: alerts.0.email: '': source data must be an array or slice, got struct
It looks like your last commit tries to address what I initially thought was the problem. Unfortunately, I still receive the above error after fixing that.
The problem is with how the flatten
functions handle the email and slack data. I've added some suggestions inline to address this and the test cases.
I also added a comment regarding the alerts
property set as a schema.TypeList
for some discussion.
@scotchneat @danaelhe @andrewsomething Thanks for the assistance. I do have one testing regarding tests: I would have liked to include some tests for validation, e.g., window not in [5m, 10m, 30m, 1h]. However, it doesn't seem like the provider has anything else but acceptance tests (which all require network connections, and a token), so I suppose this is not essential. I think it's approaching a stable state :) |
My branch is open for edits/commits from maintainers. |
Co-authored-by: Cesar Garza <[email protected]>
d53a1eb
to
a96ed26
Compare
@@ -31,7 +31,8 @@ The following arguments are supported: | |||
* `backups` - (Optional) Boolean controlling if backups are made. Defaults to | |||
false. | |||
* `monitoring` - (Optional) Boolean controlling whether monitoring agent is installed. | |||
Defaults to false. | |||
Defaults to false. If set to `true`, you can configure monitor alert policies | |||
[monitor alert resource](/providers/digitalocean/digitalocean/latest/docs/resources/monitor_alert) |
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.
👍 Good call out!
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 think we're probably good to go here, but wouldn't mind a second look 👀 @scotchneat
Thanks for another great contribution @atombrella!
I'm glad you saw the comment in the code about those constants :)
Great. I appreciate the help and coaching. A colleague's first reaction to me highlighting the alert monitor feature for droplets was "Can I do it in Terraform?". Now they will be. |
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:
Thanks for the contribution @atombrella, and the collaboration!
I just had one question about a couple of tests but it's not a blocker.
65e9d77
@scotchneat I deleted the duplicated test, and thought it'd be nice to test having two slack channels connected. Please see 65e9d77 |
@atombrella Good call. |
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.
* [skip ci] Alert policy resource WIP * [skip ci] More work * [skip ci] read and create functions * [skip ci] flatten/expand functions * More work * flatten/expand work * Add template for testing. WIP * Feedback from @andrewsomething * Temlate for testing * flatten function, and documentation * enable support for monitor_alert in provider.go * add Enabled to create request * flatten/expand functions * Rename resourceDigitalMonitorAlert to resourceDigitalOceanMonitorAlert * creating alerts :) * import acceptance tests * template for update test * added CheckDestroy to acceptance tests * Update digitalocean/resource_digitalocean_monitor_alert.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert_test.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert_test.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert_test.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert_test.go Co-authored-by: Cesar Garza <[email protected]> * Update digitalocean/resource_digitalocean_monitor_alert_test.go Co-authored-by: Cesar Garza <[email protected]> * Make destroy method for tests * Support alerts using only tags. * Fix import test. * Update godo to pick up new consts. * Use consts for all alert types. * Support full updates. * Add test with two Slack channels Co-authored-by: Cesar Garza <[email protected]> Co-authored-by: Andrew Starr-Bochicchio <[email protected]> Co-authored-by: Andrew Starr-Bochicchio <[email protected]>
It still needs a lot of work, but at least it's a start :) Hoping for a bit of help to get it pushed over the finish line. I'll start also on unit/acceptance tests.
#479