[ACR] az acr check-health: Fix "DOCKER_PULL_ERROR" on Windows
#13158
Conversation
|
@yugangw-msft Could you help to review? |
Wwwsylvia
changed the title
az acr check-health: Fix docker pull bugaz acr check-health: Fix "DOCKER_PULL_ERROR" for Windows container
Wwwsylvia
changed the title
az acr check-health: Fix "DOCKER_PULL_ERROR" for Windows containeraz acr check-health: Fix "DOCKER_PULL_ERROR" for Windows containers
Wwwsylvia
changed the title
az acr check-health: Fix "DOCKER_PULL_ERROR" for Windows containersaz acr check-health: Fix "DOCKER_PULL_ERROR"
Wwwsylvia
changed the title
az acr check-health: Fix "DOCKER_PULL_ERROR"az acr check-health: Fix "DOCKER_PULL_ERROR" on Windows
|
add to S169 |
| if stderr: | ||
| _handle_error(DOCKER_PULL_ERROR.append_error_message(stderr), ignore_errors) | ||
| if DOCKER_PULL_WRONG_PLATFORM in stderr: | ||
| print_pass("Docker pull of '{}'".format(IMAGE)) |
There was a problem hiding this comment.
in cli we don't use print but logger, just checked this print_pass is used several places in this file. what scenario it's used for besides normal logger methods, could you pls change to logger.xxx?
There was a problem hiding this comment.
Other information like versions is also printed in stderr, how do users use the result of acr check-health command in a script?
There was a problem hiding this comment.
Thanks, I've replaced print() with logger.warning().
Other information like versions is also printed in stderr, how do users use the result of
acr check-healthcommand in a script?
az acr check-health is for troubleshooting purpose, usually users would not need the output of this command in script. An exit code is enough to check the status.
|
How about output error without modification? |
The original error was due to the limitation of mcr (the hello-world image does not support Windows), in this case the user's environment was still "healthy" as long as the image can be pulled. The idea of this PR is to dismiss this error and avoid confusing users.
We knew string matching is definitely not a good practice, but unfortunately it seems to be the only way to implement this, since currently Docker CLI does not return different exit codes to distinguish different errors types. Hope The Docker team can improve this someday. (I found someone raised this [Proposal] docker pull return code issue in the Docker CLI repo, but it's not resolved yet...) |
|
This PR is ready for review |
yungezz
left a comment
yungezz
left a comment
There was a problem hiding this comment.
LGTM. will approve after the color/style removed
| from colorama import Fore, Style, init | ||
| init() | ||
| print(str(message) + " : " + Fore.GREEN + "OK" + Style.RESET_ALL, file=sys.stderr) | ||
| logger.warning(str(message) + " : " + Fore.GREEN + "OK" + Style.RESET_ALL) |
There was a problem hiding this comment.
pls be aware of that warning will display to stderr. Some customers who're doing automation don't want warning in stderr to break auto pipeline
There was a problem hiding this comment.
and we don't suggest color in output, except it's from CLI underlying framework.
|
Can anyone help to merge if no more concerns? |
ghost
added
5 participants
Description
Fix az acr check-health : Error image operating system "linux" cannot be used on this platform
Fix the bug that
az acr check-healthalways throws "DOCKER_PULL_ERROR" on WindowsBefore:
After:
Testing Guide
az acr check-health --name myregistry --ignore-errors --yesHistory Notes
[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.
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.