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

azurerm_app_configuration - add support for identity #8875

Merged
merged 15 commits into from
Oct 20, 2020
Merged

azurerm_app_configuration - add support for identity #8875

merged 15 commits into from
Oct 20, 2020

Conversation

favoretti
Copy link
Collaborator

Adding support for identity to azurerm_app_configuration.

@favoretti
Copy link
Collaborator Author

Acceptance tests pass:

$ TF_ACC=1 go test -v ./azurerm/internal/services/appconfiguration/tests -timeout=600m
=== RUN   TestAccAppConfigurationDataSource_basic
=== PAUSE TestAccAppConfigurationDataSource_basic
=== RUN   TestAccAppConfigurationResource_free
=== PAUSE TestAccAppConfigurationResource_free
=== RUN   TestAccAppConfigurationResource_standard
=== PAUSE TestAccAppConfigurationResource_standard
=== RUN   TestAccAppConfigurationResource_requiresImport
=== PAUSE TestAccAppConfigurationResource_requiresImport
=== RUN   TestAccAppConfigurationResource_complete
=== PAUSE TestAccAppConfigurationResource_complete
=== RUN   TestAccAppConfigurationResource_identity
=== PAUSE TestAccAppConfigurationResource_identity
=== RUN   TestAccAppConfigurationResource_update
=== PAUSE TestAccAppConfigurationResource_update
=== CONT  TestAccAppConfigurationDataSource_basic
=== CONT  TestAccAppConfigurationResource_complete
=== CONT  TestAccAppConfigurationResource_update
=== CONT  TestAccAppConfigurationResource_standard
=== CONT  TestAccAppConfigurationResource_requiresImport
=== CONT  TestAccAppConfigurationResource_identity
=== CONT  TestAccAppConfigurationResource_free
--- PASS: TestAccAppConfigurationResource_free (87.13s)
--- PASS: TestAccAppConfigurationResource_complete (91.11s)
--- PASS: TestAccAppConfigurationResource_standard (91.12s)
--- PASS: TestAccAppConfigurationResource_identity (94.64s)
--- PASS: TestAccAppConfigurationResource_requiresImport (96.07s)
--- PASS: TestAccAppConfigurationResource_update (107.52s)
--- PASS: TestAccAppConfigurationDataSource_basic (113.66s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/appconfiguration/tests	114.840s

Copy link
Contributor

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

Hi @favoretti thanks for this PR! Looking good so far, but I assume since this identity block is optional, could we add some more tests to test the update behaviour of the identity?
For instance, a test from no identity to systemAssigned identity and then back to no identity to confirm the identity block could be properly updated.

@ghost ghost added size/L and removed size/M labels Oct 15, 2020
@favoretti
Copy link
Collaborator Author

favoretti commented Oct 15, 2020

@ArcturusZhang Something like commit below?

$ TF_ACC=1 go test -v ./azurerm/internal/services/appconfiguration/tests -timeout=600m
=== RUN   TestAccAppConfigurationDataSource_basic
=== PAUSE TestAccAppConfigurationDataSource_basic
=== RUN   TestAccAppConfigurationResource_free
=== PAUSE TestAccAppConfigurationResource_free
=== RUN   TestAccAppConfigurationResource_standard
=== PAUSE TestAccAppConfigurationResource_standard
=== RUN   TestAccAppConfigurationResource_requiresImport
=== PAUSE TestAccAppConfigurationResource_requiresImport
=== RUN   TestAccAppConfigurationResource_complete
=== PAUSE TestAccAppConfigurationResource_complete
=== RUN   TestAccAppConfigurationResource_identity
=== PAUSE TestAccAppConfigurationResource_identity
=== RUN   TestAccAppConfigurationResource_identityUpdated
=== PAUSE TestAccAppConfigurationResource_identityUpdated
=== RUN   TestAccAppConfigurationResource_update
=== PAUSE TestAccAppConfigurationResource_update
=== CONT  TestAccAppConfigurationDataSource_basic
=== CONT  TestAccAppConfigurationResource_complete
=== CONT  TestAccAppConfigurationResource_identity
=== CONT  TestAccAppConfigurationResource_identityUpdated
=== CONT  TestAccAppConfigurationResource_update
=== CONT  TestAccAppConfigurationResource_standard
=== CONT  TestAccAppConfigurationResource_free
=== CONT  TestAccAppConfigurationResource_requiresImport
--- PASS: TestAccAppConfigurationResource_standard (91.21s)
--- PASS: TestAccAppConfigurationResource_free (91.26s)
--- PASS: TestAccAppConfigurationResource_complete (92.40s)
--- PASS: TestAccAppConfigurationResource_identity (92.67s)
--- PASS: TestAccAppConfigurationResource_requiresImport (94.91s)
--- PASS: TestAccAppConfigurationResource_update (107.08s)
--- PASS: TestAccAppConfigurationDataSource_basic (107.12s)
--- PASS: TestAccAppConfigurationResource_identityUpdated (132.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/appconfiguration/tests	133.243s

@favoretti
Copy link
Collaborator Author

Adjusted the test to actually check for identity being present.

Copy link
Contributor

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

Hi @favoretti thanks for the revision! I just left some minor comments. And out of curiosity, the SDK supports more values of enum type IdentityType actually, would we also support those other types of identity (such as UserAssignedIdentity)? But it should be fine if we only add the SystemAssigned identity in this PR for now.

@favoretti
Copy link
Collaborator Author

Re: UserAssigned identities - those are a bit more complex and SystemAssigned is what we currently need support for, so in the interest of time getting this in I decided to implement just SystemAssigned. Happy to work on UserAssigned in a separate PR. Sorry for being selfish, time is precious unfortunately, so contributing the bare minimum needs atm :(

Copy link
Contributor

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks @favoretti ! And it is very fine that we could get the user assigned identity supported in another PR.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @favoretti

Thanks for pushing those changes - taking a look through this is mostly looking good, if we can fix up the couple of comments then this should otherwise be good to merge 👍

Thanks!

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

left a couple of comments inline

@favoretti
Copy link
Collaborator Author

"Fixed" it by sending payload with identity type None when clearing.

$ TF_ACC=1 go test -v ./azurerm/internal/services/appconfiguration/tests -timeout=600m -run=TestAccAppConfigurationResource_identityUpdated
=== RUN   TestAccAppConfigurationResource_identityUpdated
=== PAUSE TestAccAppConfigurationResource_identityUpdated
=== CONT  TestAccAppConfigurationResource_identityUpdated
--- PASS: TestAccAppConfigurationResource_identityUpdated (148.10s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/appconfiguration/tests	149.290s

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

otherwise LGTM 👍

@favoretti
Copy link
Collaborator Author

Yep, you're right, works :)

$ TF_ACC=1 go test -v ./azurerm/internal/services/appconfiguration/tests -timeout=600m -run=TestAccAppConfigurationResource_identityUpdated
=== RUN   TestAccAppConfigurationResource_identityUpdated
=== PAUSE TestAccAppConfigurationResource_identityUpdated
=== CONT  TestAccAppConfigurationResource_identityUpdated
--- PASS: TestAccAppConfigurationResource_identityUpdated (147.64s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/appconfiguration/tests	148.858s

@ghost ghost removed the waiting-response label Oct 20, 2020
@tombuildsstuff
Copy link
Contributor

Thanks for pushing those changes @favoretti 👍

@tombuildsstuff tombuildsstuff merged commit 8d05c28 into hashicorp:master Oct 20, 2020
tombuildsstuff added a commit that referenced this pull request Oct 20, 2020
@ghost
Copy link

ghost commented Oct 22, 2020

This has been released in version 2.33.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.33.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Nov 19, 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 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 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.

3 participants