Skip to content

[ACR] az acr login: Support Podman for --name parameter #33115

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.

Sign up for GitHub

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

Open
lizMSFT wants to merge 1 commit into
base: dev
Choose a base branch
from
+27 −10
Open

[ACR] az acr login: Support Podman for --name parameter #33115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions src/azure-cli/azure/cli/command_modules/acr/check_health.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# --------------------------------------------------------------------------------------------

import re
import json
from knack.util import CLIError
from knack.log import get_logger
from .custom import get_docker_command
Expand Down Expand Up @@ -73,11 +74,11 @@ def _subprocess_communicate(command_parts, shell=False):


# Checks for the environment
# Checks docker command, docker daemon, docker version and docker pull
# Checks container command, daemon, version and image pull
def _get_docker_status_and_version(ignore_errors, yes):
from ._errors import DOCKER_DAEMON_ERROR, DOCKER_PULL_ERROR, DOCKER_VERSION_ERROR

# Docker command and docker daemon check
# Container command and daemon check
docker_command, error = get_docker_command(is_diagnostics_context=True)
docker_daemon_available = True

Expand All @@ -88,18 +89,31 @@ def _get_docker_status_and_version(ignore_errors, yes):
docker_daemon_available = False

if docker_daemon_available:
logger.warning("Docker daemon status: available")
logger.warning("%s daemon status: available", docker_command.title())
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using docker_command.title() can produce odd display names (e.g., docker.exe -> Docker.Exe, or titles for full paths if DOCKER_COMMAND is a path). Consider deriving a stable display label from the executable basename (and stripping .exe), or explicitly mapping known values (docker/podman) to user-facing names.

Copilot uses AI. Check for mistakes.

# Docker version check
output, warning, stderr, succeeded = _subprocess_communicate(
[docker_command, "version", "--format", "'Docker version {{.Server.Version}}, "
"build {{.Server.GitCommit}}, platform {{.Server.Os}}/{{.Server.Arch}}'"])
output, warning, stderr, succeeded = _subprocess_communicate([docker_command, "version", "--format", "json"])
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Docker, docker version --format expects a Go template (e.g., {{json .}}), not the literal string json. As written, Docker will not output JSON, and the subsequent json.loads(output) will fall back to unknown fields, degrading diagnostics. A concrete fix is to use a Docker-compatible --format template for Docker (and keep --format json for Podman if needed), or switch to a Docker/PODMAN invocation that reliably outputs JSON across both CLIs.

Suggested change
output, warning, stderr, succeeded = _subprocess_communicate([docker_command, "version", "--format", "json"])
format_arg = "{{json .}}" if docker_command == "docker" else "json"
output, warning, stderr, succeeded = _subprocess_communicate(
[docker_command, "version", "--format", format_arg]
)

Copilot uses AI. Check for mistakes.
if not succeeded:
_handle_error(DOCKER_VERSION_ERROR.append_error_message(stderr), ignore_errors)
else:
if warning:
logger.warning(warning)
logger.warning("Docker version: %s", output)
try:
json_output = json.loads(output).get("Client")
except json.decoder.JSONDecodeError:
Comment on lines +102 to +103
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the diagnostic from reporting the daemon/server version (previously {{.Server.Version}}) to reporting the client version (Client.Version). That’s a behavior regression for a health check that’s validating the daemon. If the intent is to keep reporting daemon version, prefer parsing Server when present/when the daemon is available (and fall back to Client only when server info is absent).

Suggested change
json_output = json.loads(output).get("Client")
except json.decoder.JSONDecodeError:
docker_info = json.loads(output)
except json.decoder.JSONDecodeError:
docker_info = {}
if isinstance(docker_info, dict):
json_output = docker_info.get("Server") or docker_info.get("Client") or {}
else:

