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_api_management_custom_domain: resource gets updated every time #10636

Merged
merged 5 commits into from
Mar 9, 2021

Conversation

patst
Copy link
Contributor

@patst patst commented Feb 18, 2021

A possible solution for #10253 .

As described in the issue it is unclear if the default hostname for a proxy (<apim-name>.azure-api.net) needs to be included in the azurerm_api_management_custom_domain resource. The test cases include the default hostname, the example in the docs don't.

In both cases there are problems using the azurerm_api_management_custom_domain resource at the moment because tf plan will try to apply changes on subsequent runs (see issue).

My solution filters out the default hostname (as the azurerm_api_management resource does as well, see https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/services/apimanagement/api_management_resource.go#L986 ).

The test cases are succeeding and my local tests are successful with this behaviour:

make testacc TEST='./azurerm/internal/services/apimanagement/api_management_custom_domain_resource_test.go' TESTARGS='-run=TestAccApiManagementCustomDomain_update' TESTTIMEOUT='120m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/apimanagement/api_management_custom_domain_resource_test.go -v -run=TestAccApiManagementCustomDomain_update -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccApiManagementCustomDomain_update
=== PAUSE TestAccApiManagementCustomDomain_update
=== CONT  TestAccApiManagementCustomDomain_update
--- PASS: TestAccApiManagementCustomDomain_update (6269.11s)
PASS
ok      command-line-arguments  6271.038s


make testacc TEST='./azurerm/internal/services/apimanagement/api_management_custom_domain_resource_test.go' TESTARGS='-run=TestAccApiManagementCustomDomain_basic' TESTTIMEOUT='120m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/apimanagement/api_management_custom_domain_resource_test.go -v -run=TestAccApiManagementCustomDomain_basic -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccApiManagementCustomDomain_basic
=== PAUSE TestAccApiManagementCustomDomain_basic
=== CONT  TestAccApiManagementCustomDomain_basic
--- PASS: TestAccApiManagementCustomDomain_basic (4115.71s)
PASS
ok      command-line-arguments  4118.349s

filter proxy default hostname in custom_domain resource

remove default proxy hostname from test cases
@ghost ghost added the size/S label Feb 18, 2021
@manicminer manicminer self-requested a review March 2, 2021 23:37
@sirlatrom
Copy link
Contributor

As the original author of the resource, I'm pleased to see you found a solution to this quirk 😄 How will existing configurations be affected? If they include the default hostname today, will it appear twice in the config (thus becoming invalid) or will it be filtered out then, too?

@patst
Copy link
Contributor Author

patst commented Mar 5, 2021

@sirlatrom I just tried that. It will just work.

You will get a diff in tf plan if you have specified the default hostname at the moment because the hostname is filtered out and the indices for the other hostnames change in the list of domains. This change will be displayed until you remove the default hostname configuration.

If you delete the default hostname there will be no change in tf plan .

@ghost ghost removed the waiting-response label Mar 5, 2021
@sirlatrom
Copy link
Contributor

It will just work.

@patst That is excellent! Thanks again for the improvement!

@patst
Copy link
Contributor Author

patst commented Mar 5, 2021

@sirlatrom sorry my test had a little error. I updated my post above.

@sirlatrom
Copy link
Contributor

sirlatrom commented Mar 5, 2021

@sirlatrom sorry my test had a little error. I updated my post above.

@patst Well that's the second-best possible outcome and is still fine, as it will help current users see they don't need to add it anymore. Maybe it should be added to the website docs for the resource, in order to help people understand what is happening if they have been copying the current example from the docs?

@ghost ghost added the documentation label Mar 5, 2021
@patst
Copy link
Contributor Author

patst commented Mar 5, 2021

@sirlatrom thanks for the feedback, good point. I added a note in the resource documentation to not include the default hostname

Copy link
Member

@manicminer manicminer 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 @patst, this is looking good and I think it's a reasonable compromise to have a persistent plan in pursuit of removing this boilerplate, as long as its nondestructive. I still need to test this myself, but I have a suggestion about simplifying the flatten function in the meantime.

@manicminer
Copy link
Member

Awaiting acceptance test results

@katbyte katbyte added this to the v2.51.0 milestone Mar 9, 2021
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.

tests passed, LGTM 👍

@katbyte katbyte merged commit 85cccac into hashicorp:master Mar 9, 2021
katbyte added a commit that referenced this pull request Mar 9, 2021
@patst patst deleted the 10253-apim-custom-domain branch March 10, 2021 05:13
@ghost
Copy link

ghost commented Mar 12, 2021

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

@ghost
Copy link

ghost commented Apr 9, 2021

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
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

4 participants