Remove ACR Webhooks; Do not manage container settings via Terraform #324
Conversation
9/24 update
merge 9/25
nmiodice
requested review from
KeithJRome,
ThePhen,
erikschlegel,
TechnicallyWilliams,
ianphil,
awkwardindustries and
tasdevani21
| @@ -1,5 +1,5 @@ | |||
| provider "azurerm" { | |||
| version = "~>1.32.0" | |||
| version = "~>1.34.0" | |||
There was a problem hiding this comment.
Is there a changelog available for this? I'm wondering if we need to be on the lookout for any new breaks related to a provider rev.
There was a problem hiding this comment.
Good point. If the upstream is following SemVer, that change should t be breaking (or else the major number would have been incremented).
But, I don’t that they’re using SemVer. I can help check tomorrow, @KeithJRome
There was a problem hiding this comment.
There was a problem hiding this comment.
I upgraded specifically because of a bug related to the provider code that was fixed -- hashicorp/terraform-provider-azurerm#4184.
This was causing issues when trying to run apply multiple times on my test environment
There was a problem hiding this comment.
Note that this bumps the Terraform dependency to 12.8 also (hashicorp/terraform-provider-azurerm#4341)
These changes rolled up into 1.34 look like they are good for us on balance, but I see several behavioral changes that we probably need to be aware of like how certain errors are being reported and some workarounds we have in place now may no longer be needed.
There was a problem hiding this comment.
Can you elaborate on that @KeithJRome ?
There was a problem hiding this comment.
@KeithJRome I read that thread as they are bumping the dependency to 0.12.8. Do you think that means anyone with the 1.34 provider needs to be running on 0.12.8? or is that only for the provider team? @nmiodice did you upgrade TF to 0.12.8? I didn't see that in the commits...
There was a problem hiding this comment.
I looked more closely into each of the changes since 1.32 and the one I was most interested in happens to be the same PR that fixes what you need. The others are very unlikely to cause us problems now that I've dug deeper into the actual code changes involved.
So I don't think this should cause us any problems. Carry On :)
There was a problem hiding this comment.
Yes @iphilpot, I think that means we need to rev our direct dependency on Terraform to 0.12.8, but I don't think this will cause us any problems. A few weird error messages might become less weird, and that's probably the extent of impact for us (aside from the bug that @nmiodice referred to being fixed).
There was a problem hiding this comment.
@KeithJRome @iphilpot I don't think that we should be driving the version of Terraform based off of the version of the library that the providers consume as its not currently possible to satisfy each provider.
Null Resource Provider: https://github.com/terraform-providers/terraform-provider-null/blob/b8568b9b73a19322eaf0c44982f3834ff34854ff/go.mod#L5
Azure AD Resource Provider: https://github.com/terraform-providers/terraform-provider-azuread/blob/0ed698531d3dd13fed4e51469fda2d517f98aa9d/go.mod#L10
|
Aha! So this was letting the .terraform.d folder hang around between jobs/stages, @nmiodice? |
Merge 9/27
* Remove ACR webhook code * Refactor app service module to not require a container to be specified for an app service * Refactor app service module to not care about changes to specified Docker contianer upon subsequent terraform apply commands * Refactor isolated template to leave the docker container unspecified * Update tests to reflect new behavior
All Submissions:
What is the current behavior?
ACR Webhooks are determined to be unsuitable for the deployment strategy that we want to use. This is detailed in #316.
This change removes the ACR webhooks and also prevents Terraform from changing the container settings for existing webapps.
Issue Number: #316
What is the new behavior?
az-hello-worldis unchangedaz-service-single-regiondeploys containerized webapp. The Image is specified after the initial deployment and subsequent terraform deployments will not try to override the container settings. No ACR webhook is configured.az-isolated-service-single-regiondeploys containerized webapp. No image is specified after the initial deployment and subsequent terraform deployments will not try to override the container settings. No ACR webhook is configured.Does this introduce a breaking change?
Any relevant logs, error output, etc?