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

Support for Librato Alerts and Notification Services #8170

Merged
merged 8 commits into from
Aug 31, 2016

Conversation

elblivion
Copy link
Contributor

@elblivion elblivion commented Aug 13, 2016

This PR extends the Librato plugin to support managing Alerts, which we're using quite heavily at Contentful. I've also added very basic support for notification Services, basically to have a self-contained acceptance test suite - Services have freeform configuration that heavily depends on each particular integration, not sure yet whether we would be managing them through Terraform.

The vendored go-librato lib is updated to the latest master which incorporates support for the relevant APIs.

I've tried to follow the example of @henrikhodne 's existing code, please let me know if anything looks odd. I'm still not very proficient with Go so there may well be some howlers. :-)

@elblivion
Copy link
Contributor Author

BTW I found it quite confusing to update a vendored library. I followed the instructions on https://github.com/hashicorp/terraform#updating-a-dependency but nothing happened (or nothing was happening for a very long time); after reading a lot of examples/docs for govendor - which could really use a verbose mode - I ended up with govendor update +vendor which updated everything and then I checked in only the go-librato lib.

@elblivion
Copy link
Contributor Author

Hi, any chance of having this reviewed soonish? Thanks!

@elblivion
Copy link
Contributor Author

Hi @radeksimko, anything I could do from my side to get this merged? Thanks!

},
},
},
// TODO add missing condition attrs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a to-do before the PR get's merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, a left-over reminder, removing.

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Aug 31, 2016
@stack72
Copy link
Contributor

stack72 commented Aug 31, 2016

Hi @elblivion

Thanks for the PR here. I left a few comments inline - have a look at them if you would. Please can you update the librato.erb file to add the new documentation pages to the sidebar?

@elblivion
Copy link
Contributor Author

Also added the pages to the sidebar template.

@stack72 stack72 merged commit e4af2d5 into hashicorp:master Aug 31, 2016
@stack72
Copy link
Contributor

stack72 commented Aug 31, 2016

Hi @elblivion

I made an extra commit here for the failed build: 32ad221

Tests pass as expected :)

% make testacc TEST=./builtin/providers/librato TESTARGS='-run=Test'                                                                                                                                      ✹
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/31 21:42:36 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/librato -v -run=Test -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccLibratoAlert_Basic
--- PASS: TestAccLibratoAlert_Basic (1.87s)
=== RUN   TestAccLibratoAlert_Full
--- PASS: TestAccLibratoAlert_Full (3.00s)
=== RUN   TestAccLibratoAlert_Updated
--- PASS: TestAccLibratoAlert_Updated (1.75s)
=== RUN   TestAccLibratoService_Basic
--- PASS: TestAccLibratoService_Basic (2.58s)
=== RUN   TestAccLibratoService_Updated
--- PASS: TestAccLibratoService_Updated (2.82s)
=== RUN   TestAccLibratoSpaceChart_Basic
--- PASS: TestAccLibratoSpaceChart_Basic (5.00s)
=== RUN   TestAccLibratoSpaceChart_Full
--- PASS: TestAccLibratoSpaceChart_Full (7.02s)
=== RUN   TestAccLibratoSpaceChart_Updated
--- PASS: TestAccLibratoSpaceChart_Updated (8.71s)
=== RUN   TestAccLibratoSpace_Basic
--- PASS: TestAccLibratoSpace_Basic (4.21s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/librato        36.968s

Thanks for all the work here!

Paul

@jonleighton
Copy link

Hi, thanks for this. Unfortunately I have found two bugs with the implementation:

@ghost
Copy link

ghost commented Apr 22, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/librato waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants