From 21fb5ea4f27891e9c4105037370e33f1b01101b9 Mon Sep 17 00:00:00 2001
From: Nalin Dahyabhai <nalin@redhat.com>
Date: Fri, 21 Jun 2024 15:31:59 -0400
Subject: [PATCH] Rework parsing of --pull flags

Rework parsing of --pull flags to add "newer" as an alias for "ifnewer",
and to reject unrecognized values instead of treating them as synonymous
with "ifmissing".

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
---
 define/pull.go          |  3 ++
 pkg/parse/parse.go      | 63 ++++++++++++++++++++++++++++++-----------
 pkg/parse/parse_test.go | 27 ++++++++++++++++++
 3 files changed, 76 insertions(+), 17 deletions(-)

diff --git a/define/pull.go b/define/pull.go
index 00787bd9b6..a98cc3d209 100644
--- a/define/pull.go
+++ b/define/pull.go
@@ -5,6 +5,9 @@ import (
 )
 
 // PullPolicy takes the value PullIfMissing, PullAlways, PullIfNewer, or PullNever.
+// N.B.: the enumeration values for this type differ from those used by
+// github.com/containers/common/pkg/config.PullPolicy (their zero values
+// indicate different policies), so they are not interchangeable.
 type PullPolicy int
 
 const (
diff --git a/pkg/parse/parse.go b/pkg/parse/parse.go
index fd802775a2..abc0a05d9e 100644
--- a/pkg/parse/parse.go
+++ b/pkg/parse/parse.go
@@ -449,6 +449,42 @@ func SystemContextFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name strin
 	return ctx, nil
 }
 
+// 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.
+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 "true", "missing", "ifmissing", "notpresent":
+		return define.PullIfMissing, nil
+	case "always":
+		return define.PullAlways, nil
+	case "false", "never":
+		return define.PullNever, nil
+	case "ifnewer", "newer":
+		return define.PullIfNewer, nil
+	}
+	return 0, fmt.Errorf("unrecognized pull policy %q", policySpec)
+}
+
 // 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) {
@@ -474,30 +510,23 @@ func PullPolicyFromFlagSet(flags *pflag.FlagSet, findFlagFunc func(name string)
 		return 0, errors.New("can only set one of 'pull' or 'pull-always' or 'pull-never'")
 	}
 
-	// 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
-	}
+	// The --pull-never and --pull-always options will not be documented.
 	pullAlwaysFlagValue, err := flags.GetBool("pull-always")
 	if err != nil {
-		return 0, err
-	}
-	if pullAlwaysFlagValue || strings.EqualFold(pullFlagValue, "always") {
-		pullPolicy = define.PullAlways
+		return 0, fmt.Errorf("checking the --pull-always flag value: %w", err)
 	}
 	pullNeverFlagValue, err := flags.GetBool("pull-never")
 	if err != nil {
-		return 0, err
+		return 0, fmt.Errorf("checking the --pull-never flag value: %w", err)
 	}
-	if pullNeverFlagValue ||
-		strings.EqualFold(pullFlagValue, "never") ||
-		strings.EqualFold(pullFlagValue, "false") {
-		pullPolicy = define.PullNever
+
+	// The --pull[=...] flag is the one we really care about.
+	pullFlagValue := findFlagFunc("pull").Value.String()
+	pullPolicy, err := pullPolicyWithFlags(pullFlagValue, pullAlwaysFlagValue, pullNeverFlagValue)
+	if err != nil {
+		return 0, err
 	}
+
 	logrus.Debugf("Pull Policy for pull [%v]", pullPolicy)
 
 	return pullPolicy, nil
diff --git a/pkg/parse/parse_test.go b/pkg/parse/parse_test.go
index c1f8833604..f5b12e1d45 100644
--- a/pkg/parse/parse_test.go
+++ b/pkg/parse/parse_test.go
@@ -10,6 +10,7 @@ import (
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCommonBuildOptionsFromFlagSet(t *testing.T) {
@@ -193,6 +194,32 @@ func TestParsePlatform(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestParsePullPolicy(t *testing.T) {
+	testCases := map[string]bool{
+		"missing":    true,
+		"ifmissing":  true,
+		"notpresent": true,
+		"always":     true,
+		"true":       true,
+		"ifnewer":    true,
+		"newer":      true,
+		"false":      true,
+		"never":      true,
+		"trye":       false,
+		"truth":      false,
+	}
+	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())
+			}
+		})
+	}
+}
+
 func TestSplitStringWithColonEscape(t *testing.T) {
 	tests := []struct {
 		volume         string