Check if file exists when executable path is passed in through jib dockerClient.executable #3744
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
…ockerClient.executable
mpeddada1
commented
Aug 25, 2022
| * | ||
| * @return {@code true} if Docker is installed on the user's system and accessible | ||
| */ | ||
| public static boolean isDefaultDockerInstalled() { | ||
| return isDockerInstalled(DEFAULT_DOCKER_CLIENT); | ||
| try { | ||
| new ProcessBuilder(DEFAULT_DOCKER_CLIENT.toString()).start(); |
There was a problem hiding this comment.
Some background of this change: Checking for existence of the default docker command when it is on PATH has been a little tricky. There can be several places where docker could be installed on a system and this also differs from OS to OS. For checking whether the path passed through jib.dockerClient.executable is installed correctly, however, we no longer run executable itself.
|
Kudos, SonarCloud Quality Gate passed! |
elefeint
approved these changes
Aug 25, 2022
| * | ||
| * @return {@code true} if Docker is installed on the user's system and accessible | ||
| */ | ||
| public static boolean isDefaultDockerInstalled() { | ||
| return isDockerInstalled(DEFAULT_DOCKER_CLIENT); | ||
| try { | ||
| new ProcessBuilder(DEFAULT_DOCKER_CLIENT.toString()).start(); |
This was referenced Aug 25, 2022
Closed
|
Thank you for the review @elefeint! Changes also succeeded on continuous build. The integration tests cover both scenarios: when default docker is installed and when docker executable is provided. Will be merging this PR. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, in order to verify if the docker path provided (either default or specified by the user) is installed correctly, we actually run the executable. The side effect of this is that any arbitrary executable can be executed by Jib.
This PR avoids the need to run the executable by using Files.isExecutable which verifies if the path provided exists and the JVM has the correct permissions to execute the path. However in the case of verifying whether docker is on PATH, running the
dockercommand seems unavoidable at the moment. Reasoning in #3744 (comment)Verified with
./gradlew jib-gradle-plugin:integrationTest