-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Support non-string type for parameters of azurerm_managed_application #8632
Conversation
Bump! Please merge this asap we really need this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pr @neil-yechenwei - looking at this i'm wondering if it would be better to instead support both and make them conflict? either parameters for those who need a simple key=value
and then paramters_as_json
for users who want to just pass in a json block?
d0a2c33
to
e6cdd2f
Compare
@katbyte , thanks for your comments. I've updated code. Please have an another look. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @neil-yechenwei - couple more comments to address and this should be good to merge
azurerm/internal/services/managedapplications/managed_application_resource.go
Outdated
Show resolved
Hide resolved
@@ -94,6 +94,21 @@ func TestAccManagedApplication_update(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestAccManagedApplication_parameterValues(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a test that goes string -> non-string -> string and checking to make sure the correct property is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tc
@katbyte , thanks for your comments. I've updated code. Please have an another look. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @neil-yechenwei, looks good. There are two test failures:
------- Stdout: -------
=== RUN TestAccManagedApplication_complete
=== PAUSE TestAccManagedApplication_complete
=== CONT TestAccManagedApplication_complete
testing.go:620: Step 1/2 error: Error running apply: exit status 1
Error: A resource with the ID "/subscriptions/*******/providers/Microsoft.MarketplaceOrdering/agreements/cisco/offers/meraki-vmx/plans/meraki-vmx100" already exists - to be managed via Terraform this resource needs to be imported into the State. Please see the resource documentation for "azurerm_marketplace_agreement" for more information.
on terraform_plugin_test.tf line 35, in resource "azurerm_marketplace_agreement" "test":
35: resource "azurerm_marketplace_agreement" "test" {
--- FAIL: TestAccManagedApplication_complete (106.85s)
FAIL
------- Stderr: -------
2021/04/27 04:03:28 [DEBUG] not using binary driver name, it's no longer needed
2021/04/27 04:03:28 [DEBUG] not using binary driver name, it's no longer needed
------- Stdout: -------
=== RUN TestAccManagedApplication_update
=== PAUSE TestAccManagedApplication_update
=== CONT TestAccManagedApplication_update
testing.go:620: Step 3/6 error: Error running apply: exit status 1
Error: A resource with the ID "/subscriptions/*******/providers/Microsoft.MarketplaceOrdering/agreements/cisco/offers/meraki-vmx/plans/meraki-vmx100" already exists - to be managed via Terraform this resource needs to be imported into the State. Please see the resource documentation for "azurerm_marketplace_agreement" for more information.
on terraform_plugin_test.tf line 35, in resource "azurerm_marketplace_agreement" "test":
35: resource "azurerm_marketplace_agreement" "test" {
--- FAIL: TestAccManagedApplication_update (437.25s)
FAIL
------- Stderr: -------
2021/04/27 04:03:28 [DEBUG] not using binary driver name, it's no longer needed
2021/04/27 04:03:28 [DEBUG] not using binary driver name, it's no longer needed
once fixed this should be good to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's an ongoing problem on master to, these tests always fail in TC - should look into a way to fix them so they don't fail anymore but it doesn't block this pr 👍
This has been released in version 2.57.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.57.0"
}
# ... other configuration ... |
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. |
fixes #8608