From 56fe1c09d259c84a697e9bab609b315bcff2862c Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Sat, 16 Mar 2024 07:30:12 -0400 Subject: [PATCH] Clarify definition of --pull options Use github.com/containers/common/pkg/config for handling pull policy. No longer document --pull=true|false Fixes: https://github.com/containers/buildah/issues/5406 Signed-off-by: Daniel J Walsh --- buildah.go | 26 ++---------------- cmd/buildah/from.go | 7 ++++- cmd/buildah/pull.go | 8 +++--- convertcw.go | 3 +- define/build.go | 3 +- define/pull.go | 50 ---------------------------------- define/pull_test.go | 13 --------- define/types.go | 3 +- docs/buildah-build.1.md | 31 ++++----------------- docs/buildah-from.1.md | 21 +++----------- imagebuildah/build.go | 8 +++--- imagebuildah/executor.go | 2 +- imagebuildah/stage_executor.go | 8 +++--- pkg/cli/common.go | 7 ++++- pkg/parse/parse.go | 22 +++++++-------- pull.go | 3 +- tests/bud.bats | 3 ++ tests/from.bats | 4 +-- 18 files changed, 59 insertions(+), 163 deletions(-) delete mode 100644 define/pull.go delete mode 100644 define/pull_test.go diff --git a/buildah.go b/buildah.go index 1aa6e173de..61a1ebe912 100644 --- a/buildah.go +++ b/buildah.go @@ -14,6 +14,7 @@ import ( "github.com/containers/buildah/define" "github.com/containers/buildah/docker" nettypes "github.com/containers/common/libnetwork/types" + "github.com/containers/common/pkg/config" "github.com/containers/image/v5/types" encconfig "github.com/containers/ocicrypt/config" "github.com/containers/storage" @@ -40,29 +41,6 @@ const ( stateFile = Package + ".json" ) -// PullPolicy takes the value PullIfMissing, PullAlways, PullIfNewer, or PullNever. -type PullPolicy = define.PullPolicy - -const ( - // PullIfMissing is one of the values that BuilderOptions.PullPolicy - // can take, signalling that the source image should be pulled from a - // registry if a local copy of it is not already present. - PullIfMissing = define.PullIfMissing - // PullAlways is one of the values that BuilderOptions.PullPolicy can - // take, signalling that a fresh, possibly updated, copy of the image - // should be pulled from a registry before the build proceeds. - PullAlways = define.PullAlways - // PullIfNewer is one of the values that BuilderOptions.PullPolicy - // can take, signalling that the source image should only be pulled - // from a registry if a local copy is not already present or if a - // newer version the image is present on the repository. - PullIfNewer = define.PullIfNewer - // PullNever is one of the values that BuilderOptions.PullPolicy can - // take, signalling that the source image should not be pulled from a - // registry if a local copy of it is not already present. - PullNever = define.PullNever -) - // NetworkConfigurationPolicy takes the value NetworkDefault, NetworkDisabled, // or NetworkEnabled. type NetworkConfigurationPolicy = define.NetworkConfigurationPolicy @@ -283,7 +261,7 @@ type BuilderOptions struct { // PullPolicy decides whether or not we should pull the image that // we're using as a base image. It should be PullIfMissing, // PullAlways, or PullNever. - PullPolicy define.PullPolicy + PullPolicy config.PullPolicy // Registry is a value which is prepended to the image's name, if it // needs to be pulled and the image name alone can not be resolved to a // reference to a source image. No separator is implicitly added. diff --git a/cmd/buildah/from.go b/cmd/buildah/from.go index f5e02a331c..b7687a5cac 100644 --- a/cmd/buildah/from.go +++ b/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: +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`) + flags.Lookup("pull").NoOptDefVal = "true" //allow `--pull ` to be set to `true` as expected. flags.BoolVar(&opts.pullAlways, "pull-always", false, "pull the image even if the named image is present in store") diff --git a/cmd/buildah/pull.go b/cmd/buildah/pull.go index 72b0b1c999..9abcd69ca0 100644 --- a/cmd/buildah/pull.go +++ b/cmd/buildah/pull.go @@ -8,10 +8,10 @@ import ( "time" "github.com/containers/buildah" - "github.com/containers/buildah/define" "github.com/containers/buildah/pkg/cli" "github.com/containers/buildah/pkg/parse" "github.com/containers/common/pkg/auth" + "github.com/containers/common/pkg/config" "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -119,9 +119,9 @@ func pullCmd(c *cobra.Command, args []string, iopts pullOptions) error { return fmt.Errorf("unable to obtain decryption config: %w", err) } - policy, ok := define.PolicyMap[iopts.pullPolicy] - if !ok { - return fmt.Errorf("unsupported pull policy %q", iopts.pullPolicy) + policy, err := config.ParsePullPolicy(iopts.pullPolicy) + if err != nil { + return err } options := buildah.PullOptions{ SignaturePolicyPath: iopts.signaturePolicy, diff --git a/convertcw.go b/convertcw.go index be12b7f848..20f8e72410 100644 --- a/convertcw.go +++ b/convertcw.go @@ -8,6 +8,7 @@ import ( "github.com/containers/buildah/define" "github.com/containers/buildah/internal/mkcw" + "github.com/containers/common/pkg/config" "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/types" encconfig "github.com/containers/ocicrypt/config" @@ -50,7 +51,7 @@ type CWConvertImageOptions struct { // Passed through to BuilderOptions. Most settings won't make // sense to be made available here because we don't launch a process. ContainerSuffix string - PullPolicy PullPolicy + PullPolicy config.PullPolicy BlobDirectory string SignaturePolicyPath string ReportWriter io.Writer diff --git a/define/build.go b/define/build.go index e361995582..b1d4ed4aea 100644 --- a/define/build.go +++ b/define/build.go @@ -5,6 +5,7 @@ import ( "time" nettypes "github.com/containers/common/libnetwork/types" + "github.com/containers/common/pkg/config" "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/types" encconfig "github.com/containers/ocicrypt/config" @@ -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 // Registry is a value which is prepended to the image's name, if it // needs to be pulled and the image name alone can not be resolved to a // reference to a source image. No separator is implicitly added. diff --git a/define/pull.go b/define/pull.go deleted file mode 100644 index 00787bd9b6..0000000000 --- a/define/pull.go +++ /dev/null @@ -1,50 +0,0 @@ -package define - -import ( - "fmt" -) - -// PullPolicy takes the value PullIfMissing, PullAlways, PullIfNewer, or PullNever. -type PullPolicy int - -const ( - // PullIfMissing is one of the values that BuilderOptions.PullPolicy - // can take, signalling that the source image should be pulled from a - // registry if a local copy of it is not already present. - PullIfMissing PullPolicy = iota - // PullAlways is one of the values that BuilderOptions.PullPolicy can - // take, signalling that a fresh, possibly updated, copy of the image - // should be pulled from a registry before the build proceeds. - PullAlways - // PullIfNewer is one of the values that BuilderOptions.PullPolicy - // can take, signalling that the source image should only be pulled - // from a registry if a local copy is not already present or if a - // newer version the image is present on the repository. - PullIfNewer - // PullNever is one of the values that BuilderOptions.PullPolicy can - // take, signalling that the source image should not be pulled from a - // registry. - PullNever -) - -// String converts a PullPolicy into a string. -func (p PullPolicy) String() string { - switch p { - case PullIfMissing: - return "missing" - case PullAlways: - return "always" - case PullIfNewer: - return "ifnewer" - case PullNever: - return "never" - } - return fmt.Sprintf("unrecognized policy %d", p) -} - -var PolicyMap = map[string]PullPolicy{ - "missing": PullIfMissing, - "always": PullAlways, - "never": PullNever, - "ifnewer": PullIfNewer, -} diff --git a/define/pull_test.go b/define/pull_test.go deleted file mode 100644 index fbaf8319f6..0000000000 --- a/define/pull_test.go +++ /dev/null @@ -1,13 +0,0 @@ -package define - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestPullPolicy(t *testing.T) { - for name, val := range PolicyMap { - assert.Equal(t, name, val.String()) - } -} diff --git a/define/types.go b/define/types.go index e8fbaf8d2c..3596b186f9 100644 --- a/define/types.go +++ b/define/types.go @@ -14,6 +14,7 @@ import ( "path/filepath" "strings" + "github.com/containers/common/pkg/config" "github.com/containers/image/v5/manifest" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chrootarchive" @@ -159,7 +160,7 @@ const ( type SBOMScanOptions struct { Type []string // a shorthand name for a defined group of these options Image string // the scanner image to use - PullPolicy PullPolicy // how to get the scanner image + PullPolicy config.PullPolicy // how to get the scanner image Commands []string // one or more commands to invoke for the image rootfs or ContextDir locations ContextDir []string // one or more "source" directory locations SBOMOutput string // where to save SBOM scanner output outside of the image (i.e., the local filesystem) diff --git a/docs/buildah-build.1.md b/docs/buildah-build.1.md index 25b7cb6794..76a90542a1 100644 --- a/docs/buildah-build.1.md +++ b/docs/buildah-build.1.md @@ -746,31 +746,12 @@ The `buildah build` command allows building images for all Linux architectures, **--pull** -When the *--pull* flag is enabled or set explicitly to `true` (with -*--pull=true*), attempt to pull the latest versions of base and SBOM scanner -images from the registries listed in registries.conf if a local base or SBOM -scanner image does not exist or the image in the registry is newer than the one -in local storage. Raise an error if the base or SBOM scanner image is not in -any listed registry and is not present locally. - -If the flag is disabled (with *--pull=false*), do not pull base and SBOM -scanner images from registries, use only local versions. Raise an error if a -base or SBOM scanner image is not present locally. - -If the pull flag is set to `always` (with *--pull=always*), pull base and SBOM -scanner images from the registries listed in registries.conf. Raise an error -if a base or SBOM scanner image is not found in the registries, even if an -image with the same name is present locally. - -If the pull flag is set to `missing` (with *--pull=missing*), pull base and -SBOM scanner images only if they could not be found in the local containers -storage. Raise an error if no image could be found and the pull fails. - -If the pull flag is set to `never` (with *--pull=never*), do not pull base and -SBOM scanner images from registries, use only the local versions. Raise an -error if the image is not present locally. - -Defaults to *true*. +Pull image policy. The default is **missing**. + +- **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. **--quiet**, **-q** diff --git a/docs/buildah-from.1.md b/docs/buildah-from.1.md index 082e920308..8ead513f9c 100644 --- a/docs/buildah-from.1.md +++ b/docs/buildah-from.1.md @@ -388,24 +388,11 @@ the help of emulation provided by packages like `qemu-user-static`. **--pull** -When the flag is enabled or set explicitly to `true` (with *--pull=true*), attempt to pull the latest image from the registries -listed in registries.conf if a local image does not exist or the image is newer -than the one in storage. Raise an error if the image is not in any listed -registry and is not present locally. +Pull image policy. The default is **missing**. -If the flag is disabled (with *--pull=false*), do not pull the image from the -registry, use only the local version. Raise an error if the image is not -present locally. - -If the pull flag is set to `always` (with *--pull=always*), -pull the image from the first registry it is found in as listed in registries.conf. -Raise an error if not found in the registries, even if the image is present locally. - -If the pull flag is set to `never` (with *--pull=never*), -Do not pull the image from the registry, use only the local version. Raise an error -if the image is not present locally. - -Defaults to *true*. +- **always**, **true**: Always pull the image and throw an error if the pull fails. +- **missing**: Only pull the image when it does not exist in the local containers storage. Throw an error if no image is found and the pull fails. +- **never**, **false**: Never pull the image but use the one from the local containers storage. Throw an error when no image is found. **--quiet**, **-q** diff --git a/imagebuildah/build.go b/imagebuildah/build.go index 39e9837063..92e2fe7ec7 100644 --- a/imagebuildah/build.go +++ b/imagebuildah/build.go @@ -42,10 +42,10 @@ import ( ) const ( - PullIfMissing = define.PullIfMissing - PullAlways = define.PullAlways - PullIfNewer = define.PullIfNewer - PullNever = define.PullNever + PullAlways = config.PullPolicyAlways + PullIfMissing = config.PullPolicyMissing + PullIfNewer = config.PullPolicyNewer + PullNever = config.PullPolicyNever Gzip = archive.Gzip Bzip2 = archive.Bzip2 diff --git a/imagebuildah/executor.go b/imagebuildah/executor.go index ad5414a9a1..72fd2279d8 100644 --- a/imagebuildah/executor.go +++ b/imagebuildah/executor.go @@ -69,7 +69,7 @@ type Executor struct { stages map[string]*StageExecutor store storage.Store contextDir string - pullPolicy define.PullPolicy + pullPolicy config.PullPolicy registry string ignoreUnrecognizedInstructions bool quiet bool diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index 99aa3a0c75..b7634bc2c3 100644 --- a/imagebuildah/stage_executor.go +++ b/imagebuildah/stage_executor.go @@ -846,7 +846,7 @@ func (s *StageExecutor) UnrecognizedInstruction(step *imagebuilder.Step) error { // prepare creates a working container based on the specified image, or if one // isn't specified, the first argument passed to the first FROM instruction we // can find in the stage's parsed tree. -func (s *StageExecutor) prepare(ctx context.Context, from string, initializeIBConfig, rebase, preserveBaseImageAnnotations bool, pullPolicy define.PullPolicy) (builder *buildah.Builder, err error) { +func (s *StageExecutor) prepare(ctx context.Context, from string, initializeIBConfig, rebase, preserveBaseImageAnnotations bool, pullPolicy config.PullPolicy) (builder *buildah.Builder, err error) { stage := s.stage ib := stage.Builder node := stage.Node @@ -1087,7 +1087,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string, var preserveBaseImageAnnotationsAtStageStart bool if stageImage, isPreviousStage := s.executor.imageMap[base]; isPreviousStage { base = stageImage - pullPolicy = define.PullNever + pullPolicy = config.PullPolicyNever preserveBaseImageAnnotationsAtStageStart = true } s.executor.stagesLock.Unlock() @@ -1668,7 +1668,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string, // Enforce pull "never" since we already have an image // ID that we really should not be pulling anymore (see // containers/podman/issues/10307). - if _, err := s.prepare(ctx, imgID, false, true, true, define.PullNever); err != nil { + if _, err := s.prepare(ctx, imgID, false, true, true, config.PullPolicyNever); err != nil { return "", nil, false, fmt.Errorf("preparing container for next step: %w", err) } } @@ -2023,7 +2023,7 @@ func (s *StageExecutor) pullCache(ctx context.Context, cacheKey string) (referen RetryDelay: s.executor.retryPullPushDelay, AllTags: false, ReportWriter: nil, - PullPolicy: define.PullIfNewer, + PullPolicy: config.PullPolicyNewer, } id, err := buildah.Pull(ctx, src.DockerReference().String(), options) if err != nil { diff --git a/pkg/cli/common.go b/pkg/cli/common.go index 31c52f6e0f..14a227622a 100644 --- a/pkg/cli/common.go +++ b/pkg/cli/common.go @@ -263,7 +263,12 @@ func GetBudFlags(flags *BudResults) pflag.FlagSet { fs.String("os", runtime.GOOS, "set the OS to the provided value instead of the current operating system of the host") fs.StringArrayVar(&flags.OSFeatures, "os-feature", []string{}, "set required OS `feature` for the target image in addition to values from the base image") fs.StringVar(&flags.OSVersion, "os-version", "", "set required OS `version` for the target image instead of the value from the base image") - fs.StringVar(&flags.Pull, "pull", "true", "pull base and SBOM scanner images from the registry if newer or not present in store, if false, only pull base and SBOM scanner images if not present, if always, pull base and SBOM scanner images even if the named images are present in store, if never, only use images present in store if available") + fs.StringVar(&flags.Pull, "pull", "missing", `pull base and SBOM scanner images from the registry. Values: +true: pull if newer or not present in store. +false: never pull base and SBOM scanner images. +always: pull base and SBOM scanner images even if the named images are present in store. +missing: pull base and SBOM scanner images if the named images are not present in store. +never: only use images present in store if available`) fs.Lookup("pull").NoOptDefVal = "true" //allow `--pull ` to be set to `true` as expected. fs.BoolVar(&flags.PullAlways, "pull-always", false, "pull the image even if the named image is present in store") if err := fs.MarkHidden("pull-always"); err != nil { diff --git a/pkg/parse/parse.go b/pkg/parse/parse.go index fd802775a2..a4e62f93e6 100644 --- a/pkg/parse/parse.go +++ b/pkg/parse/parse.go @@ -451,13 +451,13 @@ func SystemContextFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name strin // PullPolicyFromOptions returns a PullPolicy that reflects the combination of // the specified "pull" and undocumented "pull-always" and "pull-never" flags. -func PullPolicyFromOptions(c *cobra.Command) (define.PullPolicy, error) { +func PullPolicyFromOptions(c *cobra.Command) (config.PullPolicy, error) { return PullPolicyFromFlagSet(c.Flags(), c.Flag) } // PullPolicyFromFlagSet returns a PullPolicy that reflects the combination of // the specified "pull" and undocumented "pull-always" and "pull-never" flags. -func PullPolicyFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name string) *pflag.Flag) (define.PullPolicy, error) { +func PullPolicyFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name string) *pflag.Flag) (config.PullPolicy, error) { pullFlagsCount := 0 if findFlagFunc("pull").Changed { @@ -477,30 +477,28 @@ func PullPolicyFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name string) // Allow for --pull, --pull=true, --pull=false, --pull=never, --pull=always // --pull-always and --pull-never. The --pull-never and --pull-always options // will not be documented. - pullPolicy := define.PullIfMissing + pullFlagValue := findFlagFunc("pull").Value.String() - if strings.EqualFold(pullFlagValue, "true") || strings.EqualFold(pullFlagValue, "ifnewer") { - pullPolicy = define.PullIfNewer - } pullAlwaysFlagValue, err := flags.GetBool("pull-always") if err != nil { return 0, err } - if pullAlwaysFlagValue || strings.EqualFold(pullFlagValue, "always") { - pullPolicy = define.PullAlways + if pullAlwaysFlagValue || + strings.EqualFold(pullFlagValue, "true") { + return config.PullPolicyAlways, nil } + pullNeverFlagValue, err := flags.GetBool("pull-never") if err != nil { return 0, err } + if pullNeverFlagValue || - strings.EqualFold(pullFlagValue, "never") || strings.EqualFold(pullFlagValue, "false") { - pullPolicy = define.PullNever + return config.PullPolicyNever, nil } - logrus.Debugf("Pull Policy for pull [%v]", pullPolicy) - return pullPolicy, nil + return config.ParsePullPolicy(pullFlagValue) } func getAuthFile(authfile string) string { diff --git a/pull.go b/pull.go index 343c61fba7..22a1c415ae 100644 --- a/pull.go +++ b/pull.go @@ -6,7 +6,6 @@ import ( "io" "time" - "github.com/containers/buildah/define" "github.com/containers/common/libimage" "github.com/containers/common/pkg/config" "github.com/containers/image/v5/types" @@ -49,7 +48,7 @@ type PullOptions struct { // encrypted if non-nil. If nil, it does not attempt to decrypt an image. OciDecryptConfig *encconfig.DecryptConfig // PullPolicy takes the value PullIfMissing, PullAlways, PullIfNewer, or PullNever. - PullPolicy define.PullPolicy + PullPolicy config.PullPolicy } // Pull copies the contents of the image from somewhere else to local storage. Returns the diff --git a/tests/bud.bats b/tests/bud.bats index ec0837c50d..63882b7761 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -4197,6 +4197,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 expect_output --substring "COMMIT pull" diff --git a/tests/from.bats b/tests/from.bats index dfd4486c60..9496114488 100644 --- a/tests/from.bats +++ b/tests/from.bats @@ -78,8 +78,8 @@ load helpers expect_output "" "no base name for scratch" run_buildah rm $cid run_buildah tag scratch2 scratch3 - # 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 expect_output --substring "scratch3-working-container" run_buildah rm $output run_buildah rmi scratch2 scratch3