Sync up define.PolicyMap and our internal parsing function

define/pull.go

@@ -38,16 +38,24 @@ func (p PullPolicy) String() string {
 	case PullAlways:
 		return "always"
 	case PullIfNewer:
-		return "ifnewer"
+		return "newer"
 	case PullNever:
 		return "never"
 	}
 	return fmt.Sprintf("unrecognized policy %d", p)
 }
 
+// PolicyMap maps from names of PullPolicy values (including aliases) to
+// PullPolicy values.
 var PolicyMap = map[string]PullPolicy{
-	"missing": PullIfMissing,
-	"always":  PullAlways,
-	"never":   PullNever,
-	"ifnewer": PullIfNewer,
+	"missing":    PullIfMissing,
+	"ifmissing":  PullIfMissing,
+	"notpresent": PullIfMissing,
+	"always":     PullAlways,
+	"true":       PullAlways,
+	"never":      PullNever,
+	"false":      PullNever,
+	"newer":      PullIfNewer,
+	"ifnewer":    PullIfNewer,
+	// This map is used by pkg/parse.pullPolicyWithFlags().
 }

define/pull_test.go

@@ -8,7 +8,9 @@ import (
 
 func TestPullPolicy(t *testing.T) {
 	t.Parallel()
-	for name, val := range PolicyMap {
-		assert.Equal(t, name, val.String())
+	for key, val := range PolicyMap {
+		t.Run(key, func(t *testing.T) {
+			assert.Equal(t, val, PolicyMap[val.String()])
+		})
 	}
 }

pkg/parse/parse.go

@@ -547,38 +547,19 @@ func SystemContextFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name strin
 
 // pullPolicyWithFlags parses a string value of a pull policy, evaluating it in
 // combination with "always" and "never" boolean flags.
-// Allow for:
-// * --pull
-// * --pull=""
-// * --pull=true
-// * --pull=false
-// * --pull=never
-// * --pull=always
-// * --pull=ifmissing
-// * --pull=missing
-// * --pull=notpresent
-// * --pull=newer
-// * --pull=ifnewer
-// and --pull-always and --pull-never as boolean flags.
+// Policy names and aliases are defined in define.PolicyMap.
 func pullPolicyWithFlags(policySpec string, always, never bool) (define.PullPolicy, error) {
 	if always {
 		return define.PullAlways, nil
 	}
 	if never {
 		return define.PullNever, nil
 	}
-	policy := strings.ToLower(policySpec)
-	switch policy {
-	case "missing", "ifmissing", "notpresent":
-		return define.PullIfMissing, nil
-	case "true", "always":
-		return define.PullAlways, nil
-	case "false", "never":
-		return define.PullNever, nil
-	case "ifnewer", "newer":
-		return define.PullIfNewer, nil
+	policy, ok := define.PolicyMap[strings.ToLower(policySpec)]
+	if !ok {
+		return 0, fmt.Errorf("unrecognized pull policy %q", policySpec)
 	}
-	return 0, fmt.Errorf("unrecognized pull policy %q", policySpec)
+	return policy, nil
 }
 
 // PullPolicyFromOptions returns a PullPolicy that reflects the combination of

pkg/parse/parse_test.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"slices"
 	"testing"
 
 	specs "github.com/opencontainers/runtime-spec/specs-go"
@@ -286,27 +287,17 @@ func TestParsePlatform(t *testing.T) {
 
 func TestParsePullPolicy(t *testing.T) {
 	t.Parallel()
-	testCases := map[string]bool{
-		"missing":    true,
-		"ifmissing":  true,
-		"notpresent": true,
-		"always":     true,
-		"true":       true,
-		"ifnewer":    true,
-		"newer":      true,
-		"false":      true,
-		"never":      true,
-		"try":        false,
-		"truth":      false,
+	for name, want := range define.PolicyMap {
+		t.Run(name, func(t *testing.T) {
+			got, err := pullPolicyWithFlags(name, false, false)
+			require.NoErrorf(t, err, "expected value %q to be recognized", name)
+			require.Equal(t, want, got)
+		})
 	}
-	for value, result := range testCases {
-		t.Run(value, func(t *testing.T) {
-			policy, err := pullPolicyWithFlags(value, false, false)
-			if result {
-				require.NoErrorf(t, err, "expected value %q to be recognized", value)
-			} else {
-				require.Errorf(t, err, "did not expect value %q to be recognized as %q", value, policy.String())
-			}
+	for name := range slices.Values([]string{"try", "truth"}) {
+		t.Run(name, func(t *testing.T) {
+			_, err := pullPolicyWithFlags(name, false, false)
+			require.Error(t, err)
 		})
 	}
 }

tests/bud.bats

@@ -8179,6 +8179,18 @@ _EOF
   cmp ${TEST_SCRATCH_DIR}/image1.txt ${TEST_SCRATCH_DIR}/image2.txt
 }
 
+@test "build-pull-values" {
+  local contextdir=${TEST_SCRATCH_DIR}/context
+  mkdir $contextdir
+  cat > $contextdir/Dockerfile << EOF
+  FROM scratch
+  ADD Dockerfile /Dockerfile
+EOF
+  for value in missing ifmissing notpresent always true never false ifnewer newer ; do
+    run_buildah build --pull="${value}" $contextdir
+  done
+}
+
 # Verify: https://github.com/containers/buildah/issues/5185
 @test "build-test --mount=type=secret test from env with chroot isolation" {
   skip_if_root_environment "Need to not be root for this test to work"

tests/chroot.bats

@@ -176,7 +176,7 @@ load helpers
   mkdir -p ${TEST_SCRATCH_DIR}/chroot/merged/var/lib/containers/storage
   chmod 755 ${TEST_SCRATCH_DIR}/chroot/merged/var/lib/containers/storage
   # https://github.com/podman-container-tools/buildah/issues/6967
-  # chown -R is not safe against concurent removal, it will exit 1 when
+  # chown -R is not safe against concurrent removal, it will exit 1 when
   # it happens but still walks all files so we can ignore the error here.
   # Bug: https://bugs.gnu.org/81444
   # Only once the fix landed in our test distro images coreutils version this workaround can be removed.

tests/pull.bats

@@ -401,3 +401,10 @@ load helpers
 
   assert "$amdiid" != "$armiid" "AMD and ARM ids should differ"
 }
+
+@test "pull-policy-values" {
+  image=quay.io/libpod/busybox
+  for value in missing ifmissing notpresent always true never false ifnewer newer ; do
+    run_buildah pull --platform=linux/amd64 --policy="${value}" "${image}"
+  done
+}