chore: add validation for `builder` (#1136)

* chore: add validation for `builder`

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

* address feedback

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

buildpacks/builder.go

@@ -182,7 +182,7 @@ func BuilderImage(f fn.Function) (string, error) {
 		return "", ErrRuntimeRequired{}
 	}
 
-	v, ok := f.BuilderImages["pack"]
+	v, ok := f.BuilderImages[fn.BuilderPack]
 	if ok {
 		return v, nil
 	}

cmd/build.go

@@ -49,7 +49,7 @@ and the image name is stored in the configuration file.
 		PreRunE:    bindEnv("image", "path", "builder", "registry", "confirm", "push", "builder-image", "platform"),
 	}
 
-	cmd.Flags().StringP("builder", "b", "pack", "build strategy to use when creating the underlying image. Currently supported build strategies are 'pack' and 's2i'.")
+	cmd.Flags().StringP("builder", "b", fn.DefaultBuilder, fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", fn.SupportedBuilders()))
 	cmd.Flags().StringP("builder-image", "", "", "builder image, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml (as 'builder' field) for subsequent builds. ($FUNC_BUILDER_IMAGE)")
 	cmd.Flags().BoolP("confirm", "c", false, "Prompt to confirm all configuration options (Env: $FUNC_CONFIRM)")
 	cmd.Flags().StringP("image", "i", "", "Full image name in the form [registry]/[namespace]/[name]:[tag] (optional). This option takes precedence over --registry (Env: $FUNC_IMAGE)")
@@ -58,7 +58,7 @@ and the image name is stored in the configuration file.
 	cmd.Flags().StringP("platform", "", "", "Target platform to build (e.g. linux/amd64).")
 	setPathFlag(cmd)
 
-	if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuildStrategyList); err != nil {
+	if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuildersList); err != nil {
 		fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err)
 	}
 
@@ -150,23 +150,22 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
 	} else {
 		config.Builder = function.Builder
 	}
-
-	// All set, let's write changes in the config to the disk
-	err = function.Write()
-	if err != nil {
-		return
+	if err = fn.ValidateBuilder(config.Builder); err != nil {
+		return err
 	}
-
-	if config.Builder == "pack" {
+	if config.Builder == fn.BuilderPack {
 		if config.Platform != "" {
 			err = fmt.Errorf("the --platform flag works only with s2i build")
 			return
 		}
 		builder = buildpacks.NewBuilder(buildpacks.WithVerbose(config.Verbose))
-	} else if config.Builder == "s2i" {
+	} else if config.Builder == fn.BuilderS2i {
 		builder = s2i.NewBuilder(s2i.WithVerbose(config.Verbose), s2i.WithPlatform(config.Platform))
-	} else {
-		err = errors.New("unrecognized builder: valid values are: s2i, pack")
+	}
+
+	// All set, let's write changes in the config to the disk
+	err = function.Write()
+	if err != nil {
 		return
 	}
 

cmd/completion_util.go

@@ -135,14 +135,6 @@ func CompleteDeployBuildType(cmd *cobra.Command, args []string, complete string)
 	return
 }
 
-func CompleteBuildStrategyList(cmd *cobra.Command, args []string, complete string) ([]string, cobra.ShellCompDirective) {
-	if len(complete) >= 1 {
-		if strings.HasPrefix("pack", complete) {
-			return []string{"pack"}, cobra.ShellCompDirectiveNoFileComp
-		}
-		if strings.HasPrefix("s2i", complete) {
-			return []string{"s2i"}, cobra.ShellCompDirectiveNoFileComp
-		}
-	}
-	return []string{"pack", "s2i"}, cobra.ShellCompDirectiveNoFileComp
+func CompleteBuildersList(cmd *cobra.Command, args []string, complete string) ([]string, cobra.ShellCompDirective) {
+	return fn.AllBuilders(), cobra.ShellCompDirectiveNoFileComp
 }

cmd/deploy.go

@@ -62,7 +62,7 @@ that is pushed to an image registry, and finally the function's Knative service
 	cmd.Flags().StringP("git-dir", "d", "", "Directory in the repo where the function is located (Env: $FUNC_GIT_DIR)")
 	cmd.Flags().StringP("build", "b", fn.DefaultBuildType, fmt.Sprintf("Build specifies the way the function should be built. Supported types are %s (Env: $FUNC_BUILD)", fn.SupportedBuildTypes(true)))
 	// Flags shared with Build specifically related to building:
-	cmd.Flags().StringP("builder", "", "pack", "build strategy to use when creating the underlying image. Currently supported build strategies are 'pack' and 's2i'.")
+	cmd.Flags().StringP("builder", "", fn.DefaultBuilder, fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", fn.SupportedBuilders()))
 	cmd.Flags().StringP("builder-image", "", "", "builder image, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml (as 'builder' field) for subsequent builds. ($FUNC_BUILDER_IMAGE)")
 	cmd.Flags().StringP("image", "i", "", "Full image name in the form [registry]/[namespace]/[name]:[tag]@[digest]. This option takes precedence over --registry. Specifying digest is optional, but if it is given, 'build' and 'push' phases are disabled. (Env: $FUNC_IMAGE)")
 	cmd.Flags().StringP("registry", "r", GetDefaultRegistry(), "Registry + namespace part of the image to build, ex 'quay.io/myuser'.  The full image name is automatically determined based on the local directory name. If not provided the registry will be taken from func.yaml (Env: $FUNC_REGISTRY)")
@@ -74,7 +74,7 @@ that is pushed to an image registry, and finally the function's Knative service
 		fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err)
 	}
 
-	if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuildStrategyList); err != nil {
+	if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuildersList); err != nil {
 		fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err)
 	}
 
