-
Notifications
You must be signed in to change notification settings - Fork 29
Add New Relic Infrastructure API support #15
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.
First review for the project, but this looks good to me.
@paultyng—would appreciate a quick spot check too since it's my first PR review in this repo :)
@paultyng Gentle ping here, just as I'm very keen to see these changes downstream in the NewRelic Terraform provider :) |
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.
One minor change (rename i
), but otherwise looks good to me. I would like there to be more assertions in the test coverage if possible, but not going to hold it up for that.
@@ -14,6 +14,20 @@ type Client struct { | |||
RestyClient *resty.Client | |||
} | |||
|
|||
// InfraClient represents the client state for the Infrastructure API | |||
type InfraClient struct { |
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'm not as familiar with this part of the New Relic API, so Infra is an entirely separate API (including a separate endpoint) I take it?
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.
That's correct—separate endpoint.
api/alert_infra_conditions.go
Outdated
"strconv" | ||
) | ||
|
||
func (i *InfraClient) queryAlertInfraConditions(policyID int) ([]AlertInfraCondition, error) { |
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
is probably a bad name for this due to its more idiomatic usage, looping, etc, maybe just c
or cli
or something that evokes client?
Thanks for the feedback, I will address this later today. |
@paultyng updated the InfraClient variable name to be more consistent with the rest of the project. |
Released as v1.6.0 |
Add NRQL Alert Condition Resource
As requested by @smithclay in https://github.com/terraform-providers/terraform-provider-newrelic/pull/30
Let me know if this is what you had in mind. This adds New Relic Infrastructure API support.