Fix #1684 #2234
Merged
Fix #1684 #2234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
williamdes
reviewed
Jun 26, 2023
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.
I would have moved this line after the check to avoid a silent failure
There was a problem hiding this comment.
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.
The line DOCKER_ARCH will silently fail if docker does not exist
There was a problem hiding this comment.
Ahhh, forgot that command substitutions get executed at decl time 🤦. Fixed.
williamdes
reviewed
Jun 26, 2023
install/detect-platform.sh
Outdated
Comment on lines
15
to
22
| 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.
hubertdeng123
approved these changes
Jun 26, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ifdockereven exists on this system" verification, but we actually run call intodocker infoto set some environment variables before this script gets run, indetect-platforms.sh. So I've adde the "doesdockerexist" check to that file, directly before we calldocker info.