[BUG] ACR Autocreate may not actually test if ACR already exists #43
Comments
timothymeyers
added
bug
## [BUG] ACR Autocreate may not actually test if ACR already exists Azure-Samples#43 ### Issue - **Problem**: After a Bash session reset, `CONTAINER_REGISTRY_SERVER` might not be set, causing incorrect logging about creating a new ACR. - **Steps to Reproduce**: 1. Run `deploy.sh` to create an ACR. 2. Reset the Bash session or unset `CONTAINER_REGISTRY_SERVER`. 3. Run `deploy.sh` again; it incorrectly logs creating a new ACR. ### Expected Behavior The script should check if an ACR exists and log correctly. ### Fix Updated `createAcrIfNotExists()` to always check for an existing ACR and log appropriately. ### Updated Function ```bash createAcrIfNotExists() { printf "Checking if container registry exists... " local existingRegistry existingRegistry=$(az acr show --name $CONTAINER_REGISTRY_SERVER --query loginServer -o tsv 2>/dev/null) if [ $? -eq 0 ]; then printf "Yes. Using existing registry '$existingRegistry'.\n" CONTAINER_REGISTRY_SERVER=$existingRegistry return 0 fi printf "No. Creating container registry... " AZURE_ACR_DEPLOY_RESULT=$(az deployment group create --resource-group $RESOURCE_GROUP --name "acr-deployment" --template-file core/acr/acr.bicep --only-show-errors --no-prompt -o json \ --parameters "name=$CONTAINER_REGISTRY_SERVER") exitIfCommandFailed $? "Error creating container registry, exiting..." CONTAINER_REGISTRY_SERVER=$(jq -r .properties.outputs.loginServer.value <<< $AZURE_ACR_DEPLOY_RESULT) exitIfValueEmpty "$CONTAINER_REGISTRY_SERVER" "Unable to parse container registry login server from deployment, exiting..." printf "Container registry '$CONTAINER_REGISTRY_SERVER' created.\n" } ``` ### Summary - **Always Checks**: Now always checks if the ACR exists. - **Correct Logging**: Logs whether it found an existing ACR or created a new one. This fix ensures accurate logging and proper handling of the ACR status across sessions. Thanks for your contribution!
|
In your example, whether it's the first or any subsequent run, the "CONTAINER_REGISTRY_SERVER" variable is always unset. Given that it is an optional parameter, if you intend to use an existing registry from a prior run of deploy.sh or your subscription, then it's probably more appropriate to set this variable to your existing ACR login server. If it's left unset, the Bicep file will simply generate the same unique ACR name on subsequent runs (Assuming you're using the same subscription and resource group) and will not apply any updates to your existing infrastructure. I'm not sure if there's a way to log this in Bicep. It seems a bit counter-intuitive to check if a resource exists and to use it when the resource identifier is not provided. |
|
Nice catch and makes a lot of sense. I'm working on a PR (#106) to clean up the logging so it's a bit clearer what is happening. |
Describe the bug
Note, I believe the desired result is always achieved -- we end up using the existing ACR resources if it exists. This is mostly a bash logic / logging issue and absolutely a low level nitpick.
During subsequent runs of deploy.sh, the
createAcrIfNotExists()function has anif [ !-zblock that is not always run. If your bash session is reset between runs (e.g., my Codespace timed out and restarted!), theCONTAINER_REGISTRY_SERVERenv variable may not be set any more after the first run.To Reproduce
deploy.shwithout an ACR created (have one created for you using new autocreate logic). This will result in theCONTAINER_REGISTRY_SERVERvariable being set for you.CONTAINER_REGISTRY_SERVERis not set. (or just unset the variable).deploy.shagain. The output will say "Creating container registry" even though it is not actually creating a new one. Theaz deployment group createcall on line 511 will return the existing server and everything proceeds as normal.Expected behavior
I would expect the script output to say it is checking for the server to exist and that an existing server was found.
The text was updated successfully, but these errors were encountered: