-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add podman buildx version support #16799
Conversation
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.
Let's wait for an answer to #16793 (comment). I have the feeling we need podman buildx version
and not a flag.
Fixes: containers#16793 Signed-off-by: Daniel J Walsh <[email protected]>
@Luap99 Switched to podman buildx version. |
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.
@flouthoc PTAL
I am not sure whether returning the Buildah version may confuse clients? The buildx is version is something different.
I thought about impersonating the buildx version, but that felt wrong, and not something we could even guarantee. |
I would have done the very same thing. @flouthoc has probably the most experience with buildkit, so I prefer to a head node from him. |
The problem is that a external tool is calling this command which likely means that it tires to parse the output, so having non matching output will just defer the error one step later. |
Hmm upon checking sudo docker buildx version
github.com/docker/buildx v0.8.0-71-g6854eec4 6854eec48de2fa05fadbce978dd9d7de848e82fb
[fl@fedora secretdir3]$ sudo docker buildx --version
unknown flag: --version
See 'docker buildx --help'.
Usage: docker buildx [OPTIONS] COMMAND
Extended build capabilities with BuildKit
Options:
--builder string Override the configured builder instance
Management Commands:
imagetools Commands to work on images in registry
Commands:
bake Build from a file
build Start a build
create Create a new builder instance
du Disk usage
inspect Inspect current builder instance
ls List builder instances
prune Remove build cache
rm Remove a builder instance
stop Stop builder instance
use Set the current builder instance
version Show buildx version information
Run 'docker buildx COMMAND --help' for more information on a command. I think we should also support |
That's what the PR does. I just wonder whether the output is useful. I share Paul's comment on deferring the error to a later point. Maybe it's better to explicitly error out? Some clients may misinterpret the output and run into hard to debug failures after? |
@albinkc could you check to see if this would fix your problem or just confuse the tool? IE is the tool using this information for display purposes or actually making assumptions on the version returned. |
@vrothberg @Luap99 anything depending upon the output of This analogy is also true for |
I agree I think we just merge this and then allow the users to deal with the differences in versions. All we can do is match the CLI not necessarily the CLI output. |
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.
LGTM
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.
LGTM
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes: #16793
Signed-off-by: Daniel J Walsh [email protected]
Does this PR introduce a user-facing change?