Skip to content
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

Merged

Conversation

atombrella
Copy link
Contributor

@atombrella atombrella commented Sep 1, 2021

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.

  • Have Terraform create the alerts
  • Documentation. There is an example, attribute reference.
  • import acceptance test
  • More acceptance and uni test

#479

@atombrella atombrella changed the title Droplet monitoring gh 479 Add resource for monitor alerts Sep 1, 2021
@atombrella atombrella marked this pull request as draft Sep 1, 2021
Copy link
Contributor Author

@atombrella atombrella left a comment

@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.

digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewsomething andrewsomething left a comment

This is looking great! Offered some pointers inline including for the expandAlerts.

digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
@atombrella atombrella force-pushed the droplet_monitoring_gh_479 branch from d6be4e7 to 1b376db Compare Sep 5, 2021
@atombrella
Copy link
Contributor Author

@atombrella atombrella commented Sep 5, 2021

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.

make testacc TESTARGS='-run=TestAccDigitalOceanMonitorAlertNoAlerts'

@atombrella
Copy link
Contributor Author

@atombrella atombrella commented Sep 9, 2021

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.

Copy link
Member

@scotchneat scotchneat left a comment

@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.

digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert_test.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert_test.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert_test.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert_test.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert_test.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
digitalocean/resource_digitalocean_monitor_alert.go Outdated Show resolved Hide resolved
docs/resources/monitor_alert.md Outdated Show resolved Hide resolved
@atombrella
Copy link
Contributor Author

@atombrella atombrella commented Sep 14, 2021

@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 :)

@atombrella atombrella marked this pull request as ready for review Sep 15, 2021
@atombrella
Copy link
Contributor Author

@atombrella atombrella commented Sep 16, 2021

My branch is open for edits/commits from maintainers.

@atombrella atombrella force-pushed the droplet_monitoring_gh_479 branch from d53a1eb to a96ed26 Compare Sep 20, 2021
@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Sep 21, 2021

@atombrella In addition to fixing the import test (f69c6d6), I pushed a few additional commits:

  • Support alerts using only tags (48ca9ce).
  • Update godo to pick up new consts (9eb4610) and use them for all alert types (67f3812).
  • Support updating all attributes (96cc08c).

@@ -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)
Copy link
Member

@andrewsomething andrewsomething Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good call out!

andrewsomething
andrewsomething previously approved these changes Sep 21, 2021
Copy link
Member

@andrewsomething andrewsomething left a comment

I think we're probably good to go here, but wouldn't mind a second look 👀 @scotchneat

Thanks for another great contribution @atombrella!

@atombrella
Copy link
Contributor Author

@atombrella atombrella commented Sep 22, 2021

@atombrella In addition to fixing the import test (f69c6d6), I pushed a few additional commits:

  • Support alerts using only tags (48ca9ce).
  • Update godo to pick up new consts (9eb4610) and use them for all alert types (67f3812).

I'm glad you saw the comment in the code about those constants :)

  • Support updating all attributes (96cc08c).

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.

scotchneat
scotchneat previously approved these changes Sep 22, 2021
Copy link
Member

@scotchneat scotchneat left a comment

: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.

digitalocean/resource_digitalocean_monitor_alert_test.go Outdated Show resolved Hide resolved
@atombrella atombrella dismissed stale reviews from scotchneat and andrewsomething via 65e9d77 Sep 22, 2021
@atombrella
Copy link
Contributor Author

@atombrella atombrella commented Sep 22, 2021

@scotchneat I deleted the duplicated test, and thought it'd be nice to test having two slack channels connected. Please see 65e9d77

@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Sep 22, 2021

@atombrella Good call.

Copy link
Member

@andrewsomething andrewsomething left a comment

:shipit:

@andrewsomething andrewsomething merged commit a9c5474 into digitalocean:main Sep 22, 2021
3 checks passed
@atombrella atombrella deleted the droplet_monitoring_gh_479 branch Sep 22, 2021
atombrella added a commit to atombrella/terraform-provider-digitalocean that referenced this issue Oct 10, 2021
* [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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants