Clarify definition of --pull options #5407

rhatdan · 2024-03-16T11:35:53Z

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 · 2024-03-16T11:35:59Z

[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:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TomSweeneyRedHat · 2024-04-06T18:32:29Z

docs/buildah-build.1.md

+- **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.


this doesn't specify what happens with true or false, does it?

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.

This drops all mention of SBOM scanner images.

TomSweeneyRedHat · 2024-04-06T18:49:13Z

rebase needed. I think similar changes are needed to buildah-commit.1.md

packit-as-a-service · 2024-04-16T18:54:13Z

Ephemeral COPR build failed. @containers/packit-build please check.

rhatdan · 2024-04-16T18:57:08Z

@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.

TomSweeneyRedHat · 2024-04-18T13:32:29Z

@rhatdan all kinds or red test unhappiness here.

nalind · 2024-04-24T16:20:39Z

buildah.go

-	// registry if a local copy of it is not already present.
-	PullNever = define.PullNever
-)
-


nalind · 2024-04-24T16:20:43Z

define/pull.go

-	"always":  PullAlways,
-	"never":   PullNever,
-	"ifnewer": PullIfNewer,
-}


nalind · 2024-04-24T17:46:29Z

define/build.go

@@ -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


This changes the default (unset) value from PullIfMissing to config.PullPolicyAlways.

krrg · 2024-05-10T06:16:04Z

cmd/buildah/from.go

+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`)


Possibly this should be ifnewer and not newer? Related PR that fixes the docs.

I would rather go with Podman's definitions so these matchup.

nalind · 2024-06-04T20:50:56Z

tests/bud.bats

@@ -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


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.

nalind · 2024-06-04T20:54:34Z

cmd/buildah/from.go

@@ -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:


The commit log and PR description don't mention this change in the default. It seems important.

nalind · 2024-06-04T20:55:31Z

tests/from.bats

-  # 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


Was the test failing without this change?

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>

openshift-ci bot added the approved label Mar 16, 2024

rhatdan force-pushed the pull branch 4 times, most recently from efc0397 to c584795 Compare March 20, 2024 06:09

rhatdan force-pushed the pull branch 2 times, most recently from c2bcd22 to 08b1e2f Compare March 31, 2024 10:15

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2024

TomSweeneyRedHat reviewed Apr 6, 2024

View reviewed changes

rhatdan force-pushed the pull branch from 08b1e2f to 41db443 Compare April 16, 2024 18:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024

rhatdan force-pushed the pull branch 3 times, most recently from 2d57606 to ab48c9b Compare April 24, 2024 15:28

nalind reviewed Apr 24, 2024

View reviewed changes

buildah.go Outdated

// registry if a local copy of it is not already present.

PullNever = define.PullNever

)

Copy link

Member

nalind Apr 24, 2024

Choose a reason for hiding this comment

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

API break.

nalind reviewed Apr 24, 2024

View reviewed changes

define/pull.go Outdated

"always": PullAlways,

"never": PullNever,

"ifnewer": PullIfNewer,

}

Copy link

Member

nalind Apr 24, 2024

Choose a reason for hiding this comment

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

API break.

nalind reviewed Apr 24, 2024

View reviewed changes

krrg reviewed May 10, 2024

View reviewed changes

rhatdan force-pushed the pull branch 2 times, most recently from 4b59b2a to d01a135 Compare June 4, 2024 19:29

nalind reviewed Jun 4, 2024

View reviewed changes

rhatdan force-pushed the pull branch from d01a135 to 1455227 Compare June 4, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify definition of --pull options #5407

Clarify definition of --pull options #5407

rhatdan commented Mar 16, 2024

openshift-ci bot commented Mar 16, 2024

TomSweeneyRedHat Apr 6, 2024

rhatdan Apr 16, 2024

nalind Apr 24, 2024

TomSweeneyRedHat commented Apr 6, 2024

packit-as-a-service bot commented Apr 16, 2024

rhatdan commented Apr 16, 2024

TomSweeneyRedHat commented Apr 18, 2024

nalind Apr 24, 2024

nalind Apr 24, 2024

nalind Apr 24, 2024

krrg May 10, 2024

rhatdan Jun 4, 2024

nalind Jun 4, 2024

nalind Jun 4, 2024

nalind Jun 4, 2024

rhatdan Jun 4, 2024

Clarify definition of --pull options #5407

Are you sure you want to change the base?

Clarify definition of --pull options #5407

Conversation

rhatdan commented Mar 16, 2024

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?

openshift-ci bot commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Apr 6, 2024

packit-as-a-service bot commented Apr 16, 2024

rhatdan commented Apr 16, 2024

TomSweeneyRedHat commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment