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

Make pull commands be consistent #3735

Merged

Conversation

TomSweeneyRedHat
Copy link
Member

Per @edsantiago 's suggestion, make the pull commands consistent, always
accepting a value. Currently we have:

--pull
--pull=true
--pull=false
--pull-never
--pull-always

With this changes, we will only have pull with a variety of options,
ala:

--pull
--pull=true
--pull=false
--pull=never
--pull=always

For backward compatibility, the --pull-never and --pull-always
options will remain operational, however they are not documented
and are conisdered deprecated.

Signed-off-by: tomsweeneyredhat tsweeney@redhat.com

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TomSweeneyRedHat

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

@TomSweeneyRedHat
Copy link
Member Author

If we merge this before we build the next release for Podman v4.0, I'll need to at least change the documentation in Podman too. @mheon, FYI

@rhatdan
Copy link
Member

rhatdan commented Jan 24, 2022

You need to mark --pull-always and --pull-never as hidden so that we don't see them in --help.

./bin/buildah build --help | grep pull
--pull string pull the image from the registry if newer or not present in store, if false, only pull the image if not present, if always, pull the image even if the named image is present in store, if never, only use the image present in store if available (default "true")
--pull-always pull the image even if the named image is present in store
--pull-never do not pull the image, use the image present in store if available

@rhatdan
Copy link
Member

rhatdan commented Jan 24, 2022

@TomSweeneyRedHat This is the time to get this done. Since it is a somewhat breaking change (Not really but a change in UI).

@TomSweeneyRedHat TomSweeneyRedHat force-pushed the dev/tsweeney/fixpull2 branch 2 times, most recently from 2ce93cc to cbe07e7 Compare January 25, 2022 01:40
@TomSweeneyRedHat
Copy link
Member Author

@Luap99 Cobra question for you, or maybe @bbaude. I switched the --pull option from a BoolVar to StrVar for the build and from commands. I set the default value for pull to "true". If I do:

buildah bud --pull --quiet .

The pull flag gets set to --quiet rather than the default true. The quiet option is gobbled by the pull and is ignored and the pull doesn't get set appropriately.

If I add an equal sign after the pull, ala:

buildah bud --pull= --quiet .

Then pull gets set to true, the quiet flag is not eaten, the build builds quietly, and the pull works as expected.

I've looked all over the limited Cobra doc to get this to work, and maybe my eyelids are just too heavy and I'm missing it at this point, but any tips/tricks for this? In a perfect world, I'd have the --pull and --pull= work the same way and go to the default value.

@Luap99
Copy link
Member

Luap99 commented Jan 25, 2022

You have to set flag.NoOptDefVal = "true"

@rhatdan
Copy link
Member

rhatdan commented Jan 25, 2022

LGTM once you add @Luap99 line.

@TomSweeneyRedHat
Copy link
Member Author

@Luap99 TYVM for the pointer, looks like:

        flags.Lookup("pull").NoOptDefVal = "true"

turns the trick. As a bonus, I finally located the docs for the Cobra flags. Off to test a bit more, then I'll resubmit.

@TomSweeneyRedHat TomSweeneyRedHat force-pushed the dev/tsweeney/fixpull2 branch 2 times, most recently from 7ef52a7 to 8eb4171 Compare January 25, 2022 15:32
@TomSweeneyRedHat
Copy link
Member Author

OK, this looks happy in my testing now. Hopefully, the CI will concur.

@rhatdan
Copy link
Member

rhatdan commented Jan 25, 2022

/lgtm
/hold

@TomSweeneyRedHat
Copy link
Member Author

One flake, one real error, I think I've the real one figured out. Pushing again soon.

Per @edsantiago 's suggestion, make the pull commands consistent, always
accepting a value.  Currently we have:

--pull
--pull=true
--pull=false
--pull-never
--pull-always

With this changes, we will only have pull with a variety of options,
ala:

--pull
--pull=true
--pull=false
--pull=never
--pull=always

For backward compatibility, the --pull-never and --pull-always
options will remain operational, however they are not documented
and are conisdered deprecated.

Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jan 25, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 25, 2022
@openshift-merge-robot openshift-merge-robot merged commit 350a835 into containers:main Jan 25, 2022
@nalind
Copy link
Member

nalind commented Jan 26, 2022

@TomSweeneyRedHat This is the time to get this done. Since it is a somewhat breaking change (Not really but a change in UI).

It's an API break in pkg/cli.

@rhatdan
Copy link
Member

rhatdan commented Jan 26, 2022

No the Old CLI is still handled.

@nalind
Copy link
Member

nalind commented Jan 26, 2022

Consumers that imported pkg/cli and expected BudResults.Pull to be a bool will fail to compile if they vendor the new version without patching their own source code. If we didn't break compatibility for the CLI, that's great, but it's not the same thing.

@rhatdan
Copy link
Member

rhatdan commented Jan 26, 2022

Do you want to bump the major version of Buildah?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants