Repository: knative-sandbox/kn-plugin-func
Pull request #1136 (2022-07-21 10:48:01)

chore: add validation for `builder`
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

<!-- Thanks for sending a pull request! -->

# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

- :broom: add validation for `builder`
- :gift: add supported types (pack, s2i) to json schema

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind enhancement


knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/1136#" title="Author self-approved">zroubalik</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)~~ [zroubalik]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#1136](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (34b497f) into [main](https://codecov.io/gh/knative-sandbox/kn-plugin-func/commit/a906b8866cb97894a676aeada9212df0e76a7ee0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (a906b88) will **increase** coverage by `0.15%`.
> The diff coverage is `83.72%`.

```diff
@@            Coverage Diff             @@
##             main    #1136      +/-   ##
==========================================
+ Coverage   45.97%   46.12%   +0.15%     
==========================================
  Files          65       66       +1     
  Lines        8701     8717      +16     
==========================================
+ Hits         4000     4021      +21     
+ Misses       4315     4310       -5     
  Partials      386      386              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [cmd/completion\_util.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2NvbXBsZXRpb25fdXRpbC5nbw==) | `0.00% <0.00%> (ø)` | |
| [function.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-ZnVuY3Rpb24uZ28=) | `58.99% <ø> (ø)` | |
| [cmd/build.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2J1aWxkLmdv) | `64.83% <77.77%> (ø)` | |
| [function\_builder.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-ZnVuY3Rpb25fYnVpbGRlci5nbw==) | `88.88% <88.88%> (ø)` | |
| [cmd/deploy.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2RlcGxveS5nbw==) | `59.75% <91.66%> (+0.49%)` | :arrow_up: |
| [buildpacks/builder.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-YnVpbGRwYWNrcy9idWlsZGVyLmdv) | `42.24% <100.00%> (ø)` | |
| [s2i/builder.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-czJpL2J1aWxkZXIuZ28=) | `64.23% <100.00%> (ø)` | |



Patch:
diff --git a/buildpacks/builder.go b/buildpacks/builder.go
index 9e081720..f22f8b2f 100644
--- a/buildpacks/builder.go
+++ b/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
 	}
diff --git a/cmd/build.go b/cmd/build.go
index 670667d4..1363f9a3 100644
--- a/cmd/build.go
+++ b/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
 	}
 
diff --git a/cmd/completion_util.go b/cmd/completion_util.go
index 26d70a53..4a0124c5 100644
--- a/cmd/completion_util.go
+++ b/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
 }
diff --git a/cmd/deploy.go b/cmd/deploy.go
index 5e0bcb6d..0ff8833f 100644
--- a/cmd/deploy.go
+++ b/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 {
diff --git a/function.go b/function.go
index 394eaccf..660fe28b 100644
--- a/function.go
+++ b/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"`
diff --git a/function_builder.go b/function_builder.go
new file mode 100644
index 00000000..38c6dc98
--- /dev/null
+++ b/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)
+}
diff --git a/s2i/builder.go b/s2i/builder.go
index 4a32cc71..cc87c479 100644
--- a/s2i/builder.go
+++ b/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
 	}
diff --git a/schema/func_yaml-schema.json b/schema/func_yaml-schema.json
index e35480eb..139440ba 100644
--- a/schema/func_yaml-schema.json
+++ b/schema/func_yaml-schema.json
@@ -89,6 +89,10 @@
 					"type": "array"
 				},
 				"builder": {
+					"enum": [
+						"pack",
+						"s2i"
+					],
 					"type": "string"
 				},
 				"volumes": {


Test patch:
diff --git a/function_builder_unit_test.go b/function_builder_unit_test.go
new file mode 100644
index 00000000..3a681bed
--- /dev/null
+++ b/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)
+			}
+		})
+	}
+}