Move image pull policy processing to podspec.go

pkg/kn/commands/service/configuration_edit_flags.go

@@ -268,14 +268,6 @@ func (p *ConfigurationEditFlags) Apply(
 		servinglib.UnsetUserImageAnnotation(template)
 	}
 
-	if cmd.Flags().Changed("pull-policy") {
-
-		err = servinglib.UpdateImagePullPolicy(template, p.PodSpecFlags.ImagePullPolicy)
-		if err != nil {
-			return err
-		}
-	}
-
 	// Deprecated "min-scale" in 0.19, updated to "scale-min"
 	if cmd.Flags().Changed("scale-min") || cmd.Flags().Changed("min-scale") {
 		err = servinglib.UpdateMinScale(template, p.MinScale)

pkg/kn/flags/podspec.go

@@ -289,6 +289,14 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, flags *pflag.Flag
 		}
 	}
 
+	if flags.Changed("pull-policy") {
+
+		err = UpdateImagePullPolicy(podSpec, p.ImagePullPolicy)
+		if err != nil {
+			return err
+		}
+	}
+
 	requestsToRemove, limitsToRemove, err := p.Resources.Validate()
 	if err != nil {
 		return err

pkg/kn/flags/podspec_helper.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/yaml"
 
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"knative.dev/client/pkg/util"
 )
@@ -301,6 +302,35 @@ func UpdateContainers(spec *corev1.PodSpec, containers []corev1.Container) {
 	}
 }
 
+// UpdateImagePullPolicy updates the pull policy for the given revision template
+func UpdateImagePullPolicy(spec *corev1.PodSpec, imagePullPolicy string) error {
+	container := containerOfPodSpec(spec)
+
+	if !isValidPullPolicy(imagePullPolicy) {
+		return fmt.Errorf("invalid --pull-policy %s. Valid arguments (case insensitive): Always | Never | IfNotPresent", imagePullPolicy)
+	}
+	container.ImagePullPolicy = getPolicy(imagePullPolicy)
+	return nil
+}
+
+func getPolicy(policy string) v1.PullPolicy {
+	var ret v1.PullPolicy
+	switch strings.ToLower(policy) {
+	case "always":
+		ret = v1.PullAlways
+	case "ifnotpresent":
+		ret = v1.PullIfNotPresent
+	case "never":
+		ret = v1.PullNever
+	}
+	return ret
+}
+
+func isValidPullPolicy(policy string) bool {
+	validPolicies := []string{string(v1.PullAlways), string(v1.PullNever), string(v1.PullIfNotPresent)}
+	return util.SliceContainsIgnoreCase(validPolicies, policy)
+}
+
 // =======================================================================================
 func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate *util.OrderedMap) []corev1.EnvVar {
 	updated := sets.NewString()

pkg/kn/flags/podspec_helper_test.go

@@ -892,3 +892,66 @@ containers:
 	assert.Assert(t, err != nil)
 	assert.Assert(t, util.ContainsAll(err.Error(), "no", "file", "directory"))
 }
+
+func TestUpdateImagePullPolicy(t *testing.T) {
+	policyMap := make(map[string][]string)
+	policyMap["Always"] = []string{"always", "ALWAYS", "Always"}
+	policyMap["Never"] = []string{"never", "NEVER", "Never"}
+	policyMap["IfNotPresent"] = []string{"ifnotpresent", "IFNOTPRESENT", "IfNotPresent"}
+
+	for k, values := range policyMap {
+		expectedPodSpec := &corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Image:           "repo/user/imageID:tag",
+					ImagePullPolicy: corev1.PullPolicy(k),
+					Command:         []string{"/app/start"},
+					Args:            []string{"myArg1"},
+					Ports: []corev1.ContainerPort{
+						{
+							ContainerPort: 8080,
+						},
+					},
+				},
+			},
+		}
+		for _, v := range values {
+			podSpec := &corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Image:   "repo/user/imageID:tag",
+						Command: []string{"/app/start"},
+						Args:    []string{"myArg1"},
+						Ports: []corev1.ContainerPort{
+							{
+								ContainerPort: 8080,
+							},
+						},
+					},
+				},
+			}
+			err := UpdateImagePullPolicy(podSpec, v)
+			assert.NilError(t, err, "update pull policy failed")
+			assert.DeepEqual(t, expectedPodSpec, podSpec)
+		}
+	}
+}
+
+func TestUpdateImagePullPolicyError(t *testing.T) {
+	podSpec := &corev1.PodSpec{
+		Containers: []corev1.Container{
+			{
+				Image:   "repo/user/imageID:tag",
+				Command: []string{"/app/start"},
+				Args:    []string{"myArg1"},
+				Ports: []corev1.ContainerPort{
+					{
+						ContainerPort: 8080,
+					},
+				},
+			},
+		},
+	}
+	err := UpdateImagePullPolicy(podSpec, "InvalidPolicy")
+	assert.Assert(t, util.ContainsAll(err.Error(), "invalid --pull-policy", "Valid arguments", "Always | Never | IfNotPresent"))
+}