Copilot uses AI. Check for mistakes.
json_output = {}
version = json_output.get("Version", "unknown")
commit = json_output.get("GitCommit", "unknown")[:7]
if docker_command == "docker":
os = json_output.get("Os", "unknown")
arch = json_output.get("Arch", "unknown")
else:
try:
os, arch = json_output.get("OsArch", "unknown").split("/")
except ValueError:
os = "unknown"
arch = "unknown"
logger.warning("%s version: %s, build %s, platform %s/%s", docker_command.title(), version, commit, os, arch)
Comment on lines +108 to +116
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using os as a local variable name is ambiguous in Python because os commonly refers to the standard library module. Renaming it to something like platform_os (and possibly platform_arch) would make the intent clearer and avoid confusion in future edits.

Suggested change
os = json_output.get("Os", "unknown")
arch = json_output.get("Arch", "unknown")
else:
try:
os, arch = json_output.get("OsArch", "unknown").split("/")
except ValueError:
os = "unknown"
arch = "unknown"
logger.warning("%s version: %s, build %s, platform %s/%s", docker_command.title(), version, commit, os, arch)
platform_os = json_output.get("Os", "unknown")
platform_arch = json_output.get("Arch", "unknown")
else:
try:
platform_os, platform_arch = json_output.get("OsArch", "unknown").split("/")
except ValueError:
platform_os = "unknown"
platform_arch = "unknown"
logger.warning("%s version: %s, build %s, platform %s/%s", docker_command.title(), version, commit, platform_os, platform_arch)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using docker_command.title() can produce odd display names (e.g., docker.exe -> Docker.Exe, or titles for full paths if DOCKER_COMMAND is a path). Consider deriving a stable display label from the executable basename (and stripping .exe), or explicitly mapping known values (docker/podman) to user-facing names.

Copilot uses AI. Check for mistakes.

# Docker pull check - only if docker daemon is available
if docker_daemon_available:
Expand All @@ -114,14 +128,14 @@ def _get_docker_status_and_version(ignore_errors, yes):

if not succeeded:
if stderr and DOCKER_PULL_WRONG_PLATFORM in stderr:
print_pass("Docker pull of '{}'".format(IMAGE))
print_pass(f"{docker_command.title()} pull of '{IMAGE}'")
logger.warning("Image '%s' can be pulled but cannot be used on this platform", IMAGE)
return
_handle_error(DOCKER_PULL_ERROR.append_error_message(stderr), ignore_errors)
else:
if warning:
logger.warning(warning)
print_pass("Docker pull of '{}'".format(IMAGE))
print_pass(f"{docker_command.title()} pull of '{IMAGE}'")


# Get current CLI version
Expand Down
5 changes: 4 additions & 1 deletion src/azure-cli/azure/cli/command_modules/acr/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import os
import re
import shutil
from knack.util import CLIError
from knack.log import get_logger
from azure.cli.core.azclierror import InvalidArgumentValueError, RequiredArgumentMissingError
Expand Down Expand Up @@ -396,6 +397,8 @@ def get_docker_command(is_diagnostics_context=False):
docker_command = os.getenv('DOCKER_COMMAND')
else:
docker_command = 'docker'
if not shutil.which('docker') and shutil.which('podman'):
docker_command = 'podman'

from subprocess import PIPE, Popen, CalledProcessError
try:
Expand All @@ -405,7 +408,7 @@ def get_docker_command(is_diagnostics_context=False):
logger.debug("Could not run '%s' command. Exception: %s", docker_command, str(e))
# The executable may not be discoverable in WSL so retry *.exe once
try:
docker_command = 'docker.exe'
docker_command = f'{docker_command}.exe'
p = Popen([docker_command, "ps"], stdout=PIPE, stderr=PIPE)
Comment on lines +411 to 412
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appending .exe unconditionally to docker_command can generate invalid commands (e.g., podman.exe in non-Windows contexts, or ...docker.exe.exe when DOCKER_COMMAND already includes an .exe or a path). Consider only applying the *.exe retry when the command is the default docker (or when the basename lacks an extension), and avoid double-appending by checking docker_command.lower().endswith('.exe') / using os.path.splitext on the basename.

Copilot uses AI. Check for mistakes.
_, stderr = p.communicate()
except OSError as inner:
Expand Down
Loading