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_spring_cloud_custom_domain" #10404

Merged
merged 7 commits into from
Feb 25, 2021

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented Feb 1, 2021

  • this resource needs two computed property from app and certificate, so this PR also contains small changes of app and certificate resource
  • to run the acctest, you should have a domain and delegate to azure dns zone, and then set the environment

katbyte
katbyte previously requested changes Feb 1, 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.

Thanks @njuCZ - just a couple comments but otherwise looks good

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @njuCZ - Thanks for this PR. There's a couple of comments to address below and then I should be able to pull and test locally.

@njuCZ
Copy link
Contributor Author

njuCZ commented Feb 3, 2021

@jackofallops thanks for your suggestions, I have updated this PR, I reruned the test in my local and it could succeed

@njuCZ njuCZ requested a review from jackofallops February 19, 2021 02:51
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @njuCZ - Thanks for this PR. I've left some comments and questions. If you can address those I'll continue testing/reviewing.
Thanks

Comment on lines 60 to 64
"thumbprint": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringIsNotEmpty,
},
Copy link
Member

Choose a reason for hiding this comment

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

I think this will need to be ForceNew - the service can accept this being updated in place, but if the underlying azurerm_spring_cloud_certificate needs to be recreated, this association here will cause the certificate delete to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could changed it to forcenew, but I could not understand the reason. I have tried to modify the name of azurerm_spring_cloud_certificate to let it recreate, it actually failed and reoprts: The cert xxx is used by domain xx.xx.xx, can not be deleted. This behaviour is controlled by the backend service, I think change it to forcenew does not help.

what's more, if this field is forcenew, I think maybe field certificate_name should also be forcenew ?

another thought, since certificate_name and thumbprint both comes from azurerm_spring_cloud_certificate, maybe I could directly reference the resource id

Copy link
Member

@jackofallops jackofallops Feb 24, 2021

Choose a reason for hiding this comment

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

The cert xxx is used by domain xx.xx.xx, can not be deleted

Correct, this is exactly the behaviour I was suggesting we avoid. If a user updates / replaces the referenced certificate, Terraform will be unable to delete that referenced certificate and will fail apply, since it will be in use. By having ForceNew on this field (which will have a different value for a replace certificate, where name won't necessarily), we recreate the Custom Domain to release the association and allow the underlying certificate to be replaced. Does that make sense?

maybe I could directly reference the resource id

I think this would be too restrictive? The name may not be the only one on the certificate and another name on the same certificate (and subsequently same thumbprint) could be changed without needing to recreate this virtual resource? If it's a difference certificate, then the thumbprint will change and trigger the ForceNew in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks you for your explanation. I have made it forcenew

website/docs/r/spring_cloud_app.html.markdown Outdated Show resolved Hide resolved
@njuCZ njuCZ requested a review from jackofallops February 23, 2021 09:04
@katbyte katbyte added this to the v2.49.0 milestone Feb 23, 2021
@njuCZ
Copy link
Contributor Author

njuCZ commented Feb 25, 2021

@jackofallops thanks for your carefully review. I have refined this PR.

Copy link
Member

@jackofallops jackofallops 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 the updates @njuCZ - This LGTM now 👍 Assuming the test results look good, I'll get this merged later today.

@jackofallops
Copy link
Member

Tests are looking good 👍

@jackofallops jackofallops merged commit 8955969 into hashicorp:master Feb 25, 2021
jackofallops added a commit that referenced this pull request Feb 25, 2021
@ghost
Copy link

ghost commented Feb 26, 2021

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

@ghost
Copy link

ghost commented Mar 28, 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 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 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.

3 participants