-
Notifications
You must be signed in to change notification settings - Fork 760
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
Clarify definition of --pull options #5407
base: main
Are you sure you want to change the base?
Conversation
[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 |
efc0397
to
c584795
Compare
c2bcd22
to
08b1e2f
Compare
docs/buildah-build.1.md
Outdated
- **always**: Always pull the image and throw an error if the pull fails. | ||
- **missing**: Pull the image only when the image is not in the local containers storage. Throw an error if no image is found and the pull fails. | ||
- **never**: Never pull the image but use the one from the local containers storage. Throw an error if no image is found. | ||
- **newer**: Pull if the image on the registry is newer than the one in the local containers storage. An image is considered to be newer when the digests are different. Comparing the time stamps is prone to errors. Pull errors are suppressed if a local image was found. |
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.
this doesn't specify what happens with true or false, does it?
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.
No we want to hide those, since we do not want people using them any longer. Just use the newer terminology so it is consistent with Podman.
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.
This drops all mention of SBOM scanner images.
rebase needed. I think similar changes are needed to buildah-commit.1.md |
Ephemeral COPR build failed. @containers/packit-build please check. |
@TomSweeneyRedHat since buildah commit is pulling SBOM Scanner image, I will leave this to @nalind to decide, lets get this merged so that standard images work in the same was as podman. |
@rhatdan all kinds or red test unhappiness here. |
2d57606
to
ab48c9b
Compare
buildah.go
Outdated
// registry if a local copy of it is not already present. | ||
PullNever = define.PullNever | ||
) | ||
|
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.
API break.
define/pull.go
Outdated
"always": PullAlways, | ||
"never": PullNever, | ||
"ifnewer": PullIfNewer, | ||
} |
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.
API break.
define/build.go
Outdated
@@ -120,7 +121,7 @@ type BuildOptions struct { | |||
ContextDirectory string | |||
// PullPolicy controls whether or not we pull images. It should be one | |||
// of PullIfMissing, PullAlways, PullIfNewer, or PullNever. | |||
PullPolicy PullPolicy | |||
PullPolicy config.PullPolicy |
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.
This changes the default (unset) value from PullIfMissing
to config.PullPolicyAlways
.
always: pull images even if the named images are present in store, | ||
missing: pull images if the named images are not present in store, | ||
never: only use images present in store if available, | ||
newer: pull if the image on the registry is newer than the in store`) |
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.
Possibly this should be ifnewer
and not newer
? Related PR that fixes the docs.
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.
I would rather go with Podman's definitions so these matchup.
4b59b2a
to
d01a135
Compare
tests/bud.bats
Outdated
@@ -4254,6 +4254,9 @@ _EOF | |||
run_buildah 125 build $WITH_POLICY_JSON -t ${target} --pull=false $BUDFILES/pull | |||
expect_output --substring "busybox: image not known" | |||
|
|||
run_buildah build $WITH_POLICY_JSON -t ${target} --pull=missing $BUDFILES/pull | |||
expect_output --substring "COMMIT pull" | |||
|
|||
run_buildah build $WITH_POLICY_JSON -t ${target} --pull $BUDFILES/pull |
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.
This was previously going to have to pull the image in order for the build to complete at all, but the newly-inserted --pull=missing
run will make that unnecessary, so it either needs to specify a different base image, or it needs to be removed in between.
@@ -69,7 +69,12 @@ func init() { | |||
flags.StringVar(&opts.creds, "creds", "", "use `[username[:password]]` for accessing the registry") | |||
flags.StringVarP(&opts.format, "format", "f", defaultFormat(), "`format` of the image manifest and metadata") | |||
flags.StringVar(&opts.name, "name", "", "`name` for the working container") | |||
flags.StringVar(&opts.pull, "pull", "true", "pull images from the registry if newer or not present in store, if false, only pull images if not present, if always, pull images even if the named images are present in store, if never, only use images present in store if available") | |||
flags.StringVar(&opts.pull, "pull", "missing", `pull images from the registry values: |
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 commit log and PR description don't mention this change in the default. It seems important.
# Set --pull=false to prevent looking for a newer scratch3 image. | ||
run_buildah from --pull=false $WITH_POLICY_JSON scratch3 | ||
# Set --pull=never to prevent looking for a newer scratch3 image. | ||
run_buildah from --pull=never $WITH_POLICY_JSON scratch3 |
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.
Was the test failing without this change?
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.
No I could put it back and maybe test both pull=false and pull=never.
buildah from and buildah build will now default to --pull=missing as opposed to --pull=always, which they did before. This better matches to the defaults in docker and podman. No longer document --pull=true|false Fixes: containers#5406 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Fixes: #5406
What type of PR is this?
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?