Skip to content

Commit e0b7694

Browse files
Dhawal JainDhawal Jain
authored andcommitted
fix(acr): address review feedback on connected-registry MI support
- Simplify _get_current_auth_type to use authType as the sole source of truth (drop identity-based inference and try/except guard; matches pre-existing direct-access style). - get-settings (MI mode): emit only the spec-required connection string; drop the extra ACR_MANAGED_IDENTITY_CLIENT_ID / ACR_MANAGED_IDENTITY_RESOURCE_ID env vars. - Coerce connection_state via .value before comparing to CONNECTION_STATE_OFFLINE (defensive against future SDK regen; symmetry with auth_type). - Help/params/constants tidy-ups: typo fixes (midnight, its, immediate), MI parameter help, SyncToken caveats on permissions show/update. - Tests: update _fake_cr helper so has_identity=True implies MI auth_type; rework TestGetCurrentAuthType; add plain-Enum coercion regression guard for connection_state; tighten MI get-settings assertions.
1 parent 7f5ad04 commit e0b7694

5 files changed

Lines changed: 119 additions & 101 deletions

File tree

src/azure-cli/azure/cli/command_modules/acr/_constants.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515

1616
USER_ASSIGNED_IDENTITY_RESOURCE_ID_TEMPLATE = '/subscriptions/{sub_id}/resourceGroups/{rg}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identity_name}'
1717

18-
CONNECTED_REGISTRY_AUTH_TYPES = ['SyncToken', 'ManagedIdentity']
18+
class ConnectedRegistryAuthType(Enum):
19+
SYNC_TOKEN = 'SyncToken'
20+
MANAGED_IDENTITY = 'ManagedIdentity'
1921

2022
TASK_RESOURCE_TYPE = REGISTRY_RESOURCE_TYPE + '/tasks'
2123
TASK_VALID_VSTS_URLS = ['visualstudio.com', 'dev.azure.com']

src/azure-cli/azure/cli/command_modules/acr/_help.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1650,7 +1650,7 @@
16501650
text: |
16511651
az acr connected-registry create --registry mycloudregistry --name myreadonlyacr \\
16521652
--mode readonly --parent myconnectedregistry --sync-token mySyncTokenName
1653-
- name: Create a read only connected registry with client tokens, that syncs every day at midninght and sync window of 4 hours.
1653+
- name: Create a read only connected registry with client tokens, that syncs every day at midnight and sync window of 4 hours.
16541654
text: |
16551655
az acr connected-registry create -r mycloudregistry -n myreadonlyacr -p myconnectedregistry \\
16561656
--repository "app/mycomponent" -m ReadOnly -s "0 12 * * *" -w PT4H \\
@@ -1659,7 +1659,7 @@
16591659
text: |
16601660
az acr connected-registry create --registry mycloudregistry --name myconnectedregistry \\
16611661
--auth-type ManagedIdentity \\
1662-
--identity /subscriptions/<sub>/resourceGroups/<rg>/providers/Microsoft.ManagedIdentity/userAssignedIdentities/<msi>
1662+
--identity "/subscriptions/<SUBSCRIPTON ID>/resourcegroups/<RESOURCE GROUP>/providers/Microsoft.ManagedIdentity/userAssignedIdentities/myUserAssignedIdentitiy"
16631663
"""
16641664

16651665
helps['acr connected-registry delete'] = """
@@ -1669,7 +1669,7 @@
16691669
- name: Delete a read only connected registry 'myconnectedregistry' from parent registry 'mycloudregistry'.
16701670
text: >
16711671
az acr connected-registry delete --registry mycloudregistry --name myconnectedregistry
1672-
- name: Delete a read only connected registry 'myconnectedregistry' and it's sync token and scope-map from parent registry 'mycloudregistry'.
1672+
- name: Delete a read only connected registry 'myconnectedregistry' and its sync token and scope-map from parent registry 'mycloudregistry'.
16731673
text: >
16741674
az acr connected-registry delete -r mycloudregistry -n myconnectedregistry --cleanup
16751675
"""
@@ -1690,7 +1690,7 @@
16901690
- name: List all the connected registries of 'mycloudregistry' in table format.
16911691
text: >
16921692
az acr connected-registry list --registry mycloudregistry --output table
1693-
- name: List only the inmediate children of 'mycloudregistry' in expanded form in a table.
1693+
- name: List only the immediate children of 'mycloudregistry' in expanded form in a table.
16941694
text: >
16951695
az acr connected-registry list --registry mycloudregistry --no-children --output table
16961696
- name: List all the offspring of 'myconnectedregistry' in expanded form inside a table.
@@ -1737,7 +1737,7 @@
17371737
text: |
17381738
az acr connected-registry update --registry mycloudregistry --name myconnectedregistry \\
17391739
--auth-type ManagedIdentity \\
1740-
--identity /subscriptions/<sub>/resourceGroups/<rg>/providers/Microsoft.ManagedIdentity/userAssignedIdentities/<msi>
1740+
--identity "/subscriptions/<SUBSCRIPTON ID>/resourcegroups/<RESOURCE GROUP>/providers/Microsoft.ManagedIdentity/userAssignedIdentities/myUserAssignedIdentitiy"
17411741
"""
17421742

