Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Management Group CreateUpdate Function #6668

Merged

Conversation

wsf11
Copy link

@wsf11 wsf11 commented Apr 28, 2020

Addresses issue #6091

Issue: The Azure REST API returns a 403 if either you do not have access to a management group or a management group cannot be found. When creating a new management group, a check is always performed to see if the management group exists already, causing a 403, and an error.

Fix: Change response not found to response forbidden and log message. In addition, there was a regression in PR #6276 that did not check for the name, rather it was checking for the group_name

@ghost ghost added the size/XS label Apr 28, 2020
@wsf11
Copy link
Author

wsf11 commented May 8, 2020

@katbyte Let me know if you have any feedback on this PR!

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @wsf11! These changes look great. We can get this in once the merge conflicts are sorted.

@@ -36,3 +36,7 @@ func ResponseWasStatusCode(resp autorest.Response, statusCode int) bool { // nol

return false
}

func ResponseWasForbidden(resp autorest.Response) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was added recently so we can remove it.

@katbyte katbyte modified the milestones: v2.10.0, v2.11.0 May 14, 2020
@jackofallops jackofallops self-assigned this May 15, 2020
@jackofallops
Copy link
Member

Quick update - Our CI System still being worked on to be able to run the suite there to be able to merge/release this, but we're getting there... When running locally:

Tests Pass

--- PASS: TestAccAzureRMManagementGroup_basic (54.19s)
--- PASS: TestAccAzureRMManagementGroup_requiresImport (58.97s)
--- PASS: TestAccAzureRMManagementGroup_withName (62.22s)
--- PASS: TestAccDataSourceArmManagementGroup_basic (65.46s)
--- PASS: TestAccAzureRMManagementGroup_withSubscriptions (85.23s)
--- PASS: TestAccAzureRMManagementGroup_nested (88.34s)
--- PASS: TestAccAzureRMManagementGroup_multiLevel (121.81s)
--- PASS: TestAccAzureRMManagementGroup_multiLevelUpdated (147.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/managementgroup/tests	227.362s

@jackofallops jackofallops modified the milestones: v2.11.0, v2.12.0 May 20, 2020
@jackofallops
Copy link
Member

I've looped back around to this now I can test review it properly.
What I've found is that the service appears to respond with a 403 when a get is sent for a non-existent group ID, where it should respond with a 404. This behaviour would have been masked pre-2.0 as there was a conditional test on the import / existence check (features.ShouldResourcesBeImported()) so unless opted into that feature in 1.x, one would never see it.
As a result, I think this needs a slightly different change for this bug. I'll suggest in a review for that.

There's also the name update bug as mentioned by @ArcturusZhang - But I think that will need some careful thought and probably separating the Create and Update functions, as the change of name appears to change the resource ID of the group also.

@ghost ghost added size/S and removed size/XS labels May 21, 2020
@jackofallops
Copy link
Member

Hi @wsf11 - I hope you don't mind, I've pushed a few changes to address the issue and resolve a couple other items. Tests are now passing fully:
image
image

403 issue logged here

@jackofallops jackofallops modified the milestones: v2.12.0, v2.11.0 May 21, 2020
@jackofallops jackofallops merged commit c262ca6 into hashicorp:master May 21, 2020
jackofallops added a commit that referenced this pull request May 21, 2020
@wsf11
Copy link
Author

wsf11 commented May 21, 2020

@jackofallops no worries. Glad I could help with this!

@ghost
Copy link

ghost commented May 22, 2020

This has been released in version 2.11.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.11.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jun 20, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Jun 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants