fix: fixes CI tests ptn/deployment-script/import-image-to-acr
#4973
Conversation
…eHezser/bicep-registry-modules into fix-import-image-to-acr-test
…eHezser/bicep-registry-modules into fix-import-image-to-acr-test
ReneHezser
added
the
Needs: External Changes ⚒️
microsoft-github-policy-service
bot
added
Needs: Triage 🔍
avm-team-linter
bot
added
the
Needs: Core Team 🧞
ReneHezser
removed
the
Needs: External Changes ⚒️
| @@ -28,14 +28,14 @@ module dependencies 'dependencies.bicep' = { | |||
| name: 'dependencies' | |||
| scope: resourceGroup | |||
| params: { | |||
| acrName: 'dep${namePrefix}acr${serviceShort}' | |||
| acrName: uniqueString('dep${namePrefix}acr${serviceShort}', subscription().subscriptionId, resourceGroupName) | |||
There was a problem hiding this comment.
Did we not agree this is a bad call?
There was a problem hiding this comment.
Some for all other uniqueString usages
There was a problem hiding this comment.
We didn't want to use tags. The likeliness of our "manually" generated names to be not unique is higher than this approach.
There was a problem hiding this comment.
Ah I think that was a conversation in a call I wasn't able to join. Pardon
We absolutely should not use tags. We did in the very beginning of AVM's predecessor's CARML's predecessor IaCS and has many issues one may not think off when approaching it. E.g., that they're not supported by all resources, nor propagated immediately.
The 'manually' generated names are fine as the namePrefix is unique to your fork and the serviceShort unique to each test case. Plus it can be consistently applied to all scopes. Using e.g. the subscription() function does not work for any scope above. Same for resource group etc. The other problem is that it becomes impossible to trace the name back to its origin.
If you have a conflict, there are several other subjectively better options (which may be objectively wrong 😄)
- Change the
namePrefix - Add a number suffix to the resource name
- Use the
uniqueString(), but only for a suffix including atake()function to ensure the name is not getting too long
I'd probably argue the 3rd option is probably the best and could be rolled out to each test. Using e.g. uniqueString(subscription().subscriptionId) would ensure the name is always unique to each user and the entire namePrefix would not be needed. For mgmtGroup we could I guess use it's ID. Something like that. Thoughts?
cc: @eriqua (as you may be interested too)
There was a problem hiding this comment.
I'm not a big fan of updating all test cases for an issue which seems to be very limited. We have multiple name chunks decreasing the likelihood of name conflicts, plus not all resources have a globally unique name.
As mentioned in our call I'd suggest the number suffix as the easiest approach. Option 3 Alex suggested is also fine, but I'd suggest to keep it to the owner to change if needed and only for globally unique resource names
There was a problem hiding this comment.
Is anything still happening here?
avm/ptn/deployment-script/import-image-to-acr/tests/e2e/max/main.test.bicep
Show resolved
Hide resolved
...pipelines/staticValidation/psrule/.ps-rule/containerregistry-contenttrust-suppress.Rule.yaml
Outdated
Show resolved
Hide resolved
Description
The names for Azure resources in the test cases are now calculated and should not generate "name in use" exceptions.
For the max test to pass, we are waiting for Resources in nested template not shown if reference is used in the parameter values
Closes #4943
Closes #4918
Pipeline Reference
Type of Change
version.json:version.json.version.json.Checklist
Set-AVMModulelocally to generate the supporting module files.