17431743
helps['acr connected-registry get-settings'] = """
@@ -1754,12 +1754,12 @@
17541754

17551755
helps['acr connected-registry permissions'] = """
17561756
type: group
1757-
short-summary: Manage the repository permissions accross multiple connected registries. Only supported for connected registries configured with SyncToken authentication (output is derived from the sync-token scope map). Please see https://aka.ms/acr/connected-registry for more information.
1757+
short-summary: Manage the repository permissions across multiple connected registries. Only supported for connected registries configured with SyncToken authentication (output is derived from the sync-token scope map). Please see https://aka.ms/acr/connected-registry for more information.
17581758
"""
17591759

17601760
helps['acr connected-registry permissions update'] = """
17611761
type: command
1762-
short-summary: Add and remove repository permissions accross all the necessary connected registry sync scope maps.
1762+
short-summary: Add and remove repository permissions across all the necessary connected registry sync scope maps. Only supported for connected registries configured with SyncToken authentication.
17631763
examples:
17641764
- name: Add permissions to synchronize images from 'repo1' and 'repo2' to the connected registry 'myconnectedregistry' and its ancestors.
17651765
text: >
@@ -1774,7 +1774,7 @@
17741774

17751775
helps['acr connected-registry permissions show'] = """
17761776
type: command
1777-
short-summary: Show the connected registry sync scope map information.
1777+
short-summary: Show the connected registry sync scope map information. Only supported for connected registries configured with SyncToken authentication.
17781778
examples:
17791779
- name: Show details and attributes of a sync scope map for a connected registry.
17801780
text: >

src/azure-cli/azure/cli/command_modules/acr/_params.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525

2626
from ._constants import (
2727
AbacRoleAssignmentMode,
28-
CONNECTED_REGISTRY_AUTH_TYPES,
28+
ConnectedRegistryAuthType,
2929
REGISTRY_RESOURCE_TYPE,
3030
WEBHOOK_RESOURCE_TYPE,
3131
REPLICATION_RESOURCE_TYPE,
@@ -585,7 +585,7 @@ def load_arguments(self, _): # pylint: disable=too-many-statements
585585
c.argument('garbage_collection_schedule', options_list=['--gc-schedule'],
586586
help='Used to determine garbage collection schedule. Uses cron expression to determine the schedule. If not specified, garbage collection is set to run once a day.', required=False, default="0 0 * * *")
587587
c.argument('identity', help='Resource ID of a user-assigned managed identity to authenticate the connected registry with its parent. Required when --auth-type is ManagedIdentity.')
588-
c.argument('auth_type', arg_type=get_enum_type(CONNECTED_REGISTRY_AUTH_TYPES), options_list=['--auth-type'], help='Authentication type used by the connected registry to sync with its parent. Defaults to SyncToken.')
588+
c.argument('auth_type', arg_type=get_enum_type([e.value for e in ConnectedRegistryAuthType]), options_list=['--auth-type'], help='Authentication type used by the connected registry to sync with its parent. Defaults to SyncToken.')
589589

590590
with self.argument_context('acr connected-registry update') as c:
591591
c.argument('log_level', help='Set the log level for logging on the instance. Accepted log levels are Debug, Information, Warning, Error, and None.')
@@ -604,7 +604,7 @@ def load_arguments(self, _): # pylint: disable=too-many-statements
604604
help='Indicate whether garbage collection is enabled. It is enabled by default.', arg_type=get_three_state_flag())
605605
c.argument('garbage_collection_schedule', options_list=['--gc-schedule'], help='Used to determine garbage collection schedule. Uses cron expression to determine the schedule. If not specified, garbage collection is set to run once a day.')
606606
c.argument('identity', help='Resource ID of a user-assigned managed identity. Required when migrating --auth-type to ManagedIdentity.')
607-
c.argument('auth_type', arg_type=get_enum_type(CONNECTED_REGISTRY_AUTH_TYPES), options_list=['--auth-type'], help='Target authentication type. Only one-way migration from SyncToken to ManagedIdentity is supported. Connected registry must be in Offline state.')
607+
c.argument('auth_type', arg_type=get_enum_type([ConnectedRegistryAuthType.MANAGED_IDENTITY.value]), options_list=['--auth-type'], help='Target authentication type. Only one-way migration from SyncToken to ManagedIdentity is supported. Connected registry must be in Offline state.')
608608
with self.argument_context('acr connected-registry permissions') as c:
609609
c.argument('add_repos', options_list=['--add'], nargs='*',
610610
help='repository permissions to be added to the targeted connected registry and it\'s ancestors sync scope maps. Use the format "--add [REPO1 REPO2 ...]" per flag. ' + repo_valid_actions)

0 commit comments

Comments
 (0)