fix(gitreceive,k8s): use the api.PullPolicy type everywhere

And don’t hard-code api.PullAlways in buildPod. Also change tests to
expose this regression.

Author: Aaron Schlesinger

pkg/gitreceive/build.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/deis/builder/pkg"
 	"github.com/deis/builder/pkg/git"
+	"github.com/deis/builder/pkg/k8s"
 	"github.com/deis/builder/pkg/storage"
 	"github.com/deis/builder/pkg/sys"
 	"github.com/deis/pkg/log"
@@ -51,6 +52,16 @@ func build(
 	builderKey,
 	rawGitSha string) error {
 
+	dockerBuilderImagePullPolicy, err := k8s.PullPolicyFromString(conf.DockerBuilderImagePullPolicy)
+	if err != nil {
+		return err
+	}
+
+	slugBuilderImagePullPolicy, err := k8s.PullPolicyFromString(conf.SlugBuilderImagePullPolicy)
+	if err != nil {
+		return nil
+	}
+
 	repo := conf.Repository
 	gitSha, err := git.NewSha(rawGitSha)
 	if err != nil {
@@ -133,7 +144,7 @@ func build(
 			slugName,
 			conf.StorageType,
 			conf.DockerBuilderImage,
-			conf.DockerBuilderImagePullPolicy,
+			dockerBuilderImagePullPolicy,
 		)
 	} else {
 		buildPodName = slugBuilderPodName(appName, gitSha.Short())
@@ -147,7 +158,7 @@ func build(
 			buildPackURL,
 			conf.StorageType,
 			conf.SlugBuilderImage,
-			conf.SlugBuilderImagePullPolicy,
+			slugBuilderImagePullPolicy,
 		)
 	}
 

pkg/gitreceive/k8s_util.go

@@ -43,14 +43,14 @@ func dockerBuilderPod(
 	tarKey,
 	imageName,
 	storageType,
-	dockerBuilderImage,
-	dockerBuilderImagePullPolicy string,
+	image string,
+	pullPolicy api.PullPolicy,
 ) *api.Pod {
 
-	pod := buildPod(debug, name, namespace, dockerBuilderImagePullPolicy, env)
+	pod := buildPod(debug, name, namespace, pullPolicy, env)
 
 	pod.Spec.Containers[0].Name = dockerBuilderName
-	pod.Spec.Containers[0].Image = dockerBuilderImage
+	pod.Spec.Containers[0].Image = image
 
 	addEnvToPod(pod, tarPath, tarKey)
 	addEnvToPod(pod, "IMG_NAME", imageName)
@@ -82,14 +82,14 @@ func slugbuilderPod(
 	putKey,
 	buildpackURL,
 	storageType,
-	slugBuilderImage,
-	slugBuilderImagePullPolicy string,
+	image string,
+	pullPolicy api.PullPolicy,
 ) *api.Pod {
 
-	pod := buildPod(debug, name, namespace, slugBuilderImagePullPolicy, env)
+	pod := buildPod(debug, name, namespace, pullPolicy, env)
 
 	pod.Spec.Containers[0].Name = slugBuilderName
-	pod.Spec.Containers[0].Image = slugBuilderImage
+	pod.Spec.Containers[0].Image = image
 
 	addEnvToPod(pod, tarPath, tarKey)
 	addEnvToPod(pod, putPath, putKey)
@@ -105,16 +105,16 @@ func slugbuilderPod(
 func buildPod(
 	debug bool,
 	name,
-	namespace,
-	imagePullPolicy string,
+	namespace string,
+	pullPolicy api.PullPolicy,
 	env map[string]interface{}) api.Pod {
 
 	pod := api.Pod{
 		Spec: api.PodSpec{
 			RestartPolicy: api.RestartPolicyNever,
 			Containers: []api.Container{
 				api.Container{
-					ImagePullPolicy: api.PullAlways,
+					ImagePullPolicy: pullPolicy,
 				},
 			},
 			Volumes: []api.Volume{},

pkg/gitreceive/k8s_util_test.go

@@ -64,6 +64,8 @@ func TestBuildPod(t *testing.T) {
 		{true, "test", "default", emptyEnv, "tar", "put-url", "buildpack", "", api.PullAlways, ""},
 		{true, "test", "default", env, "tar", "put-url", "buildpack", "", api.PullAlways, ""},
 		{true, "test", "default", env, "tar", "put-url", "buildpack", "customimage", api.PullAlways, ""},
+		{true, "test", "default", env, "tar", "put-url", "buildpack", "customimage", api.PullIfNotPresent, ""},
+		{true, "test", "default", env, "tar", "put-url", "buildpack", "customimage", api.PullNever, ""},
 	}
 
 	for _, build := range slugBuilds {
@@ -77,7 +79,7 @@ func TestBuildPod(t *testing.T) {
 			build.buildPack,
 			build.storageType,
 			build.slugBuilderImage,
-			string(build.slugBuilderImagePullPolicy),
+			build.slugBuilderImagePullPolicy,
 		)
 
 		if pod.ObjectMeta.Name != build.name {
@@ -116,6 +118,8 @@ func TestBuildPod(t *testing.T) {
 		{true, "test", "default", emptyEnv, "tar", "img", "", api.PullAlways, ""},
 		{true, "test", "default", env, "tar", "img", "", api.PullAlways, ""},
 		{true, "test", "default", env, "tar", "img", "customimage", api.PullAlways, ""},
+		{true, "test", "default", env, "tar", "img", "customimage", api.PullIfNotPresent, ""},
+		{true, "test", "default", env, "tar", "img", "customimage", api.PullNever, ""},
 	}
 
 	for _, build := range dockerBuilds {
@@ -128,7 +132,7 @@ func TestBuildPod(t *testing.T) {
 			build.imgName,
 			build.storageType,
 			build.dockerBuilderImage,
-			string(build.dockerBuilderImagePullPolicy),
+			build.dockerBuilderImagePullPolicy,
 		)
 
 		if pod.ObjectMeta.Name != build.name {

pkg/k8s/pull_policy.go

@@ -0,0 +1,36 @@
+package k8s
+
+import (
+	"fmt"
+
+	"k8s.io/kubernetes/pkg/api"
+)
+
+var (
+	emptyPullPolicy = api.PullPolicy("")
+	// ValidPullPolicies is the set of pull policies that this package considers valid
+	ValidPullPolicies = map[api.PullPolicy]struct{}{
+		api.PullAlways:       struct{}{},
+		api.PullIfNotPresent: struct{}{},
+		api.PullNever:        struct{}{},
+	}
+)
+
+// ErrInvalidPullPolicy is the error returned when trying to convert an unknown string to an api.PullPolicy
+type ErrInvalidPullPolicy struct {
+	str string
+}
+
+// Error is the error interface implementation
+func (e ErrInvalidPullPolicy) Error() string {
+	return fmt.Sprintf("%s is an invalid pull policy", e.str)
+}
+
+// PullPolicyFromString converts a string into an api.PullPolicy. returns an error if the string does not match a pull policy in ValidPullPolicies()
+func PullPolicyFromString(ppStr string) (api.PullPolicy, error) {
+	candidatePP := api.PullPolicy(ppStr)
+	if _, ok := ValidPullPolicies[candidatePP]; !ok {
+		return emptyPullPolicy, ErrInvalidPullPolicy{str: ppStr}
+	}
+	return candidatePP, nil
+}