Skip to content
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

Alias build to buildx, so it won't fail #11134

Merged
merged 1 commit into from
Aug 9, 2021
Merged

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Aug 4, 2021

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Aug 4, 2021

Partially fixes #11130

FROM $IMAGE
RUN echo $rand_content > /$rand_filename
EOF

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be a run_podman buildx ... here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little heavy on the cut and pasting.

@baude
Copy link
Member

baude commented Aug 4, 2021

lgtm

@edsantiago
Copy link
Member

My only concern, and I say this as someone who has no clue what buildx is, is: what will happen when podman buildx gets implemented as its own command? What incompatibilities are there between build & buildx, that we will pay for because someone has written a script that does podman buildx --foo --bar, relying on --foo --bar from current podman build, that will break because those options don't exist or do something different in real-buildx?

@rhatdan
Copy link
Member Author

rhatdan commented Aug 4, 2021

We don't plan on adding buildx, only some of the options. But understand your point that their could be an option for
podman build --foo
which is different then
podman buildx --foo.

@@ -44,6 +44,7 @@ var (
// Command: podman _diff_ Object_ID
buildDescription = "Builds an OCI or Docker image using instructions from one or more Containerfiles and a specified build context directory."
buildCmd = &cobra.Command{
Aliases: []string{"buildx"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the alias should be buildx build (buildx has other subcommands) so when building an image with docker, one would call docker buildx build ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it has to be podman buildx build, other buildx subcommands should raise an error, see https://docs.docker.com/buildx/working-with-buildx/.

@@ -43,6 +43,8 @@ containers can be left in container storage. Use the `podman ps --all --storage`
command to see these containers. External containers can be removed with the
`podman rm --storage` command.

`podman buildx` command is an alias of `podman build`. Not all `buildx` features are available in Podman. The `buildx` option is provided for scripting compatibility.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`podman buildx` command is an alias of `podman build`. Not all `buildx` features are available in Podman. The `buildx` option is provided for scripting compatibility.
The `podman buildx` command is an alias of `podman build`. Not all `buildx` features are available in Podman. The `buildx` option is provided strictly for scripting compatibility.

@rhatdan
Copy link
Member Author

rhatdan commented Aug 6, 2021

@Luap99 could you take a look at the failures, seems to be some command completion problem?

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is broken, this diff should make it work:

diff --git a/cmd/podman/shell_completion_test.go b/cmd/podman/shell_completion_test.go
index 9bd821d8d..792beeb19 100644
--- a/cmd/podman/shell_completion_test.go
+++ b/cmd/podman/shell_completion_test.go
@@ -33,7 +33,9 @@ func TestShellCompletionFunctions(t *testing.T) {
 func checkCommand(t *testing.T, cmd *cobra.Command) {
        if cmd.HasSubCommands() {
                for _, childCmd := range cmd.Commands() {
-                       checkCommand(t, childCmd)
+                       if !childCmd.Hidden {
+                               checkCommand(t, childCmd)
+                       }
                }
 
                // if not check if completion for that command is provided

var (
// Command: podman _buildx_
// This is a hidden command, which was added to make converting
// from Docekr to Podman easier.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo Docker

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luap99 aotocompletion tests are still not happy.

@rhatdan
Copy link
Member Author

rhatdan commented Aug 6, 2021

Thanks @Luap99

@TomSweeneyRedHat
Copy link
Member

@rhatdan test still aren't hip with this one.

@afbjorklund
Copy link
Contributor

@edsantiago "buildx" means to build using BuildKit, normally implemented as a CLI plugin

It has a lot of subcommands, the usual one being build (which works as docker build)

@@ -90,12 +90,20 @@ func init() {
Command: imageBuildCmd,
Parent: imageCmd,
})
registry.Commands = append(registry.Commands, registry.CliCommand{
Command: imageBuildCmd,
Parent: buildxCmd,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to create a separate command struct for buildx build. Do not reuse the imageBuildStruct. Cobra is not really designed to use the same command on several subcommands. Right now the use line in podman image build --help will show podman buildx build [options] [CONTEXT] instead of podman image build [options] [CONTEXT]. I think this is causing the other completion test to fail.

@markusthoemmes
Copy link
Contributor

Seeing as this seems to interfere quite a bit with cobra and how the help text is shown, I wonder if we should consider moving the "compatibility" layer to the docker script as initially proposed. It feels like that'd reduce the collateral damage area to the docker domain (where one could argue, it should live).

Any thoughts on that @rhatdan? I'm very green behind my podman ears so that might be total bogus as well 😂

@afbjorklund

This comment has been minimized.

@rhatdan
Copy link
Member Author

rhatdan commented Aug 9, 2021

Hopefully this one will work, I attempted the failing tests locally and it passed.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Add hidden --load and --progress flag as well.

Signed-off-by: Daniel J Walsh <[email protected]>
@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2021
@openshift-ci openshift-ci bot merged commit 6513add into containers:main Aug 9, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants