-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(arm): ACREnableZoneRedundancy #6326
base: main
Are you sure you want to change the base?
feat(arm): ACREnableZoneRedundancy #6326
Conversation
return CheckResult.FAILED | ||
|
||
# check each replica. default=false | ||
replications = conf.get("replications", {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the replications
field in Microsoft.ContainerRegistry/registries/replications
or Microsoft.ContainerRegistry/registries
, are you sure this is the field we should check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the error
can you confirm me please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was for the resource template, Are you sure that those resources have the field replications
to configure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and I think it's good now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are looking for the field replications
in the Microsoft.ContainerRegistry/registries
resource type
but I don't see here the replications
filed
I changed the mistakes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your UT is failing, please fix it and the other failing jobs.
# check registry. default=false | ||
properties = conf.get("properties") | ||
if properties and isinstance(properties, dict): | ||
if properties.get("zoneRedundancy", []) != [True]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zoneRedundancy should be Enabled or Disabled and not a boolean value
…REnableZoneRedundancy
# check registry. default=false | ||
properties = conf.get("properties") | ||
if properties and isinstance(properties, dict): | ||
if properties.get("zoneRedundancy") == "disabled": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if properties.get("zoneRedundancy") == "disabled": | |
if properties.get("zoneRedundancy") == "Disabled": |
"properties": { | ||
"adminUserEnabled": "[parameters('acrAdminUserEnabled')]", | ||
"zoneRedundancy": | ||
"disabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"disabled" | |
"Disabled" |
"[resourceId('Microsoft.ContainerRegistry/registries/', parameters('acrName'))]" | ||
], | ||
"properties": { | ||
"zoneRedundancy": "disabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"zoneRedundancy": "disabled" | |
"zoneRedundancy": "Disabled" |
}, | ||
"properties": { | ||
"adminUserEnabled": "[parameters('acrAdminUserEnabled')]", | ||
"zoneRedundancy": "enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"zoneRedundancy": "enabled" | |
"zoneRedundancy": "Enabled" |
"[resourceId('Microsoft.ContainerRegistry/registries/', parameters('acrName'))]" | ||
], | ||
"properties": { | ||
"zoneRedundancy": "enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"zoneRedundancy": "enabled" | |
"zoneRedundancy": "Enabled" |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Description
adds a ACREnableZoneRedundancy
Description
The function checks whether the "zoneRedundancy" property is set to True for a resource and all its replications. If any "zoneRedundancy" is not True, it returns a failure result; otherwise, it returns a pass result.
Fix
How does someone fix the issue in code and/or in runtime?
Checklist: