provider/newrelic: Add a New Relic provider #10317

paultyng · 2016-11-23T16:19:19Z

This is still a work in progress, but I wanted to get some feedback / thoughts on if this is desirable and ways to improve it as its my first larger contribution to terraform.

The provider allows you to manage your New Relic alert policies, notification channels, alert conditions, etc.

Coupled with the Pager Duty provider, you can even link up alerts to pager duty solely in terraform.

(Ignore the commit messages, I'll clean it up all up once its closer to complete)

Update: Running Acceptance Tests

To run acceptance tests, you'll need to setup a new New Relic account, and generate an Admin API key. You will also need the license key, only for acceptance tests though, its not used in the actual provider.

You can then pass the API key via the NEWRELIC_API_KEY environment var and the license key via NEWRELIC_LICENSE_KEY

NEWRELIC_API_KEY=1234567 NEWRELIC_LICENSE_KEY=8901234 make testacc TEST=./builtin/providers/newrelic

paultyng · 2016-11-23T16:20:49Z

The main issue was there was no good Go API client for New Relic alerts. So I've also been writing one of those as well. Their API is unfortunately pretty limited and not fully featured, so I've had to work around some of those in this implementation. They also only provide a Swagger 1.1 spec, and I couldn't get a great autogenerated client from it in any tools I tried, so I just went with hand coding for now.

See https://github.com/paultyng/go-newrelic

stack72 · 2016-11-23T16:32:08Z

Hey @paultyng

This is an exciting provider - ability to just bootstrap systems like this is awesome. In fairness, this is going to be a bit of a beast to review. It's a single commit with 225 files so far :)

Can you break this up a bit? Something like a commit per resource and a commit for the SDK addition would be really awesome!

Thoughts?

Paul

paultyng · 2016-11-23T16:58:42Z

Yeah no problem, I was planning on rewriting the commits once I got this closer to an MVP, so I'll do it in the way you suggest.

stack72 · 2016-11-23T17:04:10Z

Awesome! That will help a lot :)

P.

paultyng · 2016-11-23T21:05:24Z

@stack72 Updated with better commit messages and I think a good set of MVP resources. There are still some annoyances where you need magic ID's from new relic to make it work. I can add data sources to help in looking those up (like applications you have configured, etc). As far as I can tell though you can't create them via API so they can't be resources. Not sure exactly how much I should add to the PR as it will probably just make reviewing a pain.

tiny-dancer · 2016-11-24T05:00:37Z

very exciting, awesome work @paultyng! on a slightly similar note, were there any thoughts/plans around the synthetics api as well?
Synthetics API Docs | Scripted Browser Example

paultyng · 2016-11-25T16:13:24Z

I've added some ValidationFuncs in to help with attributes that are lists of keys and some other validation as well.

paultyng · 2016-11-26T18:07:09Z

@tiny-dancer I don't currently use the Synthetics API so wasn't in my initial plan. I want to try to keep this PR small but useful, but once the provider is in, adding additional functionality should be a bit more streamlined and can look in to it then.

tiny-dancer · 2016-11-26T18:40:45Z

@paultyng sound good. and yah wasn't thinking for this PR, just curious if it was in your plans. was about to implement a temp solution with jenkinsfile to bring source control to the pingers/scripted tests until i ran across this pr. if we can build off the start here, makes muuch more sense in terraform. on the surface would seem to build nicely for directly relating the policies defined here to the synthetic tests. 👍

paultyng · 2016-11-26T18:49:25Z

@stack72 I'm not sure what to do in the acceptance tests about the data source testing, as it requires manually creating data in New Relic and testing to see if its present. (see here).

I guess its possible I could fake creating an application by sending a metric ping to new relic, not sure if that works or not, I guess I can test it out.

paultyng · 2016-11-26T18:53:19Z

Seems like it will work, but it will require their go agent and the reporting api key for the acceptance tests, but seems a lot nicer than having to configure it in the UI for tests to pass.

I'll add that.

paultyng · 2016-11-27T16:03:17Z

Ok, added app creation via the agent sdk just for testing. Can maybe eventually work that in to a resource instead of a data source but it requires a separate API key and integration, so a little annoying. I updated my acceptance test instructions above.

paultyng · 2016-11-30T19:14:04Z

@stack72 need anything from me on this? Anyone available for a code review?

stack72 · 2016-12-01T10:49:03Z

Hi @paultyng

is this no longer considered a work in progress now?

Paul

paultyng · 2016-12-01T11:18:50Z

Correct, I think this has enough for v1.

stack72

Hi @paultyng

Overall, this is looking really good. I have left a few comments inline around Import and also test names - this related to all resources

If possible, please can you also look at the case where someone deletes something from the UI - currently, most of the Read funcs will error out on that as it's not found

We have a pattern in our AWS provider for this:

getResp, err := iamconn.ListAccessKeys(request)
	if err != nil {
		if iamerr, ok := err.(awserr.Error); ok && iamerr.Code() == "NoSuchEntity" { // XXX TEST ME
			// the user does not exist, so the key can't exist.
			d.SetId("")
			return nil
		}
		return fmt.Errorf("Error reading IAM acces key: %s", err)
	}

This refreshes the current resource from state, continues and terraform will then allow it to create a new one :)

Hope this helps

Paul

stack72 · 2016-12-01T11:03:56Z

builtin/providers/newrelic/data_source_newrelic_application_test.go

+// This name must match the constant defined in provider_test.go
+const testAccNewRelicApplicationConfig = `
+data "newrelic_application" "app" {
+	name = "terraform-test-application"


We should randomise this application name - this would stop any duplicate resource names

An example can be found here

stack72 · 2016-12-01T11:26:21Z

builtin/providers/newrelic/resource_newrelic_alert_channel.go

+
+	d.Set("name", channel.Name)
+	d.Set("type", channel.Type)
+	d.Set("configuration", channel.Configuration)


As this is a complex structure - I think we should check for an error setting the value, e.g.

if err := d.Set("configuration", channel.Configuration); err != nil { return fmt.Errorf("[DEBUG] Error setting Alert Channel Configuration: %#v", err) }

stack72 · 2016-12-01T11:27:41Z

builtin/providers/newrelic/resource_newrelic_alert_channel.go

+	return nil
+}
+
+func resourceNewRelicAlertChannelImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {


As we don't have any custom logic in this import, we should just add it to the schema as follows:

Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, },

This calls the Read func by default :)

stack72 · 2016-12-01T11:28:15Z

builtin/providers/newrelic/resource_newrelic_alert_channel_test.go

+
+const testAccCheckNewRelicAlertChannelConfig = `
+resource "newrelic_alert_channel" "foo" {
+  name = "foo"


Should be randomized to avoid test collision

stack72 · 2016-12-01T11:28:17Z

builtin/providers/newrelic/resource_newrelic_alert_channel_test.go

+
+const testAccCheckNewRelicAlertChannelConfigUpdated = `
+resource "newrelic_alert_channel" "foo" {
+  name = "bar"


Should be randomized to avoid test collision

stack72 · 2016-12-01T11:31:47Z

builtin/providers/newrelic/resource_newrelic_alert_condition.go

+	policyID := ids[0]
+	id := ids[1]
+
+	log.Printf("[INFO] Deleting New Relic alert condition %v", id)


is %v correct for this? Wouldn't %s be better as it's a string?

Its an int in this case

I can switch to %d?

stack72 · 2016-12-01T11:32:02Z

builtin/providers/newrelic/resource_newrelic_alert_condition_test.go

+
+const testAccCheckNewRelicAlertConditionConfig = `
+resource "newrelic_alert_policy" "foo" {
+  name = "foo"


Randomize the test name :)

stack72 · 2016-12-01T11:32:13Z

builtin/providers/newrelic/resource_newrelic_alert_condition_test.go

+resource "newrelic_alert_condition" "foo" {
+  policy_id = "${newrelic_alert_policy.foo.id}"
+
+  name        = "foo"


randomize the test name :)

stack72 · 2016-12-01T11:32:19Z

builtin/providers/newrelic/resource_newrelic_alert_condition_test.go

+
+const testAccCheckNewRelicAlertConditionConfigUpdated = `
+resource "newrelic_alert_policy" "foo" {
+  name = "foo"


stack72 · 2016-12-01T11:32:34Z

builtin/providers/newrelic/resource_newrelic_alert_policy.go

+		// Update: Not currently supported in API
+		Delete: resourceNewRelicAlertPolicyDelete,
+		Importer: &schema.ResourceImporter{
+			State: resourceNewRelicAlertPolicyImport,


Can just be a PassThrough

paultyng · 2016-12-01T12:32:57Z

Most (if not all) of the names do not have unique constraints in new relic. Should I still randomize?

paultyng · 2016-12-01T16:03:06Z

@stack72 changes made, I went ahead and added the randomization to the names anyway.

paultyng · 2016-12-09T14:39:38Z

I imagine you are all very busy with 0.8, let me know if there is anything I can do before you take another pass at this to help expedite it.

stack72 · 2016-12-15T19:01:44Z

hi @paultyng

Coming back to look at this again :) Give me like 30 mins and I will know the state of what is going on with it

Paul

stack72 · 2016-12-15T19:16:50Z

Hi @paultyng

This is looking wonderful now :) The tests all pass and I added the documentation link to the nav bar

% make testacc TEST=./builtin/providers/newrelic
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/15 19:07:45 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/newrelic -v  -timeout 120m
=== RUN   TestAccNewRelicApplication_Basic
--- PASS: TestAccNewRelicApplication_Basic (4.79s)
=== RUN   TestParseIDs_Basic
--- PASS: TestParseIDs_Basic (0.00s)
=== RUN   TestSerializeIDs_Basic
--- PASS: TestSerializeIDs_Basic (0.00s)
=== RUN   TestAccNewRelicAlertChannel_import
--- PASS: TestAccNewRelicAlertChannel_import (4.48s)
=== RUN   TestAccNewRelicAlertCondition_import
--- PASS: TestAccNewRelicAlertCondition_import (4.72s)
=== RUN   TestAccNewRelicAlertPolicy_import
--- PASS: TestAccNewRelicAlertPolicy_import (3.84s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProviderImpl
--- PASS: TestProviderImpl (0.00s)
=== RUN   TestAccNewRelicAlertChannel_Basic
--- PASS: TestAccNewRelicAlertChannel_Basic (9.13s)
=== RUN   TestAccNewRelicAlertCondition_Basic
--- PASS: TestAccNewRelicAlertCondition_Basic (7.91s)
=== RUN   TestAccNewRelicAlertPolicyChannel_Basic
--- PASS: TestAccNewRelicAlertPolicyChannel_Basic (9.83s)
=== RUN   TestAccNewRelicAlertPolicy_Basic
--- PASS: TestAccNewRelicAlertPolicy_Basic (5.67s)
=== RUN   TestValidationIntInInSlice
--- PASS: TestValidationIntInInSlice (0.00s)
=== RUN   TestValidationFloat64Gte
--- PASS: TestValidationFloat64Gte (0.00s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/newrelic	50.390s

Going to manually merge this as I am rebasing into 3 logical commits. Thanks for all the work you have put in here - it's great!

Paul

ghost · 2020-04-04T02:25:01Z

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.

stack72 added enhancement new-provider labels Nov 23, 2016

paultyng changed the title ~~[WIP] provider/newrelic: Add a New Relic provider~~ provider/newrelic: Add a New Relic provider Nov 25, 2016

stack72 reviewed Dec 1, 2016

View reviewed changes

stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Dec 1, 2016

Paul Tyng added 7 commits December 12, 2016 09:45

Add new vendor libraries for newrelic provider

3095c92

Add initial provider and alert channel resource

d236086

Add alert policy and policy channel resources

ba8ecce

Add alert condition resource

b33d3b7

Add new relic application data source

d0d2bcc

Add docs for New Relic provider

856f132

Add newrelic go-agent to vendor

076784a

Paul Tyng added 2 commits December 12, 2016 09:45

Add app creation via agent SDK for testing

f91a522

Flag some schema as sensitive

4f598cc

stack72 closed this Dec 15, 2016

paultyng deleted the pt/newrelic branch February 28, 2018 14:50

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/newrelic: Add a New Relic provider #10317

provider/newrelic: Add a New Relic provider #10317

paultyng commented Nov 23, 2016 •

edited

Loading

paultyng commented Nov 23, 2016 •

edited

Loading

stack72 commented Nov 23, 2016

paultyng commented Nov 23, 2016

stack72 commented Nov 23, 2016

paultyng commented Nov 23, 2016

tiny-dancer commented Nov 24, 2016 •

edited

Loading

paultyng commented Nov 25, 2016 •

edited

Loading

paultyng commented Nov 26, 2016

tiny-dancer commented Nov 26, 2016

paultyng commented Nov 26, 2016

paultyng commented Nov 26, 2016 •

edited

Loading

paultyng commented Nov 27, 2016

paultyng commented Nov 30, 2016

stack72 commented Dec 1, 2016

paultyng commented Dec 1, 2016

stack72 left a comment

stack72 Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

paultyng Dec 1, 2016 •

edited

Loading

paultyng Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

stack72 Dec 1, 2016

paultyng commented Dec 1, 2016

paultyng commented Dec 1, 2016

paultyng commented Dec 9, 2016

stack72 commented Dec 15, 2016

stack72 commented Dec 15, 2016

ghost commented Apr 4, 2020

provider/newrelic: Add a New Relic provider #10317

provider/newrelic: Add a New Relic provider #10317

Conversation

paultyng commented Nov 23, 2016 • edited Loading

Update: Running Acceptance Tests

paultyng commented Nov 23, 2016 • edited Loading

stack72 commented Nov 23, 2016

paultyng commented Nov 23, 2016

stack72 commented Nov 23, 2016

paultyng commented Nov 23, 2016

tiny-dancer commented Nov 24, 2016 • edited Loading

paultyng commented Nov 25, 2016 • edited Loading

paultyng commented Nov 26, 2016

tiny-dancer commented Nov 26, 2016

paultyng commented Nov 26, 2016

paultyng commented Nov 26, 2016 • edited Loading

paultyng commented Nov 27, 2016

paultyng commented Nov 30, 2016

stack72 commented Dec 1, 2016

paultyng commented Dec 1, 2016

stack72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultyng Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultyng commented Dec 1, 2016

paultyng commented Dec 1, 2016

paultyng commented Dec 9, 2016

stack72 commented Dec 15, 2016

stack72 commented Dec 15, 2016

ghost commented Apr 4, 2020

paultyng commented Nov 23, 2016 •

edited

Loading

paultyng commented Nov 23, 2016 •

edited

Loading

tiny-dancer commented Nov 24, 2016 •

edited

Loading

paultyng commented Nov 25, 2016 •

edited

Loading

paultyng commented Nov 26, 2016 •

edited

Loading

paultyng Dec 1, 2016 •

edited

Loading