@@ -184,18 +184,19 @@ func runDeploy(cmd *cobra.Command, _ []string, newClient ClientFactory) (err err
 	} else {
 		config.Builder = function.Builder
 	}
-	if config.Builder == "pack" {
+	if err = fn.ValidateBuilder(config.Builder); err != nil {
+		return err
+	}
+	if config.Builder == fn.BuilderPack {
 		if config.Platform != "" {
 			err = fmt.Errorf("the --platform flag works only with s2i build")
 			return
 		}
 		builder = buildpacks.NewBuilder(buildpacks.WithVerbose(config.Verbose))
-	} else if config.Builder == "s2i" {
+	} else if config.Builder == fn.BuilderS2i {
 		builder = s2i.NewBuilder(s2i.WithVerbose(config.Verbose), s2i.WithPlatform(config.Platform))
-	} else {
-		err = errors.New("unrecognized builder: valid values are: s2i, pack")
-		return
 	}
+
 	// All set, let's write changes in the config to the disk
 	err = function.Write()
 	if err != nil {

function.go

@@ -76,8 +76,8 @@ type Function struct {
 	Buildpacks []string `yaml:"buildpacks"`
 
 	// Builder is the name of the subsystem that will complete the underlying
-	// build (Pack, s2i, remote pipeline, etc)
-	Builder string `yaml:"builder"`
+	// build (pack, s2i, etc)
+	Builder string `yaml:"builder" jsonschema:"enum=pack,enum=s2i"`
 
 	// List of volumes to be mounted to the function
 	Volumes []Volume `yaml:"volumes"`

function_builder.go

@@ -0,0 +1,33 @@
+package function
+
+import (
+	"fmt"
+)
+
+const (
+	BuilderPack    = "pack"
+	BuilderS2i     = "s2i"
+	DefaultBuilder = BuilderPack
+)
+
+func AllBuilders() []string {
+	return []string{BuilderPack, BuilderS2i}
+}
+
+// ErrInvalidBuilder indicates that the passed builder was invalid.
+type ErrInvalidBuilder error
+
+// ValidateBuilder validatest that the input Build type is valid for deploy command
+func ValidateBuilder(builder string) error {
+
+	if builder == BuilderPack || builder == BuilderS2i {
+		return nil
+	}
+
+	return ErrInvalidBuilder(fmt.Errorf("specified builder %q is not valid, allowed builders are %s", builder, SupportedBuilders()))
+}
+
+// SupportedBuilders prints string with supported build types
+func SupportedBuilders() string {
+	return fmt.Sprintf("%q or %q", BuilderPack, BuilderS2i)
+}

function_builder_unit_test.go

@@ -0,0 +1,45 @@
+//go:build !integration
+// +build !integration
+
+package function
+
+import (
+	"testing"
+)
+
+func Test_validateBuilder(t *testing.T) {
+	tests := []struct {
+		name      string
+		builder   string
+		wantError bool
+	}{
+		{
+			name:      "valid builder - pack",
+			builder:   "pack",
+			wantError: false,
+		},
+		{
+			name:      "valid builder - s2i",
+			builder:   "s2i",
+			wantError: false,
+		},
+		{
+			name:      "invalid builder",
+			builder:   "foo",
+			wantError: true,
+		},
+		{
+			name:      "builder not specified - invalid option",
+			wantError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateBuilder(tt.builder)
+			if tt.wantError != (err != nil) {
+				t.Errorf("ValidateBuilder() = Wanted error %v but actually got %v", tt.wantError, err)
+			}
+		})
+	}
+}

s2i/builder.go

@@ -372,7 +372,7 @@ func builderImage(f fn.Function) (string, error) {
 		return "", ErrRuntimeRequired
 	}
 
-	v, ok := f.BuilderImages["s2i"]
+	v, ok := f.BuilderImages[fn.BuilderS2i]
 	if ok {
 		return v, nil
 	}

schema/func_yaml-schema.json

@@ -89,6 +89,10 @@
 					"type": "array"
 				},
 				"builder": {
+					"enum": [
+						"pack",
+						"s2i"
+					],
 					"type": "string"
 				},
 				"volumes": {