-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix #1684 #2234
Fix #1684 #2234
Conversation
install/detect-platform.sh
Outdated
@@ -12,8 +12,14 @@ echo "${_group}Detecting Docker platform" | |||
# linux/amd64 by default due to virtualization. | |||
# See https://github.com/docker/cli/issues/3286 for the Docker bug. | |||
|
|||
export DOCKER_EXISTS=$(command -v docker) | |||
export DOCKER_ARCH=$(docker info --format '{{.Architecture}}') |
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.
I would have moved this line after the check to avoid a silent failure
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.
Sorry, can you elaborate a bit more - which line number are you proposing to move the `export DOCKER_EXISTS..." line to? Or are you suggesting we move it to a different file?
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 line DOCKER_ARCH will silently fail if docker does not exist
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.
Ahhh, forgot that command substitutions get executed at decl time 🤦. Fixed.
install/detect-platform.sh
Outdated
export DOCKER_EXISTS=$(command -v docker) | ||
export DOCKER_ARCH=$(docker info --format '{{.Architecture}}') | ||
|
||
if ! "$DOCKER_EXISTS" &>/dev/null; then | ||
echo "FAIL: Could not find a \`docker\` binary on this system. Are you sure it's installed?" | ||
exit 1 | ||
fi | ||
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
✔️
Previous situation: we do most of our "check minimum requirements" work in the aptly named
check-minimum-requirements.sh
. This would be a natural home for the "check ifdocker
even exists on this system" verification, but we actually run call intodocker info
to set some environment variables before this script gets run, indetect-platforms.sh
. So I've adde the "doesdocker
exist" check to that file, directly before we calldocker info
.