Conversation
There was a problem hiding this comment.
Pull request overview
Adds managed-identity-based Azure CLI authentication when listing Azure Container Registry (ACR) tags, and bumps the package version accordingly.
Changes:
- Bump package version to
0.3.20. - Update changelog for
v0.3.20. - Add an
az login --identitystep tohelpers.list_acr_tags()before invokingaz acr repository show-tags.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pyproject.toml | Version bump to reflect the new release. |
| changelog.md | Notes the new managed identity login behavior for ACR tag listing. |
| cfa/cloudops/helpers.py | Attempts managed identity login prior to listing ACR repository tags via Azure CLI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sp.run( | ||
| ["az", "login", "--identity"], | ||
| capture_output=False, | ||
| stdout=sp.DEVNULL, | ||
| stderr=sp.DEVNULL, | ||
| ) |
There was a problem hiding this comment.
az login --identity is executed unconditionally and its result is ignored (return code and stderr are suppressed). If the managed identity login fails, the function will continue and later raise a generic “Could not list tags…” error, which hides the real authentication failure. Please capture the login result and either (a) fail fast with a clear error message when login fails, or (b) log the failure and only proceed when an existing session is already authenticated (e.g., via az account show).
| sp.run( | |
| ["az", "login", "--identity"], | |
| capture_output=False, | |
| stdout=sp.DEVNULL, | |
| stderr=sp.DEVNULL, | |
| ) | |
| login_result = sp.run( | |
| ["az", "login", "--identity"], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if login_result.returncode != 0: | |
| login_error = ( | |
| login_result.stderr.strip() if login_result.stderr else "Unknown error" | |
| ) | |
| logger.warning( | |
| "Managed identity login failed; checking for an existing Azure CLI session. " | |
| f"az login --identity error: {login_error}" | |
| ) | |
| account_show_result = sp.run( | |
| ["az", "account", "show", "--output", "json"], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if account_show_result.returncode != 0: | |
| raise Exception( | |
| "Azure CLI authentication failed: managed identity login was unsuccessful " | |
| f"and no existing authenticated session was found. az login --identity " | |
| f"error: {login_error}" | |
| ) | |
| logger.info( | |
| "Managed identity login failed, but an existing Azure CLI session is " | |
| "authenticated; proceeding with the existing session." | |
| ) |
| # login | ||
| sp.run( | ||
| ["az", "login", "--identity"], | ||
| capture_output=False, | ||
| stdout=sp.DEVNULL, | ||
| stderr=sp.DEVNULL, | ||
| ) |
There was a problem hiding this comment.
Calling az login inside list_acr_tags mutates the global Azure CLI session (writes to ~/.azure and may switch the active account), which can affect other Azure CLI calls in the same process and adds overhead for repeated tag listings. Consider making the login behavior optional/explicit (parameter or config), or avoid changing the CLI session by checking authentication first and only logging in when needed.
| # login | |
| sp.run( | |
| ["az", "login", "--identity"], | |
| capture_output=False, | |
| stdout=sp.DEVNULL, | |
| stderr=sp.DEVNULL, | |
| ) | |
| # Check whether Azure CLI is already authenticated before attempting login. | |
| auth_check = sp.run( | |
| ["az", "account", "show"], | |
| capture_output=True, | |
| text=True, | |
| stdout=sp.DEVNULL, | |
| stderr=sp.DEVNULL, | |
| ) | |
| if auth_check.returncode != 0: | |
| logger.info( | |
| "Azure CLI is not authenticated; attempting managed identity login" | |
| ) | |
| login_result = sp.run( | |
| ["az", "login", "--identity"], | |
| capture_output=True, | |
| text=True, | |
| stdout=sp.DEVNULL, | |
| stderr=sp.PIPE, | |
| ) | |
| if login_result.returncode != 0: | |
| error_msg = ( | |
| login_result.stderr.strip() | |
| if login_result.stderr | |
| else "Unknown error" | |
| ) | |
| raise Exception(f"Azure CLI login failed: {error_msg}") |
| sp.run( | ||
| ["az", "login", "--identity"], | ||
| capture_output=False, | ||
| stdout=sp.DEVNULL, | ||
| stderr=sp.DEVNULL, | ||
| ) |
There was a problem hiding this comment.
New behavior (managed identity login attempt + suppressed output) isn’t covered by tests. Since the repo has pytest coverage, please add unit tests that mock subprocess.run to verify the login command invocation/ordering and that login failures are surfaced with a helpful exception/message (or that fallback behavior is correct if you intentionally allow unauthenticated use).
| sp.run( | |
| ["az", "login", "--identity"], | |
| capture_output=False, | |
| stdout=sp.DEVNULL, | |
| stderr=sp.DEVNULL, | |
| ) | |
| login_command = ["az", "login", "--identity"] | |
| logger.debug("Executing managed identity login command before listing ACR tags") | |
| login_result = sp.run(login_command, capture_output=True, text=True) | |
| if login_result.returncode != 0: | |
| logger.error("Managed identity login failed before listing ACR tags") | |
| logger.error(f"Login stderr: {login_result.stderr}") | |
| error_msg = ( | |
| login_result.stderr.strip() | |
| if login_result.stderr | |
| else login_result.stdout.strip() | |
| if login_result.stdout | |
| else "Unknown error" | |
| ) | |
| raise Exception( | |
| "Azure managed identity login failed while preparing to list tags for " | |
| f"{registry_name}.azurecr.io/{repo_name}: {error_msg}" | |
| ) |
| --- | ||
| ## v0.3.20 | ||
|
|
||
| - add a managed identity login to list_acr_tags |
There was a problem hiding this comment.
Changelog entry uses lowercase/imperative wording (“add…”). For consistency/readability with the rest of the changelog, consider past tense/capitalization (e.g., “Added managed identity login to list_acr_tags." ).
| - add a managed identity login to list_acr_tags | |
| - Added managed identity login to `list_acr_tags` |
No description provided.