Conversation
added 5 commits
September 16, 2026 15:42
Transplant the authentication discriminator, ManagedIdentity target guards, SyncToken-only help, and focused source unit tests. Source PR: Azure#33910 Source SHA: e0b7694
Transplant CREATE options, inline authentication branches, identity shaping, request payload, example, and source validation tests. Retain cmd.get_models and legacy SyncToken validation. Require a nonblank identity and reject explicitly supplied incompatible options. Enforce top-level MI and immediate-parent leaf guards before mutations; defer data-endpoint enablement until parent validation. Cover the guards and real SDK payload/token paths. Source PR: Azure#33910 Source SHA: e0b7694
Reuse the UPDATE arguments, inline input handling and PATCH shaping from Azure#33910, source e0b7694, with the existing SDK model helper. Keep ManagedIdentity as the only migration target. Require explicit auth type and a nonblank identity, while deferring current-auth and Offline eligibility checks to the RP. Correct the migration example and cover empty inputs, RP-error propagation, migration combined with ordinary property changes, and preservation of auth during ordinary updates.
Transfer GET-SETTINGS helpers, ManagedIdentity early return, and the shared SyncToken settings tail from the pinned original implementation. Source: Azure#33910 Source-commit: e0b7694 Preserve source response guards, request-name selection, endpoint/protocol resolution, output keys, errors, and MI --generate-password rejection. Keep accepted CREATE, permissions, UPDATE, fixtures, and tests unchanged. Deviations/integration: - Add a handler-level too-many-locals pylint annotation (26 vs limit 25) rather than restructuring the original source. - Reuse existing test fixtures/imports; copy the four MI tests and helper, adding one exact-output/no-token-or-credential-call test and one six-case SyncToken regression test. Update the test module description/import. - Clarify optional SyncToken generation and MI rejection in get-settings help/argument help, plus the deprecated renew-credentials restriction. Preserve examples and all parameter defaults/requirements. The source connection-string format is not independently verified against inaccessible specification section 3.3. Request-name vs GET-response-name and endpoint contract questions remain separately reported, not fixed. Validation: 34 mock-only unit tests pass; six SyncToken cases also pass against the accepted pre-transfer handler. Scoped flake8/pylint, help and command/parameter lint, in-memory compilation, and source-fidelity checks pass. No live Azure or recording validation performed.
|
Hi nihalvar, |
microsoft-github-policy-service
Bot
requested review from
Bin Du (northtyphoon) and
Terence Truong (terencet-dev)
September 21, 2026 00:46
Collaborator
|
ACR |
az acr connected-registry: add support for managed identity optionsaz acr connected-registry: Add support for managed identity options
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A unit test has deterministic failing assertions, and the migration help text needs correction.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Adds user-assigned managed identity support for ACR connected registries, including creation, migration, settings, and cleanup behavior.
Changes:
- Adds
--auth-typeand--identityoptions. - Supports SyncToken-to-managed-identity migration.
- Updates MI-aware settings, permissions, deletion, and documentation.
- Adds unit, scenario, and recording coverage.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_connectedregistry_commands.py |
Adds managed identity scenario coverage. |
src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_connected_registry_mi_unit.py |
Adds MI-focused unit tests; contains assertions that incorrectly reject required lookups. |
src/azure-cli/azure/cli/command_modules/acr/tests/latest/recordings/test_acr_connectedregistry_managed_identity.yaml |
Records managed identity scenario interactions. |
src/azure-cli/azure/cli/command_modules/acr/connected_registry.py |
Implements managed identity workflows. |
src/azure-cli/azure/cli/command_modules/acr/_params.py |
Adds managed identity CLI arguments. |
src/azure-cli/azure/cli/command_modules/acr/_help.py |
Documents MI workflows; deactivation guidance should account for Offline/Inactive registries. |
src/azure-cli/azure/cli/command_modules/acr/_constants.py |
Defines authentication types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Clarify migration prerequisites for connected registry. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Zoey Li (lizMSFT)
left a comment
Member
There was a problem hiding this comment.
Left a few nit comments
Zoey Li (lizMSFT)
approved these changes
Sep 24, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 PR Validation —⚠️ Review suggested
Related command
az acr connected-registry create,update,get-settings,delete, andpermissions show/update.Description
Adds user-assigned managed identity authentication as an alternative to sync tokens for connected registries.
Referenced PR: #33910
--auth-typeand--identityto create connected registries using an existing user-assigned identity, without creating sync tokens or scope maps.update. The service validates migration eligibility, including the required Offline state. Unrelated connected-registry settings are preserved.ACR_REGISTRY_CONNECTION_STRING, without sync-token credentials. Password generation and sync-token permission operations are rejected for MI registries.--cleanup. Separately owned identities and role assignments are not deleted.authType, and updates argument help and examples.The CLI does not automatically create the managed identity or assign its roles. No intentional breaking changes.
Testing Guide
Adds unit coverage in
test_acr_connected_registry_mi_unit.pyfor validation, identity payloads, migration updates, service-error propagation, connection settings, and deletion with both cleanup values. Existing SyncToken settings and cleanup behavior have regression coverage.Two recorded scenarios are included:
Both scenarios use Premium, ABAC-enabled registries with dedicated data endpoints. Their recordings are included in
tests/latest/recordings.These scenarios cover control-plane behavior without deploying an agent. The migration starts with a newly created Offline/Inactive registry; no Online-to-Offline transition is required. Runtime MI authentication and image synchronization are outside this coverage.
az acr connected-registry createaz acr connected-registry updateaz acr connected-registry get-settingsaz acr connected-registry deleteHistory Notes
[ACR]
az acr connected-registry create: Add user-assigned managed identity authentication options[ACR]
az acr connected-registry update: Add one-way migration from SyncToken to managed identity authentication[ACR]
az acr connected-registry get-settings: Return managed identity connection settings[ACR]
az acr connected-registry delete: Skip sync-token cleanup for managed identity authenticationThis 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.