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_app_service_certificate_binding #9415

Merged
merged 16 commits into from
Dec 9, 2020
Merged

New Resource: azurerm_app_service_certificate_binding #9415

merged 16 commits into from
Dec 9, 2020

Conversation

AdamCoulterOz
Copy link
Contributor

@AdamCoulterOz AdamCoulterOz commented Nov 22, 2020

This fixes #8069.

This is a separation of the certificate binding from the existing azurerm_app_service_custom_hostname_binding. This new resource azurerm_app_service_certificate_binding breaks the circular dependency with the azurerm_app_service_managed_certificate needing to be created after the azurerm_app_service_custom_hostname_binding resource, but before the certificate association part of it.

This still needs tests and documentation, will add in the next few days.

@phekmat / @imod - does this help address both of your contexts within #8069 ?

@ghost ghost added the size/L label Nov 22, 2020
@imod
Copy link

imod commented Nov 23, 2020

I think it does 👍

@imod
Copy link

imod commented Nov 23, 2020

thinking about it again... sorry i was a bit confused: the original issue was to introduce the ability to create 'managed certificates' (#4824). So this is "only" the prerequisite.
The reason why I referenced https://github.com/pulumi/pulumi-azure-nextgen/issues/129 was the fact that pulumi suffers the same problem - with the new implementation (azure-nextgen) which does not use the azure terraform provider anymore.

@AdamCoulterOz
Copy link
Contributor Author

@imod yeah I've been chipping away at @4824... we have a few PRs open right now which will complete it once finished... #9378 #9415 #9409... I've got a branch with them all merged if you want to build you own for now...

https://github.com/AdamCoulterOz/terraform-provider-azurerm/tree/app_service_and_custom_domain

@AdamCoulterOz
Copy link
Contributor Author

@jackofallops we need this included in the next azurerm release... what can I do to help get it there?

@jackofallops
Copy link
Member

@jackofallops we need this included in the next azurerm release... what can I do to help get it there?

Hi @AdamCoulterOz - I have this covered in the PR 9631 mentioned above, hopefully get it into today's release.

@ghost ghost added size/XL documentation and removed size/L labels Dec 7, 2020
@AdamCoulterOz AdamCoulterOz changed the title New Resource: azurerm_app_service_custom_hostname_certificate_binding New Resource: azurerm_app_service_certificate_binding Dec 7, 2020
@ghost ghost added size/XXL and removed size/XL labels Dec 8, 2020
@jackofallops
Copy link
Member

Tests passing locally:

=== RUN   TestAccAzureRMAppServiceCertificateBinding_basic
=== PAUSE TestAccAzureRMAppServiceCertificateBinding_basic
=== CONT  TestAccAzureRMAppServiceCertificateBinding_basic
--- PASS: TestAccAzureRMAppServiceCertificateBinding_basic (247.86s)
=== RUN   TestAccAzureRMAppServiceCertificateBinding_requiresImport
=== PAUSE TestAccAzureRMAppServiceCertificateBinding_requiresImport
=== CONT  TestAccAzureRMAppServiceCertificateBinding_requiresImport
--- PASS: TestAccAzureRMAppServiceCertificateBinding_requiresImport (269.07s)
PASS

@jackofallops jackofallops added this to the v2.40.0 milestone Dec 8, 2020
@jackofallops jackofallops self-assigned this Dec 8, 2020
Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @AdamCoulterOz @jackofallops

Thanks for this PR.

I've taken a look through and left some comments inline but this otherwise LGTM 👍

Thanks!

@jackofallops
Copy link
Member

Post review fix tests passing:

=== PAUSE TestAccAzureRMAppServiceCertificateBinding_basic
=== CONT  TestAccAzureRMAppServiceCertificateBinding_basic
--- PASS: TestAccAzureRMAppServiceCertificateBinding_basic (352.98s)
=== RUN   TestAccAzureRMAppServiceCertificateBinding_basicSniEnabled
=== PAUSE TestAccAzureRMAppServiceCertificateBinding_basicSniEnabled
=== CONT  TestAccAzureRMAppServiceCertificateBinding_basicSniEnabled
--- PASS: TestAccAzureRMAppServiceCertificateBinding_basicSniEnabled (308.68s)
=== RUN   TestAccAzureRMAppServiceCertificateBinding_requiresImport
=== PAUSE TestAccAzureRMAppServiceCertificateBinding_requiresImport
=== CONT  TestAccAzureRMAppServiceCertificateBinding_requiresImport
--- PASS: TestAccAzureRMAppServiceCertificateBinding_requiresImport (371.84s)
PASS```

@jackofallops jackofallops merged commit c34d5b9 into hashicorp:master Dec 9, 2020
jackofallops added a commit that referenced this pull request Dec 9, 2020
@AdamCoulterOz AdamCoulterOz deleted the app_service_certificate_binding branch December 9, 2020 23:32
@ghost
Copy link

ghost commented Dec 10, 2020

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

@ghost
Copy link

ghost commented Jan 8, 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 Jan 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants