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

[Bug Fix - azurerm_api_management_api_operation] - Handling example.value none-string types #14848

Merged
merged 4 commits into from
Apr 7, 2022

Conversation

xuzhang3
Copy link
Contributor

@xuzhang3 xuzhang3 commented Jan 7, 2022

Fix issue #14707
example.value can be any type, may be a primitive value or an object, parse non-string type values to JSON string

Models definition: https://github.com/Azure/azure-sdk-for-go/blob/main/services/apimanagement/mgmt/2021-08-01/apimanagement/models.go#L10236

AccTests:

=== RUN   TestAccApiManagementApiOperation_basic
=== PAUSE TestAccApiManagementApiOperation_basic
=== RUN   TestAccApiManagementApiOperation_requiresImport
=== PAUSE TestAccApiManagementApiOperation_requiresImport
=== RUN   TestAccApiManagementApiOperation_customMethod
=== RUN   TestAccApiManagementApiOperationTag_requiresImport
=== PAUSE TestAccApiManagementApiOperationTag_requiresImport
=== RUN   TestAccApiManagementApiOperationTag_update
=== PAUSE TestAccApiManagementApiOperationTag_update
=== CONT  TestAccApiManagementApiOperation_basic
=== CONT  TestAccApiManagementApiOperation_representations
=== CONT  TestAccApiManagementApiOperationTag_requiresImport
=== CONT  TestAccApiManagementApiOperationTag_update
=== CONT  TestAccApiManagementApiOperation_requestRepresentations
=== CONT  TestAccApiManagementApiOperationTag_basic
=== CONT  TestAccApiManagementApiOperation_customMethod
=== CONT  TestAccApiManagementApiOperation_headers
--- PASS: TestAccApiManagementApiOperation_basic (418.13s)
=== CONT  TestAccApiManagementApiOperation_requiresImport
--- PASS: TestAccApiManagementApiOperation_customMethod (419.67s)
--- PASS: TestAccApiManagementApiOperationTag_basic (421.82s)
--- PASS: TestAccApiManagementApiOperation_headers (432.89s)
--- PASS: TestAccApiManagementApiOperationTag_requiresImport (448.78s)
--- PASS: TestAccApiManagementApiOperation_requestRepresentations (497.60s)
--- PASS: TestAccApiManagementApiOperation_representations (508.32s)
--- PASS: TestAccApiManagementApiOperationTag_update (597.76s)
--- PASS: TestAccApiManagementApiOperation_requiresImport (415.80s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/apimanagement 838.375s

@github-actions github-actions bot added the size/M label Jan 7, 2022
@xuzhang3 xuzhang3 closed this Jan 7, 2022
@xuzhang3 xuzhang3 reopened this Jan 7, 2022
@xuzhang3 xuzhang3 force-pushed the f/apim_operation_import_crash branch from 4dcc475 to 76595ca Compare January 7, 2022 08:12
@lonegunmanb lonegunmanb mentioned this pull request Jan 27, 2022
@lonegunmanb
Copy link
Contributor

lonegunmanb commented Jan 27, 2022

This pr will also fix #14869

@maxbog
Copy link
Contributor

maxbog commented Jan 31, 2022

Any chance of getting this merged soon? The bug breaks API operations, they are unusable now.

@maxbog
Copy link
Contributor

maxbog commented Mar 1, 2022

Hi, this breaks API Management operations and the PR is outstanding for 2 months. What needs to happen to get this merged? @tombuildsstuff @katbyte

@LaskiBear
Copy link

Any change to get this fix live ?

@vivuu1989
Copy link

Looking for the fix. Tried with all existing version of azurerm from 2.55.0 to 2.99.0. Its blocking the API operation creation
Is there any workaround for this?

@LaskiBear
Copy link

Looking for the fix. Tried with all existing version of azurerm from 2.55.0 to 2.99.0. Its blocking the API operation creation Is there any workaround for this?

No there arent cause The problem is in Microsoft response schema, that hae breaking change for terraform process

@vivuu1989
Copy link

Looking for the fix. Tried with all existing version of azurerm from 2.55.0 to 2.99.0. Its blocking the API operation creation Is there any workaround for this?

No there arent cause The problem is in Microsoft response schema, that hae breaking change for terraform process

So, we can't create API operations with terraform now? My code was working before with the azurerm versions 2.82 and 2.88 versions. But now after using the same azurerm versions also getting the same error.we are Using terraform cli 1.0.0. .

Please guide me if we can apply any workaround for the time being

@LaskiBear
Copy link

Looking for the fix. Tried with all existing version of azurerm from 2.55.0 to 2.99.0. Its blocking the API operation creation Is there any workaround for this?

No there arent cause The problem is in Microsoft response schema, that hae breaking change for terraform process

So, we can't create API operations with terraform now? My code was working before with the azurerm versions 2.82 and 2.88 versions. But now after using the same azurerm versions also getting the same error.we are Using terraform cli 1.0.0. .

Please guide me if we can apply any workaround for the time being

As far as I tested that The ooerations gets created but import to state fails and that fails The process overall. There as far as I know arent any good workaround. But im pretty new with terraform but i guess you cant go around this problem.
Okay maybe you could Create them with other state file which Will fail but creates them and on that terraform code Repo go with data reference and then on your main Repo use data refence to those operations but you still end up with problem that those operations arent in terraform management. And you ens up with corrupted state file. More Wise guys than me can maybe help but i dont have any good solution than wait Till they get this fixed

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.

Thanks for this PR @xuzhang3 and apologies for the delayed review. In addition to the comment in-line I have two further suggestions

1 . The current tests did not pick up on this panic so can we add a test that reproduces the error and also confirms that this fix works.
2. We should also conditionally unmarshal value as a JSON and fallback to string in the expand function or we may get a diff in the plan.

Once those suggestions have been addressed we can take another look.
Thanks!

internal/services/apimanagement/schemaz/api_management.go Outdated Show resolved Hide resolved
@LaskiBear
Copy link

@xuzhang3 Not trying to pressure but could you find sometime for this, this problem causing problems in our release process. :)