pkg/kn/flags/podspec_test.go

@@ -70,13 +70,14 @@ func TestPodSpecResolve(t *testing.T) {
 		"--port", "8080", "--limit", "cpu=1000m", "--limit", "memory=1024Mi",
 		"--cmd", "/app/start", "--arg", "myArg1", "--service-account", "foo-bar-account",
 		"--mount", "/mount/path=volume-name", "--volume", "volume-name=cm:config-map-name",
-		"--env-from", "config-map:config-map-name", "--user", "1001"}
+		"--env-from", "config-map:config-map-name", "--user", "1001", "--pull-policy", "always"}
 	expectedPodSpec := corev1.PodSpec{
 		Containers: []corev1.Container{
 			{
-				Image:   "repo/user/imageID:tag",
-				Command: []string{"/app/start"},
-				Args:    []string{"myArg1"},
+				Image:           "repo/user/imageID:tag",
+				ImagePullPolicy: "Always",
+				Command:         []string{"/app/start"},
+				Args:            []string{"myArg1"},
 				Ports: []corev1.ContainerPort{
 					{
 						ContainerPort: 8080,

pkg/serving/config_changes.go

@@ -22,9 +22,7 @@ import (
 	"strings"
 	"time"
 
-	v1 "k8s.io/api/core/v1"
 	"knative.dev/client/pkg/kn/flags"
-	"knative.dev/client/pkg/util"
 	"knative.dev/pkg/ptr"
 	"knative.dev/serving/pkg/apis/autoscaling"
 	servingconfig "knative.dev/serving/pkg/apis/config"
@@ -216,32 +214,6 @@ func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec,
 	return UpdateRevisionTemplateAnnotations(template, map[string]string{annotation: value}, []string{})
 }
 
-// UpdateImagePullPolicy updates the pull policy for the given revision template
-func UpdateImagePullPolicy(template *servingv1.RevisionTemplateSpec, imagePullPolicy string) error {
-	idx := ContainerIndexOfRevisionSpec(&template.Spec)
-	if idx < 0 {
-		return fmt.Errorf("no container found in spec")
-	}
-	if !isValidPullPolicy(imagePullPolicy) {
-		return fmt.Errorf("invalid --pull-policy %s. Valid arguments (case insensitive): Always | Never | IfNotPresent", imagePullPolicy)
-	}
-	template.Spec.Containers[idx].ImagePullPolicy = actualPolicy(imagePullPolicy)
-	return nil
-}
-
-func actualPolicy(policy string) v1.PullPolicy {
-	var ret v1.PullPolicy
-	switch strings.ToLower(policy) {
-	case "always":
-		ret = v1.PullAlways
-	case "ifnotpresent":
-		ret = v1.PullIfNotPresent
-	case "never":
-		ret = v1.PullNever
-	}
-	return ret
-}
-
 // =======================================================================================
 
 func updateAnnotations(annotations map[string]string, toUpdate map[string]string, toRemove []string) error {
@@ -253,8 +225,3 @@ func updateAnnotations(annotations map[string]string, toUpdate map[string]string
 	}
 	return nil
 }
-
-func isValidPullPolicy(policy string) bool {
-	validPolicies := []string{string(v1.PullAlways), string(v1.PullNever), string(v1.PullIfNotPresent)}
-	return util.SliceContainsIgnoreCase(validPolicies, policy)
-}