11347 supporting azurerm subscription tags #12287

crkg · 2021-06-19T19:40:18Z

@jackofallops this is reference to - #11347 conversation to add tags to azurerm subscription resource. please approve the workflow.

…form-provider-azurerm into support_of_tags_11347

crkg · 2021-06-24T10:04:45Z

@jackofallops , can you please approve the workflow. we will close this topic.

jackofallops

Hi @crkg - Thanks for this PR, it's off to a good start. I've left some comments and changes in line below. If you can address those, I'll re-review asap.
Thanks!

jackofallops · 2021-06-30T10:45:50Z

azurerm/internal/services/subscription/client/client.go

@@ -10,6 +11,7 @@ type Client struct {
 	Client             *subscriptions.Client
 	AliasClient        *subscriptionAlias.AliasClient
 	SubscriptionClient *subscriptionAlias.Client
+	TagsClient         *resources.TagsClient


This should belong in the resource service, can you move it to that package?

jackofallops · 2021-06-30T10:48:28Z

azurerm/internal/services/subscription/subscription_resource.go

+				Type:        pluginsdk.TypeMap,
+				Description: "The tags of the resource",
+				Computed:    true,
+				Optional:    true,


We should be able to use the common schema in the tags package here, e.g. :

"tags": tags.Schema(),

jackofallops · 2021-06-30T11:48:02Z

azurerm/internal/services/subscription/subscription_resource.go

+	if d.HasChange("tags") {
+		tagsDetails := d.Get("tags").(map[string]interface{})
+		resource_tags := resources.Tags{
+			Tags: tags.Expand(tagsDetails),
+		}
+		locks.ByID(subscriptionId)
+		defer locks.UnlockByID(subscriptionId)
+		tagParameters := resources.TagsPatchResource{Operation: "Merge", Properties: &resource_tags}
+		updateTags, updateErr := tagsClient.UpdateAtScope(context.Background(), "subscriptions/"+subscriptionId, tagParameters)
+		if updateErr != nil {
+			if !utils.ResponseWasNotFound(updateTags.Response) {
+				return fmt.Errorf("Adding tag value %q for subscription %q: %+v")
+			}
+		}
+		d.Set("tags", *updateTags.ID)
+	}


Setting the tags should be done before setting the ID, so that is only done when the full create operation is successful.

We can also skip the d.HasChange and just read and send what we have here, as it's the Create.

You can also remove the locking as that is already in place earlier in the function.

Something like this I think will be about right:

Suggested change

if d.HasChange("tags") {

tagsDetails := d.Get("tags").(map[string]interface{})

resource_tags := resources.Tags{

Tags: tags.Expand(tagsDetails),

}

locks.ByID(subscriptionId)

defer locks.UnlockByID(subscriptionId)

tagParameters := resources.TagsPatchResource{Operation: "Merge", Properties: &resource_tags}

updateTags, updateErr := tagsClient.UpdateAtScope(context.Background(), "subscriptions/"+subscriptionId, tagParameters)

if updateErr != nil {

if !utils.ResponseWasNotFound(updateTags.Response) {

return fmt.Errorf("Adding tag value %q for subscription %q: %+v")

}

}

d.Set("tags", *updateTags.ID)

}

t := tags.Expand(d.Get("tags").(map[string]interface{}))

tagsClient := meta.(*clients.Client).Resource.TagsClient

scope := fmt.Sprintf("subscription/%s", subscriptionId)

if _, err = tagsClient.CreateOrUpdateAtScope(ctx, scope, resources.TagsResource{Properties: &resources.Tags{Tags: t}}); err != nil {

return fmt.Errorf("setting tags on %s: %+v", id, err)

}

jackofallops · 2021-06-30T11:57:08Z

azurerm/internal/services/subscription/subscription_resource.go

 }

 func resourceSubscriptionRead(d *pluginsdk.ResourceData, meta interface{}) error {
 	aliasClient := meta.(*clients.Client).Subscription.AliasClient
 	client := meta.(*clients.Client).Subscription.Client
+	// tagsClient := meta.(*clients.Client).Subscription.TagsClient


Can we remove this?

Suggested change

// tagsClient := meta.(*clients.Client).Subscription.TagsClient

jackofallops · 2021-06-30T11:58:01Z

azurerm/internal/services/subscription/subscription_resource.go

@@ -314,13 +358,15 @@ func resourceSubscriptionRead(d *pluginsdk.ResourceData, meta interface{}) error
 		}

 		t = resp.Tags
