-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use Rawxml format for global API policies string #9296
Conversation
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.
LGTM - thanks for this @knutwannheden 👍
@@ -1343,7 +1343,7 @@ func expandApiManagementPolicies(input []interface{}) (*apimanagement.PolicyCont | |||
if xmlContent != "" { | |||
return &apimanagement.PolicyContract{ | |||
PolicyContractProperties: &apimanagement.PolicyContractProperties{ | |||
Format: apimanagement.XML, | |||
Format: apimanagement.Rawxml, |
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.
Running the tests for this we get the following diff:
<policies>
<inbound>
<set-variable name=\"abc\" value=\"@(context.Request.Headers.GetValueOrDefault("X-Header-Name", ""))\" />
<find-and-replace from=\"xyz\" to=\"abc\" />
</inbound>
</policies>
" => "
<policies>
<inbound>
<set-variable name=\"abc\" value=\"@(context.Request.Headers.GetValueOrDefault(\"X-Header-Name\", \"\"))\" />
<find-and-replace from=\"xyz\" to=\"abc\" />
</inbound>
</policies>
To fix that we'll also need to update this line: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/services/apimanagement/api_management_resource.go#L362
to ignore the differences when comparing the diff - since these get returned encoded from the API, we can do this using this function: https://github.com/terraform-providers/terraform-provider-azurerm/blob/841a9c9f7da454581b6fa761754909e90d3f8b7e/azurerm/internal/services/apimanagement/api_management_product_policy_resource.go#L47
As such could we update this so that this test passes?
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 review and the guidance. I will update the code accordingly.
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.
How come the acceptance test didn't fail in GitHub? Are only unit tests performed there?
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.
I was unfortunately not able to get the acceptance test to run in my VS Code devcontainer. I think I would have to set a whole bunch of ARM_*
environment variables somehow.
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.
Yeah Github Actions only runs the unit tests - the acceptance tests take longer (and can cost a lot) - which is why we run these manually using another tool
No worries - I'll kick off the tests again now but I think we should be good to go :)
4a1a6fb
to
2722989
Compare
Tests look good 👍 |
This has been released in version 2.37.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.37.0"
}
# ... other configuration ... |
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! |
Fixes #9223