`azurerm_new_relic_monitor` - support `monitored_subscription` property #28134

teowa · 2024-11-28T08:35:18Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

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

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_new_relic_monitor - support monitored_subscription property

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

teowa · 2024-12-02T02:35:02Z

internal/services/newrelic/new_relic_monitor_resource.go

+			MinItems: 1,
+			Elem: &pluginsdk.Resource{
+				Schema: map[string]*pluginsdk.Schema{
+					"subscription_id": {


This is not flattened because there are other properties in the monitoredSubscription but not supported by API for now and they might be supported in the future.

ms-zhenhua

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 👍

ms-zhenhua · 2024-12-02T05:59:34Z

internal/services/newrelic/new_relic_monitor_resource.go

+
+			monitoredSubscriptionsClient := metadata.Client.NewRelic.MonitoredSubscriptionsClient
+			monitorId := monitoredsubscriptions.NewMonitorID(id.SubscriptionId, id.ResourceGroupName, id.MonitorName)
+			if err := metadata.Client.NewRelic.MonitoredSubscriptionsClient.CreateOrUpdateThenPoll(ctx, monitorId, monitoredSubscriptionsProperties); err != nil {


Do we still need to create this resource if monitored_subscription is not configured?

fixed, now it will only create if monitored_subscription is not configured

ms-zhenhua · 2024-12-02T06:16:13Z

internal/services/newrelic/new_relic_monitor_resource.go

+
+		resp, err := client.Get(ctx, id)
+		if err != nil {
+			return resp, "Error", fmt.Errorf("retrieving NewRelic Monitored Subscriptions of %s: %+v", id, err)


The id is created by monitoredsubscriptions , not monitors, other logs also have the same problem

updated to use monitors.MonitorId for error message

ms-zhenhua · 2024-12-02T06:38:33Z

internal/services/newrelic/new_relic_monitor_resource.go

+	return func() (interface{}, string, error) {
+		log.Printf("[DEBUG] Checking to see if NewRelic Monitored Subscriptions of %s is available ..", id)
+
+		resp, err := client.Get(ctx, id)


will it return error if the resource is deleted?

updated, now the waiting is removed as tested it is not necessary

teowa · 2024-12-02T07:49:59Z

Hi @ms-zhenhua , thanks for reviewing this! I have changed the code, please kindly take another look.

teowa · 2024-12-03T05:12:42Z

ms-zhenhua · 2024-12-03T08:22:56Z

internal/services/newrelic/new_relic_monitor_resource.go

+						},
+					}
+
+					if err := metadata.Client.NewRelic.MonitoredSubscriptionsClient.CreateOrUpdateThenPoll(ctx, monitorId, monitoredSubscriptionsProperties); err != nil {


Suggested change

if err := metadata.Client.NewRelic.MonitoredSubscriptionsClient.CreateOrUpdateThenPoll(ctx, monitorId, monitoredSubscriptionsProperties); err != nil {

if err := monitoredSubscriptionsClient.CreateOrUpdateThenPoll(ctx, monitorId, monitoredSubscriptionsProperties); err != nil {

ms-zhenhua

Hi @teowa ,

Thank you for the updates. LGTM~

support monitored_subscription

d7978eb

teowa requested a review from a team as a code owner November 28, 2024 08:35

github-actions bot added documentation dependencies size/XL labels Nov 28, 2024

teowa added 2 commits November 28, 2024 08:59

fix error message

81f830a

use delete method if monitor_subscription is removed

6c37d4b

teowa mentioned this pull request Nov 28, 2024

new resource azurerm_newrelic_monitored_subscription #27891

Closed

14 tasks

teowa commented Dec 2, 2024

View reviewed changes

fix

2248ddb

ms-zhenhua suggested changes Dec 2, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 2, 2024

fix

02efb5b

github-actions bot removed the waiting-response label Dec 2, 2024

teowa added 2 commits December 2, 2024 08:21

fix error message and refresh

0733270

fix wait method

8c24a20

github-actions bot added waiting-response enhancement labels Dec 2, 2024

fix lint

e84f41b

github-actions bot removed the waiting-response label Dec 3, 2024

remove wait

e3ed0c4

ms-zhenhua reviewed Dec 3, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 3, 2024

fix client

33e9509

github-actions bot removed the waiting-response label Dec 3, 2024

ms-zhenhua approved these changes Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_new_relic_monitor` - support `monitored_subscription` property #28134

`azurerm_new_relic_monitor` - support `monitored_subscription` property #28134

teowa commented Nov 28, 2024 •

edited by rcskosir

Loading

teowa Dec 2, 2024 •

edited

Loading

ms-zhenhua left a comment

ms-zhenhua Dec 2, 2024

teowa Dec 2, 2024

ms-zhenhua Dec 2, 2024

teowa Dec 2, 2024 •

edited

Loading

ms-zhenhua Dec 2, 2024

teowa Dec 3, 2024 •

edited

Loading

teowa commented Dec 2, 2024

teowa commented Dec 3, 2024

ms-zhenhua Dec 3, 2024

teowa Dec 3, 2024

ms-zhenhua left a comment

	if err := metadata.Client.NewRelic.MonitoredSubscriptionsClient.CreateOrUpdateThenPoll(ctx, monitorId, monitoredSubscriptionsProperties); err != nil {
	if err := monitoredSubscriptionsClient.CreateOrUpdateThenPoll(ctx, monitorId, monitoredSubscriptionsProperties); err != nil {

azurerm_new_relic_monitor - support monitored_subscription property #28134

Are you sure you want to change the base?

azurerm_new_relic_monitor - support monitored_subscription property #28134

Conversation

teowa commented Nov 28, 2024 • edited by rcskosir Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

teowa Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

ms-zhenhua left a comment

Choose a reason for hiding this comment

ms-zhenhua Dec 2, 2024

Choose a reason for hiding this comment

teowa Dec 2, 2024

Choose a reason for hiding this comment

ms-zhenhua Dec 2, 2024

Choose a reason for hiding this comment

teowa Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

ms-zhenhua Dec 2, 2024

Choose a reason for hiding this comment

teowa Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

teowa commented Dec 2, 2024

teowa commented Dec 3, 2024

ms-zhenhua Dec 3, 2024

Choose a reason for hiding this comment

teowa Dec 3, 2024

Choose a reason for hiding this comment

ms-zhenhua left a comment

Choose a reason for hiding this comment

`azurerm_new_relic_monitor` - support `monitored_subscription` property #28134

`azurerm_new_relic_monitor` - support `monitored_subscription` property #28134

teowa commented Nov 28, 2024 •

edited by rcskosir

Loading

teowa Dec 2, 2024 •

edited

Loading

teowa Dec 2, 2024 •

edited

Loading

teowa Dec 3, 2024 •

edited

Loading