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

New Resource: azurerm_automanage_configuration #21490

Merged
merged 36 commits into from
Jun 1, 2023

Conversation

liuwuliuyun
Copy link
Contributor

New Resource: azurerm_automanage_configuration

Swagger Link

https://github.com/Azure/azure-rest-api-specs/blob/main/specification/automanage/resource-manager/Microsoft.Automanage/stable/2022-05-04/automanage.json

Specital about this resource

Property configuration was flatterned in to a series of properties. I am happy to take suggestions about the method handling these properties.

This first PR is only included partial json properties, more are one the way if this handling method is approved.

Context and discussion about this case:
#19358

Testing Evidence

GOROOT=C:\Program Files\Go #gosetup
GOPATH=C:\Users\yunliu1\go #gosetup
"C:\Program Files\Go\bin\go.exe" test -c -o C:\Users\yunliu1\AppData\Local\Temp\GoLand___go_test_github_com_hashicorp_terraform_provider_azurerm_internal_services_automanage.test.exe github.com/hashicorp/terraform-provider-azurerm/internal/services/automanage #gosetup
"C:\Program Files\Go\bin\go.exe" tool test2json -t C:\Users\yunliu1\AppData\Local\Temp\GoLand___go_test_github_com_hashicorp_terraform_provider_azurerm_internal_services_automanage.test.exe -test.v -test.paniconexit0 #gosetup
=== RUN TestAccAutoManageConfigurationProfile_basic
=== PAUSE TestAccAutoManageConfigurationProfile_basic
=== CONT TestAccAutoManageConfigurationProfile_basic
--- PASS: TestAccAutoManageConfigurationProfile_basic (148.85s)
=== RUN TestAccAutoManageConfigurationProfile_requiresImport
=== PAUSE TestAccAutoManageConfigurationProfile_requiresImport
=== CONT TestAccAutoManageConfigurationProfile_requiresImport
--- PASS: TestAccAutoManageConfigurationProfile_requiresImport (149.39s)
=== RUN TestAccAutoManageConfigurationProfile_complete
=== PAUSE TestAccAutoManageConfigurationProfile_complete
=== CONT TestAccAutoManageConfigurationProfile_complete
--- PASS: TestAccAutoManageConfigurationProfile_complete (149.05s)
=== RUN TestAccAutoManageConfigurationProfile_update
=== PAUSE TestAccAutoManageConfigurationProfile_update
=== CONT TestAccAutoManageConfigurationProfile_update
--- PASS: TestAccAutoManageConfigurationProfile_update (189.91s)
PASS

Process finished with the exit code 0

# Conflicts:
#	vendor/modules.txt
# Conflicts:
#	vendor/modules.txt
# Conflicts:
#	vendor/modules.txt
# Conflicts:
#	internal/services/automanage/automanage_configuration_resource.go
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🥮

@liuwuliuyun
Copy link
Contributor Author

=== CONT TestAccAutoManageConfigurationProfile_requiresImport
--- PASS: TestAccAutoManageConfigurationProfile_requiresImport (328.52s)
=== RUN TestAccAutoManageConfigurationProfile_complete
=== PAUSE TestAccAutoManageConfigurationProfile_complete
=== CONT TestAccAutoManageConfigurationProfile_complete
--- PASS: TestAccAutoManageConfigurationProfile_complete (365.85s)
=== RUN TestAccAutoManageConfigurationProfile_update
=== PAUSE TestAccAutoManageConfigurationProfile_update
=== CONT TestAccAutoManageConfigurationProfile_update
--- PASS: TestAccAutoManageConfigurationProfile_update (459.94s)
PASS

Process finished with the exit code 0

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

In addition to the comment on the basic test config - can we add a test that toggles the antimalware feature on/off through the addition and removal of the block to verify that works?

@liuwuliuyun
Copy link
Contributor Author

=== PAUSE TestAccAutoManageConfigurationProfile_basic
=== CONT TestAccAutoManageConfigurationProfile_basic
--- PASS: TestAccAutoManageConfigurationProfile_basic (150.38s)
=== RUN TestAccAutoManageConfigurationProfile_antimalware
=== PAUSE TestAccAutoManageConfigurationProfile_antimalware
=== CONT TestAccAutoManageConfigurationProfile_antimalware
--- PASS: TestAccAutoManageConfigurationProfile_antimalware (190.12s)
=== RUN TestAccAutoManageConfigurationProfile_requiresImport
=== PAUSE TestAccAutoManageConfigurationProfile_requiresImport
=== CONT TestAccAutoManageConfigurationProfile_requiresImport
--- PASS: TestAccAutoManageConfigurationProfile_requiresImport (152.70s)
=== RUN TestAccAutoManageConfigurationProfile_complete
=== PAUSE TestAccAutoManageConfigurationProfile_complete
=== CONT TestAccAutoManageConfigurationProfile_complete
--- PASS: TestAccAutoManageConfigurationProfile_complete (94.15s)
=== RUN TestAccAutoManageConfigurationProfile_update
=== PAUSE TestAccAutoManageConfigurationProfile_update
=== CONT TestAccAutoManageConfigurationProfile_update
--- PASS: TestAccAutoManageConfigurationProfile_update (201.93s)
PASS

Process finished with the exit code 0

@liuwuliuyun liuwuliuyun requested a review from stephybun June 1, 2023 04:43
@stephybun
Copy link
Member

@liuwuliuyun - last time this PR was opened there were discussions around the ConfigurationDictionary defined in the swagger.

My expectation with the reopening of this is that the properties exposed in the resource's schema are now defined in the swagger as they should be according to this comment thread. We should go back to the service team and have that fixed.

Since we can't accept this in it's current state and without the properties being explicitly defined in the swagger I'm going to close this PR.

@stephybun stephybun closed this Jun 1, 2023
@liuwuliuyun
Copy link
Contributor Author

@stephybun I thought we had another round of discussion and agreed to proceed with with current status with service team supply the schema without changing the swagger. Let me recheck with our PM.

@stephybun stephybun reopened this Jun 1, 2023
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Re-opening after internal discussion. Thanks @liuwuliuyun LGTM 🏔️

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2024
@liuwuliuyun liuwuliuyun deleted the automanage-1 branch September 19, 2024 06:55
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