+		fmt.Println("Read resp tags %q", t)


This looks like a left over debugging line that should be removed?

Suggested change

fmt.Println("Read resp tags %q", t)

jackofallops · 2021-06-30T12:02:35Z

azurerm/internal/services/subscription/subscription_resource.go

+	if d.HasChange("tags") {
+		tagsDetails := d.Get("tags").(map[string]interface{})
+		resource_tags := resources.Tags{
+			Tags: tags.Expand(tagsDetails),
+		}
+		locks.ByID(subscriptionId)
+		defer locks.UnlockByID(subscriptionId)
+		tagPatchParamter := resources.TagsPatchResource{Operation: "Delete", Properties: &resource_tags}
+		updateTags, delerr := tagsClient.UpdateAtScope(context.Background(), "subscriptions/"+subscriptionId, tagPatchParamter)
+		if delerr != nil {
+			return fmt.Errorf("Failed to Remove tags %q from subscription %q: %+v", tags.Flatten(resource_tags.Tags), subscriptionId, delerr)
+		}
+		d.Set("tags", *updateTags.ID)
+	}
+


We shouldn't need to do anything with the tags here, as we will be disabling the subscription.

Suggested change

if d.HasChange("tags") {

tagsDetails := d.Get("tags").(map[string]interface{})

resource_tags := resources.Tags{

Tags: tags.Expand(tagsDetails),

}

locks.ByID(subscriptionId)

defer locks.UnlockByID(subscriptionId)

tagPatchParamter := resources.TagsPatchResource{Operation: "Delete", Properties: &resource_tags}

updateTags, delerr := tagsClient.UpdateAtScope(context.Background(), "subscriptions/"+subscriptionId, tagPatchParamter)

if delerr != nil {

return fmt.Errorf("Failed to Remove tags %q from subscription %q: %+v", tags.Flatten(resource_tags.Tags), subscriptionId, delerr)

}

d.Set("tags", *updateTags.ID)

}

jackofallops · 2021-06-30T12:05:43Z

azurerm/internal/services/subscription/subscription_resource.go

@@ -400,7 +462,7 @@ func resourceSubscriptionDelete(d *pluginsdk.ResourceData, meta interface{}) err
 		return fmt.Errorf("failed to cancel Subscription %q (Alias %q): %+v", subscriptionId, id.Name, err)
 	}

-	return nil
+	return resourceSubscriptionRead(d, meta)


We only ever return nil at the end of a delete, returning read here will result in Terraform presenting an erroneous not-found error to the user on success.

Suggested change

return resourceSubscriptionRead(d, meta)

return nil

jackofallops · 2021-06-30T12:08:10Z

azurerm/internal/services/subscription/subscription_resource_test.go

+				assert.ExistsInAzure(r),
+				assert.Key("tags.%").HasValue("2"),
+				assert.Key("tags.cost_center").HasValue("MSFT"),
+				assert.Key("tags.environment").HasValue("Production"),


Can we keep these in long-hand form as with the rest of the test suite?

Looks like this one was missed from the last review? Can we get these in the form:

Suggested change

assert.ExistsInAzure(r),

assert.Key("tags.%").HasValue("2"),

assert.Key("tags.cost_center").HasValue("MSFT"),

assert.Key("tags.environment").HasValue("Production"),

check.That(data.ResourceName).ExistsInAzure(r),

check.That(data.ResourceName).Key("tags.%").HasValue("2"),

check.That(data.ResourceName).Key("tags.cost_center").HasValue("MSFT"),

check.That(data.ResourceName).Key("tags.environment").HasValue("Production"),

The consistency helps us make bulk changes if/when we update approaches to the test framework.

jackofallops · 2021-06-30T12:10:59Z

