-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[ACR] az acr login: Support Podman for --name parameter
#31850
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
base: dev
Are you sure you want to change the base?
Conversation
️✔️AzureCLI-FullTest
|
|
Hi @cpressland, |
️✔️AzureCLI-BreakingChangeTest
|
|
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>
|
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.
Pull Request Overview
This PR adds Podman support to the az acr login command, allowing users to authenticate with Azure Container Registry without requiring Docker to be installed. The change implements automatic detection of Podman as an alternative container runtime when Docker is not available.
Key changes:
- Adds automatic Podman detection when Docker is not explicitly configured
- Updates Windows executable fallback logic to work with both Docker and Podman
- Imports
shutilmodule for executable path detection
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
az acr login support for Podmanaz acr login: Support Podman for --name parameter
|
@northtyphoon Could you please take a look at ACR related PR? |
|
@lizMSFT can you take a look? Basically what this PR does it performs a |
| docker_command = os.getenv('DOCKER_COMMAND') | ||
| else: | ||
| docker_command = 'docker' | ||
| if not shutil.which('docker') and shutil.which('podman'): |
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.
This method is also used by _get_docker_status_and_version which outputs hardcoded "Docker" messages (e.g., "Docker version", "Docker daemon status"). When Podman is used instead, these messages will be inaccurate. Please update check_healthy.py as well to dynamically reflect which tool is being checked, or use tool-agnostic terminology like "Container runtime".
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.
@lizMSFT this has been updated, however, docker version and podman version behave quite differently so I ended up parsing the --format json output instead of trying to reconcile the differences in the two commands. Let me know if you're happy with this.
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except ValueError: | ||
| os = "unknown" | ||
| arch = "unknown" | ||
| logger.warning("%s version: %s, build %s, platform %s/%s", docker_command.title(), version, commit, os, arch) |
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.
Should this logger.warning("%s version: %s, build %s, platform %s/%s", docker_command.title(), version, commit, os, arch) statement be indented inside the else block?
Right now it can raise a NameError if version, commit, os, or arch aren’t defined.
| def get_docker_command(is_diagnostics_context=False): | ||
| from ._errors import DOCKER_COMMAND_ERROR, DOCKER_DAEMON_ERROR | ||
| if os.getenv('DOCKER_COMMAND'): | ||
| docker_command = os.getenv('DOCKER_COMMAND') |
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.
The function name is now misleading because the logic supports Podman as well. Consider renaming it to avoid confusion for future maintainers.
Related command
az acr loginDescription
Adds a simple check for
podmanbeing inPATHand sets that as thedocker_commandif found. This enables users to runaz acr loginwithout Docker being installed or setting the DOCKER_COMMAND environment variable.Testing Guide
Execute
az acr login+ arguments to a valid ACR instance when you have podman installed, instead of getting an error, it should pass:Before:
After:
History Notes
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.