Sync up define.PolicyMap and our internal parsing function

[nalind]

What type of PR is this?

/kind bug

What this PR does / why we need it:

The internal CLI parsing function accepts aliases for a number of pull policy names, but we also export a map for API callers and for buildah pull, and it didn't include all of the same values. Rework the parsing function to use the map as the entire set of definitions.

How to verify it

Updated unit test!
New integration test!

Which issue(s) this PR fixes:

Fixes #6992

Special notes for your reviewer:

Does this PR introduce a user-facing change?

`buildah pull` now recognizes `--policy=newer` as equivalent to `--policy=ifnewer`.

[leonardomoreira00]

If you permit me, this is related to my issue and I was looking how to fix this code.
I would like to review your changes to see if I can contribute.

[nalind]

Go for it!

[leonardomoreira00]

Suggested to extend the tests of TestParsePullPolicy.

If the newly introduced define.PolicyMap can be used as a source of truth, I would change the follwing file and the tests accordingly:

diff --git a/pkg/parse/parse.go b/pkg/parse/parse.go
index 5a68eb852..d90bb9351 100644
--- a/pkg/parse/parse.go
+++ b/pkg/parse/parse.go
@@ -547,19 +547,7 @@ 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 by define.PolicyMap.
 func pullPolicyWithFlags(policySpec string, always, never bool) (define.PullPolicy, error) {
 	if always {
 		return define.PullAlways, nil
@@ -567,18 +555,12 @@ func pullPolicyWithFlags(policySpec string, always, never bool) (define.PullPoli
 	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

[leonardomoreira00]

I would remove this part, because it compares the map value with itself, and would extend the test case pkg/parse/parse_test.go:287

func TestParsePullPolicy(t *testing.T) {
...
	for name, want := range define.PolicyMap {
		t.Run(name, func(t *testing.T) {
			got, err := pullPolicyWithFlags(name, false, false)
			require.NoError(t, err)
			assert.Equal(t, want, got)
		})
	}

	for _, name := range []string{"try", "truth"} {
		t.Run(name, func(t *testing.T) {
			_, err := pullPolicyWithFlags(name, false, false)
			require.Error(t, err)
		})
	}
}

By doing this we make the test more robust, evaluating if the parser is also mapping the define.PullPolicy correctly for the existing and also future policies.

A wrong match/map in the `pullPolicyWithFlags()' would be identified as a bug:

	case "true", "always":
		return define.PullNever, nil

Could pullPolicyWithFlags use define.PolicyMap directly? That would give us one source of truth.

[TomSweeneyRedHat]

Other than @leonardomoreira00 's comments,
LGTM

[TomSweeneyRedHat]

I'm going to hold off merging this and #6993 until the dust settles a bit and we pick one to go forward with.

[nalind]

Okay, I think I've incorporated the requested changes.
I added a change in the default --policy setting for buildah pull to make it line up with podman's. It's okay that it's different from the default used by buildah build.