azurerm/internal/services/subscription/subscription_resource_test.go

+	subscriptionId := state.Attributes["subscription_id"]
+	// atags := state.Attributes["tags"]
+	tagsResp, err := client.Subscription.TagsClient.GetAtScope(ctx, "subscriptions/"+subscriptionId)
+	if err != nil {
+		return nil, fmt.Errorf("retrieving tags from subscription %q: %+v", subscriptionId, err)
+	}
+	fmt.Println(tagsResp.Properties)


This shouldn't be necessary. The exists and implicit read/import functionality will check the values for the tags. The Exists function itself is simply to check the resource is present, not for any particular setting / configuration

Suggested change

subscriptionId := state.Attributes["subscription_id"]

// atags := state.Attributes["tags"]

tagsResp, err := client.Subscription.TagsClient.GetAtScope(ctx, "subscriptions/"+subscriptionId)

if err != nil {

return nil, fmt.Errorf("retrieving tags from subscription %q: %+v", subscriptionId, err)

}

fmt.Println(tagsResp.Properties)

jackofallops · 2021-06-30T12:12:24Z

azurerm/internal/services/subscription/subscription_resource_test.go

+  tags                    = {
+	environment = "Production"
+    cost_center = "MSFT"
+  }


This is not supported here and will create a test config error when run

Suggested change

tags = {

environment = "Production"

cost_center = "MSFT"

}

crkg · 2021-06-30T12:41:21Z

Thanks Steve, i will work on the recommendation and push the changes.

crkg · 2021-07-06T09:15:09Z

Hi @crkg - Thanks for this PR, it's off to a good start. I've left some comments and changes in line below. If you can address those, I'll re-review asap.
Thanks!

Hi Steve,

PR is ready to be merged with no conflicts. please take a moment to review and merge.

Thank you.

jackofallops

Hi @crkg - Thanks for the updates, I've left a couple more comments inline below. If you can address those I'll make time to test this as the Subscription resource isn't compatible with our CI test runner currently, so needs running by hand.

Thanks!

jackofallops · 2021-07-14T07:21:08Z

azurerm/internal/services/subscription/subscription_resource.go

+				return fmt.Errorf("Updating tag value %q for subscription %q: %+v", tags.Flatten(resource_tags.Tags), *subscriptionId, updateErr)
+			}
+		}
+		d.Set("tags", *updateTags.ID)


We can remove this, it should be handled by the Read.

Suggested change

d.Set("tags", *updateTags.ID)

jackofallops · 2021-07-14T07:24:35Z

azurerm/internal/services/subscription/subscription_resource_test.go

+				assert.ExistsInAzure(r),
+				assert.Key("tags.%").HasValue("2"),
+				assert.Key("tags.cost_center").HasValue("MSFT"),
+				assert.Key("tags.environment").HasValue("Production"),


Looks like this one was missed from the last review? Can we get these in the form:

Suggested change

assert.ExistsInAzure(r),

assert.Key("tags.%").HasValue("2"),

assert.Key("tags.cost_center").HasValue("MSFT"),

assert.Key("tags.environment").HasValue("Production"),

check.That(data.ResourceName).ExistsInAzure(r),

check.That(data.ResourceName).Key("tags.%").HasValue("2"),

check.That(data.ResourceName).Key("tags.cost_center").HasValue("MSFT"),

check.That(data.ResourceName).Key("tags.environment").HasValue("Production"),

The consistency helps us make bulk changes if/when we update approaches to the test framework.

jackofallops · 2021-07-23T07:23:11Z

Hi @crkg - There's still an outstanding change requested from the previous review, specifically updating the test cases where assert := check.That(data.ResourceName) is used to being the long-hand version. If you can update those, I'll re-review and get the tests run. 👍
Thanks!

…form-provider-azurerm into support_of_tags_11347

crkg · 2021-07-23T10:38:01Z

Hi @crkg - There's still an outstanding change requested from the previous review, specifically updating the test cases where assert := check.That(data.ResourceName) is used to being the long-hand version. If you can update those, I'll re-review and get the tests run. 👍
Thanks!

Hi Steve, sorry i missed to commit those changes in previous commit. thank you for the suggestions. its fixed please approve the workflow.

crkg · 2021-08-03T10:59:41Z

Hi @crkg - There's still an outstanding change requested from the previous review, specifically updating the test cases where assert := check.That(data.ResourceName) is used to being the long-hand version. If you can update those, I'll re-review and get the tests run. 👍
Thanks!

Hi Steve, sorry i missed to commit those changes in previous commit. thank you for the suggestions. its fixed please approve the workflow.

@jackofallops ( Steve) can you please review and approve the changes.

jackofallops · 2021-08-03T14:56:25Z

Hi @crkg - We have an issue testing subscription creation/update currently, so I'm afraid this is blocked until that support ticket is resolved. I'll loop back to this just as soon as we're able to test.

crkg · 2021-08-12T17:16:25Z

Hi @crkg - We have an issue testing subscription creation/update currently, so I'm afraid this is blocked until that support ticket is resolved. I'll loop back to this just as soon as we're able to test.

Hi Steve, thank you. let me know when we are good to loop back this PR.

crkg · 2021-09-12T17:09:15Z

Hi @crkg - We have an issue testing subscription creation/update currently, so I'm afraid this is blocked until that support ticket is resolved. I'll loop back to this just as soon as we're able to test.

Hi Steve, thank you. let me know when we are good to loop back this PR.

Hi Steve, may i know if this feature can be plugged back, I was waiting for a confirmation on a blocked support ticket if its resolved?

donwlewis · 2021-10-01T20:38:47Z

It would be great to see this get implemented soon. Thanks!

pinophyta · 2021-10-03T16:00:41Z

Looking forward to having this useful feature in the provider soon!

juicybaba · 2021-10-07T22:32:43Z

Looking forward to have this feature enabled. @crkg @jackofallops

katbyte · 2021-10-21T22:53:21Z

Hey all, sorry that this PR has been languishing here for so long. We're still working to unblock our ability to test - but progress has finally been made on that front so are now working towards getting this queued up. No timeframe to share yet but hopefully sooner than later!

Eslam10 · 2021-11-30T15:35:16Z

Hello,
Would someone please share the updates on this ?
Thank you.

jackofallops · 2021-12-02T16:49:52Z

Hi @crkg - we've been able to resolve the testing issues for this. A lot has changed in the provider structure since this was first opened, so I've had to take your work forward to another PR to add changes I found to be necessary. Thanks again for your work on this, and your continued patience in getting this through. I'm going to close this PR out, please subscribe to #14445 for further updates.

@crkg

…14445) Supersedes #12287 Thanks to @crkg for the initial work.

crkg · 2021-12-03T15:27:18Z

Hi @crkg - we've been able to resolve the testing issues for this. A lot has changed in the provider structure since this was first opened, so I've had to take your work forward to another PR to add changes I found to be necessary. Thanks again for your work on this, and your continued patience in getting this through. I'm going to close this PR out, please subscribe to #14445 for further updates.

Thank you for the feature, much appreciated.

github-actions · 2022-01-09T02:11:53Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

11347 supporting azurerm subscription tags

4453756

github-actions bot added the size/L label Jun 19, 2021

Merge branch 'master' into support_of_tags_11347

37143fa

github-actions bot added size/M and removed size/L labels Jun 19, 2021

Ramesh Kallikuppa GOPALAKRISHNA added 2 commits June 23, 2021 09:48

adding tagsClient in subscription client

34b1f28

Merge branch 'support_of_tags_11347' of https://github.com/crkg/terra…

dcb9045

…form-provider-azurerm into support_of_tags_11347

github-actions bot added size/L and removed size/M labels Jun 23, 2021

fixing git workflow errors

d343792

katbyte requested a review from jackofallops June 23, 2021 07:02

katbyte assigned jackofallops Jun 23, 2021

fixing git workflow errors

15cf652

github-actions bot added size/M and removed size/L labels Jun 23, 2021

katbyte added enhancement service/subscription labels Jun 24, 2021

jackofallops requested changes Jun 30, 2021

View reviewed changes

jackofallops added the waiting-response label Jun 30, 2021

