{ACR} Update ACR test suite: remove unnecessary @live_only decorators and make time.sleep conditional on live mode#33135
Conversation
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
️✔️AzureCLI-FullTest
|
CodeJourneys
changed the title
️✔️AzureCLI-BreakingChangeTest
|
microsoft-github-policy-service
bot
requested review from
northtyphoon and
terencet-dev
CodeJourneys
changed the title
There was a problem hiding this comment.
Pull request overview
This PR primarily refactors the ACR command module test suite to enable more VCR playback coverage and reduce unnecessary delays, alongside updated VCR recordings. It also includes a production-code change in acr/check_health.py affecting CMK validation logic.
Changes:
- Removed or adjusted
@live_only()usage so more ACR scenario tests can run in VCR playback. - Made
time.sleep()calls conditional onself.is_liveto avoid slowing down playback runs; fixed command ordering in a zone-redundancy test. - Updated multiple ACR test recordings to match the new execution paths and CLI/SDK versions.
Reviewed changes
Copilot reviewed 18 out of 42 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_task_commands.py | Removes live_only usage for task tests; import list changed. |
| src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_connectedregistry_commands.py | Removes live_only usage for a connected registry test; import list changed. |
| src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_commands.py | Keeps live_only where required; gates sleeps on live runs; reorders a replication check. |
| src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_agentpool_commands.py | Cleans up and simplifies imports for agentpool tests. |
| src/azure-cli/azure/cli/command_modules/acr/tests/latest/recordings/*.yaml | Refreshes VCR recordings to align with updated tests and CLI/SDK behavior. |
| src/azure-cli/azure/cli/command_modules/acr/check_health.py | Removes API-version gating around CMK validation (runtime behavior change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_task_commands.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_connectedregistry_commands.py
Outdated
Show resolved
Hide resolved
| if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks | ||
| client_id = registry.encryption.key_vault_properties.identity |
There was a problem hiding this comment.
Removing the cmd.supported_api_version(... MGMT_CONTAINERREGISTRY ...) guard changes runtime behavior and can cause an AttributeError on clouds/profiles using older Container Registry API versions where the returned Registry model may not have an encryption attribute. Please either restore the API-version gate (as before) or switch these accesses to getattr(registry, 'encryption', None) / getattr(..., 'key_vault_properties', None) so az acr check-health remains compatible across profiles.
| if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks | |
| client_id = registry.encryption.key_vault_properties.identity | |
| encryption = getattr(registry, 'encryption', None) if registry else None | |
| key_vault_properties = getattr(encryption, 'key_vault_properties', None) | |
| if key_vault_properties: # pylint: disable=too-many-nested-blocks | |
| client_id = key_vault_properties.identity |
| # CMK settings | ||
| if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks |
There was a problem hiding this comment.
This change also makes from azure.cli.core.profiles import ResourceType (near the top of _check_registry_health) unused, since ResourceType was only referenced by the removed cmd.supported_api_version(...) call. Please remove that import (or reintroduce the version check) to avoid unused-import lint failures.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| # CMK settings | ||
| if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks | ||
| client_id = registry.encryption.key_vault_properties.identity | ||
| valid_identity = False | ||
| if registry.identity: | ||
| valid_identity = ((client_id == 'system') and | ||
| bool(registry.identity.principal_id)) # use system identity? | ||
| if not valid_identity and registry.identity.user_assigned_identities: | ||
| for k, v in registry.identity.user_assigned_identities.items(): | ||
| if v.client_id == client_id: | ||
| from azure.core.exceptions import HttpResponseError | ||
| try: | ||
| valid_identity = resolve_identity_client_id(cmd.cli_ctx, k) == client_id | ||
| except HttpResponseError: | ||
| pass | ||
| if not valid_identity: | ||
| from ._errors import CMK_MANAGED_IDENTITY_ERROR | ||
| _handle_error(CMK_MANAGED_IDENTITY_ERROR.format_error_message(registry_name), ignore_errors) |
There was a problem hiding this comment.
The PR title/description focus on ACR test refactoring, but this hunk changes production behavior in acr/check_health.py (CMK validation now runs unconditionally and the API-version gate is removed). Please update the PR description to explicitly call out this functional change (or move it into a separate PR) so reviewers can assess the runtime impact appropriately.
There was a problem hiding this comment.
That modification should be addressed by #33117
Related command
az acr (various test scenarios)
Description
Test refactoring to the ACR command test suite:
@live_only()decorators fromtest_acr_create_with_managed_registryandtest_acr_connectedregistry_dedicated_endpoint_not_enabledso those tests can run in VCR playback.@live_only()with explanatory comments where confirmed requiredtime.sleep()calls intest_acr_encryption_with_cmkconditional onself.is_liveto avoid unnecessary delays during playback.test_acr_with_zone_redundancy: movedacr replication showon the home region after replications are created, so the check matches current API source.test_acr_agentpool_commands.pyandtest_acr_connectedregistry_commands.py.This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.