@xuzhang3 xuzhang3 force-pushed the f/apim_operation_import_crash branch from 5314918 to ef5274a Compare March 30, 2022 12:32
@stephybun
Copy link
Member

stephybun commented Apr 1, 2022

Unfortunately we have some test failures @xuzhang3

------- Stdout: -------
=== RUN   TestAccApiManagementApiOperation_representations
=== PAUSE TestAccApiManagementApiOperation_representations
=== CONT  TestAccApiManagementApiOperation_representations
testcase.go:110: Step 1/4 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# azurerm_api_management_api_operation.test will be updated in-place
~ resource "azurerm_api_management_api_operation" "test" {
id                  = "/subscriptions/*******/resourceGroups/acctestRG-220330142519807257/providers/Microsoft.ApiManagement/service/acctestAM-220330142519807257/apis/acctestapi-220330142519807257/operations/acctest-operation"
# (8 unchanged attributes hidden)
~ response {
# (2 unchanged attributes hidden)
~ representation {
# (1 unchanged attribute hidden)
~ example {
name  = "sample"
+ value = <<-EOT
<response>
<user name="bravo24">
<groups>
<group id="abc123" name="First Group" />
<group id="bcd234" name="Second Group" />
</groups>
</user>
</response>
EOT
}
}
}
# (1 unchanged block hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccApiManagementApiOperation_representations (422.46s)
FAIL
------- Stdout: -------
=== RUN   TestAccApiManagementApiOperation_headers
=== PAUSE TestAccApiManagementApiOperation_headers
=== CONT  TestAccApiManagementApiOperation_headers
testcase.go:110: Step 1/2 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# azurerm_api_management_api_operation.test will be updated in-place
~ resource "azurerm_api_management_api_operation" "test" {
id                  = "/subscriptions/*******/resourceGroups/acctestRG-220330142519764820/providers/Microsoft.ApiManagement/service/acctestAM-220330142519764820/apis/acctestapi-220330142519764820/operations/acctest-operation"
# (8 unchanged attributes hidden)
~ response {
# (2 unchanged attributes hidden)
~ header {
name     = "X-Test-Operation"
+ values   = []
# (2 unchanged attributes hidden)
}
~ representation {
# (1 unchanged attribute hidden)
~ example {
name  = "sample"
+ value = <<-EOT
<response>
<user name="bravo24">
<groups>
<group id="abc123" name="First Group" />
<group id="bcd234" name="Second Group" />
</groups>
</user>
</response>
EOT
}
}
}
# (1 unchanged block hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccApiManagementApiOperation_headers (427.08s)
FAIL

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Apr 7, 2022

@stephybun AccTests fixed

=== RUN   TestAccApiManagementApiOperation_basic
=== PAUSE TestAccApiManagementApiOperation_basic
=== RUN   TestAccApiManagementApiOperation_requiresImport
=== PAUSE TestAccApiManagementApiOperation_requiresImport
=== RUN   TestAccApiManagementApiOperation_customMethod
=== PAUSE TestAccApiManagementApiOperation_customMethod
=== RUN   TestAccApiManagementApiOperation_headers
=== PAUSE TestAccApiManagementApiOperation_headers
=== RUN   TestAccApiManagementApiOperation_requestRepresentations
=== PAUSE TestAccApiManagementApiOperation_requestRepresentations
=== RUN   TestAccApiManagementApiOperation_representations
=== PAUSE TestAccApiManagementApiOperation_representations
=== CONT  TestAccApiManagementApiOperation_basic
=== CONT  TestAccApiManagementApiOperation_headers
=== CONT  TestAccApiManagementApiOperation_customMethod
=== CONT  TestAccApiManagementApiOperation_requiresImport
=== CONT  TestAccApiManagementApiOperation_representations
=== CONT  TestAccApiManagementApiOperation_requestRepresentations
--- PASS: TestAccApiManagementApiOperation_customMethod (532.66s)
--- PASS: TestAccApiManagementApiOperation_headers (534.97s)
--- PASS: TestAccApiManagementApiOperation_basic (541.52s)
--- PASS: TestAccApiManagementApiOperation_requiresImport (569.68s)
--- PASS: TestAccApiManagementApiOperation_requestRepresentations (613.88s)
--- PASS: TestAccApiManagementApiOperation_representations (630.12s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/apimanagement 632.326s

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.

Thanks for addressing the test failure @xuzhang3. I have one minor comment relating to the DiffSuppressFunc. I hope you don't mind I went ahead and committed that so that this can be merged and go into the next release.

Comment on lines 491 to 503
func exampleSuppressEquivalentJSONDiffs(k, old, new string, d *pluginsdk.ResourceData) bool {
var ojs interface{}
if json.Unmarshal([]byte(old), &ojs) != nil {
return strings.Compare(old, new) == 0
}

var njs interface{}
if json.Unmarshal([]byte(new), &njs) != nil {
return strings.Compare(old, new) == 0
}

return reflect.DeepEqual(ojs, njs)
}
Copy link
Member

Choose a reason for hiding this comment

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

There is an existing diff suppress func for JSONs in the provider.

Suggested change
func exampleSuppressEquivalentJSONDiffs(k, old, new string, d *pluginsdk.ResourceData) bool {
var ojs interface{}
if json.Unmarshal([]byte(old), &ojs) != nil {
return strings.Compare(old, new) == 0
}
var njs interface{}
if json.Unmarshal([]byte(new), &njs) != nil {
return strings.Compare(old, new) == 0
}
return reflect.DeepEqual(ojs, njs)
}

Optional: true,
Type: pluginsdk.TypeString,
Optional: true,
DiffSuppressFunc: exampleSuppressEquivalentJSONDiffs,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DiffSuppressFunc: exampleSuppressEquivalentJSONDiffs,
DiffSuppressFunc: pluginsdk.SuppressJsonDiff,

@stephybun stephybun force-pushed the f/apim_operation_import_crash branch from 65d708b to c02ca6b Compare April 7, 2022 15:32
@stephybun stephybun merged commit fe79aab into hashicorp:main Apr 7, 2022
@github-actions
Copy link

github-actions bot commented May 8, 2022

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 8, 2022
@xuzhang3 xuzhang3 deleted the f/apim_operation_import_crash branch August 14, 2024 02:40
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

6 participants