new resource azurerm_newrelic_monitored_subscription
#27891
Conversation
…m into newrelic_monitored_sub
…m into newrelic_monitored_sub
|
|
There was a problem hiding this comment.
Hi @teowa ,
Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍
| @@ -24,6 +26,13 @@ func NewClient(o *common.ClientOptions) (*Client, error) { | |||
|
|
|||
| o.Configure(monitorsClient.Client, o.Authorizers.ResourceManager) | |||
|
|
|||
| monitoredSubscriptionsClient, err := monitoredsubscriptions.NewMonitoredSubscriptionsClientWithBaseURI(o.Environment.ResourceManager) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("building Monitors client: %+v", err) | |||
There was a problem hiding this comment.
| return nil, fmt.Errorf("building Monitors client: %+v", err) | |
| return nil, fmt.Errorf("building Monitored Subscriptions client: %+v", err) |
| return fmt.Errorf("decoding: %+v", err) | ||
| } | ||
|
|
||
| client := metadata.Client.NewRelic.MonitoredSubscriptionsClient |
There was a problem hiding this comment.
please put client := ... at the beginning of the method
|
|
||
| existing := resp.Model | ||
| if existing == nil { | ||
| return fmt.Errorf("retrieving %s: model was nil", *id) |
There was a problem hiding this comment.
| return fmt.Errorf("retrieving %s: model was nil", *id) | |
| return fmt.Errorf("retrieving %s: `model` was nil", *id) |
| } | ||
|
|
||
| if existing.Properties == nil { | ||
| return fmt.Errorf("retrieving %s: property was nil", *id) |
There was a problem hiding this comment.
| return fmt.Errorf("retrieving %s: property was nil", *id) | |
| return fmt.Errorf("retrieving %s: `properties` was nil", *id) |
| if err := client.CreateOrUpdateThenPoll(ctx, monitorId, *existing); err != nil { | ||
| return fmt.Errorf("updating %s: %+v", *id, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
update does not need resourceMonitoredSubscriptionsWaitForAvailable ?
|
|
||
| monitorId := monitoredsubscriptions.NewMonitorID(id.SubscriptionId, id.ResourceGroup, id.MonitorName) | ||
|
|
||
| if _, err = client.Delete(ctx, monitorId); err != nil { |
There was a problem hiding this comment.
why delete the parent resource ?
| } | ||
|
|
||
| // The resource cannot be deleted if the parent NewRelic Monitor exists, the DELETE only clears the monitoredSubscriptionList, so here we add a custom poller | ||
| pollerType := &monitoredSubscriptionDeletedPoller{ |
There was a problem hiding this comment.
why not use the same style as the create to define a method which may be named as resourceMonitoredSubscriptionsWaitForDeleted
| if input == nil { | ||
| return make([]NewRelicMonitoredSubscription, 0) | ||
| } | ||
|
|
||
| results := make([]NewRelicMonitoredSubscription, 0) |
There was a problem hiding this comment.
| if input == nil { | |
| return make([]NewRelicMonitoredSubscription, 0) | |
| } | |
| results := make([]NewRelicMonitoredSubscription, 0) | |
| results := make([]NewRelicMonitoredSubscription, 0) | |
| if input == nil { | |
| return results | |
| } |
| resp.Model.Properties == nil || | ||
| resp.Model.Properties.MonitoredSubscriptionList == nil || | ||
| len(*resp.Model.Properties.MonitoredSubscriptionList) == 0 { | ||
| return metadata.MarkAsGone(id) |
There was a problem hiding this comment.
since "monitored_subscription" is defined as optional, it is possible len(*resp.Model.Properties.MonitoredSubscriptionList) == 0 ?
| resource "azurerm_new_relic_monitored_subscription" "test" { | ||
| monitor_id = azurerm_new_relic_monitor.test.id | ||
|
|
||
| monitored_subscription { |
There was a problem hiding this comment.
if monitored_subscription is optional, if should not exist in basic
teowa
force-pushed
the
newrelic_monitored_sub
branch
from
65a9487 to
ef53dfb
Compare
|
Hi @ms-zhenhua , thanks for reviewing this! However, this resource name can only be default and thus should be inline, following the guidelines. I have submitted #28134 to supersede this. The recommended changes have been incorporated into the new PR. |
Community Note
Description
swagger: https://github.com/Azure/azure-rest-api-specs/blob/a0b2a34b9ff6d324c31e031d6e373fc3ceb38c81/specification/newrelic/resource-manager/NewRelic.Observability/stable/2024-01-01/NewRelic.json#L1372
azure doc: https://learn.microsoft.com/en-us/azure/partner-solutions/new-relic/new-relic-how-to-manage#monitor-multiple-subscriptions
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_newrelic_monitored_subscriptionThis is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.