Ramesh Kallikuppa GOPALAKRISHNA and others added 4 commits July 1, 2021 00:14

modifying code be recommendation from Steve

b3f7db6

fixing gofmt and linter

db0fbb4

Merge branch 'terraform-providers:master' into support_of_tags_11347

ebc7e33

Update subscription_resource.go

49b8f41

jackofallops removed the waiting-response label Jul 9, 2021

jackofallops requested changes Jul 14, 2021

View reviewed changes

jackofallops added the waiting-response label Jul 14, 2021

Merge branch 'terraform-providers:master' into support_of_tags_11347

4c8c549

github-actions bot removed the waiting-response label Jul 22, 2021

crkg requested a review from jackofallops July 23, 2021 03:51

Merge branch 'terraform-providers:master' into support_of_tags_11347

01a6a2e

jackofallops added the waiting-response label Jul 23, 2021

crkg and others added 4 commits July 23, 2021 15:43

fixing the tflint error and the review fix

4fb8a46

Merge branch 'terraform-providers:master' into support_of_tags_11347

17cd32d

Merge branch 'support_of_tags_11347' of https://github.com/crkg/terra…

41138a1

…form-provider-azurerm into support_of_tags_11347

fixing lint error and missing review

b04643d

github-actions bot removed the waiting-response label Jul 23, 2021

jackofallops added this to the Blocked milestone Aug 3, 2021

jhisc mentioned this pull request Aug 18, 2021

Support for tags on azurerm_subscription resource #11103

Closed

jackofallops mentioned this pull request Dec 2, 2021

azurerm_subscription - add support for setting and updating tags #14445

Merged

jackofallops closed this Dec 2, 2021

katbyte pushed a commit that referenced this pull request Dec 2, 2021

azurerm_subscription - add support for setting and updating tags (#…

661037e

…14445) Supersedes #12287 Thanks to @crkg for the initial work.

crkg deleted the support_of_tags_11347 branch December 9, 2021 05:18

github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

11347 supporting azurerm subscription tags #12287

11347 supporting azurerm subscription tags #12287

crkg commented Jun 19, 2021 •

edited

Loading

crkg commented Jun 24, 2021

jackofallops left a comment

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

jackofallops Jul 14, 2021

jackofallops Jun 30, 2021

jackofallops Jun 30, 2021

crkg commented Jun 30, 2021

crkg commented Jul 6, 2021

jackofallops left a comment

jackofallops Jul 14, 2021

jackofallops Jul 14, 2021

jackofallops commented Jul 23, 2021

crkg commented Jul 23, 2021 •

edited

Loading

crkg commented Aug 3, 2021

jackofallops commented Aug 3, 2021

crkg commented Aug 12, 2021

crkg commented Sep 12, 2021

donwlewis commented Oct 1, 2021

pinophyta commented Oct 3, 2021

juicybaba commented Oct 7, 2021

katbyte commented Oct 21, 2021

Eslam10 commented Nov 30, 2021

jackofallops commented Dec 2, 2021

crkg commented Dec 3, 2021

github-actions bot commented Jan 9, 2022

11347 supporting azurerm subscription tags #12287

11347 supporting azurerm subscription tags #12287

Conversation

crkg commented Jun 19, 2021 • edited Loading

crkg commented Jun 24, 2021

jackofallops 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

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

crkg commented Jun 30, 2021

crkg commented Jul 6, 2021

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackofallops commented Jul 23, 2021

crkg commented Jul 23, 2021 • edited Loading

crkg commented Aug 3, 2021

jackofallops commented Aug 3, 2021

crkg commented Aug 12, 2021

crkg commented Sep 12, 2021

donwlewis commented Oct 1, 2021

pinophyta commented Oct 3, 2021

juicybaba commented Oct 7, 2021

katbyte commented Oct 21, 2021

Eslam10 commented Nov 30, 2021

jackofallops commented Dec 2, 2021

crkg commented Dec 3, 2021

github-actions bot commented Jan 9, 2022

crkg commented Jun 19, 2021 •

edited

Loading

crkg commented Jul 23, 2021 •

edited

Loading