-
Notifications
You must be signed in to change notification settings - Fork 408
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
DockerClient.version() falls back to trying '--version' #213
base: master
Are you sure you want to change the base?
Conversation
…ing null This is added in an attempt to make this plugin compatible with podman, where '-v' does not work.
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 --version
option can be used with both docker and podman. Thus, the code can be simplified.
@@ -251,9 +251,12 @@ public void rm(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws I | |||
LaunchResult result = launch(new EnvVars(), true, "-v"); |
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.
Why just not use the --version
option? It is supported by both docker (see https://docs.docker.com/engine/reference/commandline/cli/) and podman.
LaunchResult result = launch(new EnvVars(), true, "-v"); | |
LaunchResult result = launch(new EnvVars(), true, "--version"); |
This way, there is no need to add another if-statement if the -v
option fails.
The |
Fully agree, I commited the change to only check for --version. |
ping @rsandell |
Hello.
As far as I can tell, only '-v' option is used to determine the docker version.
I added a fallback to check again for '--version', since I'm trying to make this run with podman instead of docker. Podman does not support the '-v' flag.
Best,
Renat