Cleanup: Remove container flag & make the builder dynamic (var II)

cmd/build.go

@@ -98,7 +98,7 @@ EXAMPLES
 	// Globally-Configurable Flags:
 	// Options whose value may be defined globally may also exist on the
 	// contextually relevant function; sets are flattened above via cfg.Apply(f)
-	cmd.Flags().StringP("builder", "b", cfg.Builder,
+	cmd.Flags().StringP("builder", "b", defaultBuilder(f),
 		fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s. ($FUNC_BUILDER)", KnownBuilders()))
 	cmd.Flags().StringP("registry", "r", cfg.Registry,
 		"Container registry + registry namespace. (ex 'ghcr.io/myuser').  The full image name is automatically determined using this along with function name. ($FUNC_REGISTRY)")

cmd/deploy.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -155,8 +156,8 @@ EXAMPLES
 	// Globally-Configurable Flags:
 	// Options whose value may be defined globally may also exist on the
 	// contextually relevant function; but sets are flattened via cfg.Apply(f)
-	cmd.Flags().StringP("builder", "b", cfg.Builder,
-		fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s.", KnownBuilders()))
+	cmd.Flags().StringP("builder", "b", defaultBuilder(f),
+		fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s. Defaults to 'host' for python/go, otherwise '%s'", KnownBuilders(), builders.Default))
 	cmd.Flags().StringP("registry", "r", cfg.Registry,
 		"Container registry + registry namespace. (ex 'ghcr.io/myuser').  The full image name is automatically determined using this along with function name. ($FUNC_REGISTRY)")
 	cmd.Flags().Bool("registry-insecure", cfg.RegistryInsecure, "Skip TLS certificate verification when communicating in HTTPS with the registry ($FUNC_REGISTRY_INSECURE)")
@@ -328,18 +329,14 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 			if err != nil {
 				return
 			}
-			// image is valid and undigested
-			if !digested {
-				f.Deploy.Image = cfg.Image
-			}
+			// image is valid (no error); just assign here, both digested and
+			// undigested images are valid
+			f.Deploy.Image = cfg.Image
 		}
 
-		// If user provided --image with digest, they are requesting that specific
-		// image to be used which means building phase should be skipped and image
-		// should be deployed as is
-		if digested {
-			f.Deploy.Image = cfg.Image
-		} else {
+		// if user provided an undigested image or didnt provide one at all, use
+		// build route.
+		if !digested {
 			// NOT digested, build & push the Function unless specified otherwise
 			if f, justBuilt, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil {
 				return
@@ -430,10 +427,8 @@ func NewRegistryValidator(path string) survey.Validator {
 // ValidateBuilder ensures that the given builder is one that the CLI
 // knows how to instantiate, returning a builkder.ErrUnknownBuilder otherwise.
 func ValidateBuilder(name string) (err error) {
-	for _, known := range KnownBuilders() {
-		if name == known {
-			return
-		}
+	if slices.Contains(KnownBuilders(), name) {
+		return
 	}
 	return builders.ErrUnknownBuilder{Name: name, Known: KnownBuilders()}
 }
@@ -729,6 +724,11 @@ func (c deployConfig) Validate(cmd *cobra.Command) (err error) {
 		return errors.New("git settings (--git-url --git-dir and --git-branch) are only applicable when triggering remote deployments (--remote)")
 	}
 
+	// dont try to build remotely with the host builder
+	if c.Remote && c.Builder == "host" {
+		return errors.New("cannot deploy remotely using the host builder. Please choose different --builder")
+	}
+
 	// Git URL can contain at maximum one '#'
 	urlParts := strings.Split(c.GitURL, "#")
 	if len(urlParts) > 2 {

cmd/deploy_test.go

@@ -17,6 +17,7 @@ import (
 	fn "knative.dev/func/pkg/functions"
 	"knative.dev/func/pkg/k8s"
 	"knative.dev/func/pkg/mock"
+	"knative.dev/func/pkg/oci"
 	. "knative.dev/func/pkg/testing"
 )
 
@@ -230,29 +231,10 @@ func testConfigPrecedence(cmdFn commandConstructor, t *testing.T) {
 		clientFn = NewTestClient(fn.WithBuilder(builder))
 	)
 
-	// Ensure static default applied via 'builder'
-	// (a degenerate case, mostly just ensuring config values are not wiped to a
-	// zero value struct, etc)
+	// Ensure Global Config applied via config in ./testdata
 	root := FromTempDirectory(t)
 	t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global
 	f := fn.Function{Runtime: "go", Root: root, Name: "f"}
-	if f, err = fn.New().Init(f); err != nil {
-		t.Fatal(err)
-	}
-	if err := cmdFn(clientFn).Execute(); err != nil {
-		t.Fatal(err)
-	}
-	if f, err = fn.NewFunction(root); err != nil {
-		t.Fatal(err)
-	}
-	if f.Build.Builder != builders.Default {
-		t.Fatalf("expected static default builder '%v', got '%v'", builders.Default, f.Build.Builder)
-	}
-
-	// Ensure Global Config applied via config in ./testdata
-	root = FromTempDirectory(t)
-	t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global
-	f = fn.Function{Runtime: "go", Root: root, Name: "f"}
 	f, err = fn.New().Init(f)
 	if err != nil {
 		t.Fatal(err)
@@ -469,17 +451,24 @@ func testFunctionContext(cmdFn commandConstructor, t *testing.T) {
 
 	// Build the function explicitly setting the builder to !builders.Default
 	cmd := cmdFn(NewTestClient())
-	dflt := cmd.Flags().Lookup("builder").DefValue
 
-	// The initial default value should be builders.Default (see global config)
-	if dflt != builders.Default {
-		t.Fatalf("expected flag default value '%v', got '%v'", builders.Default, dflt)
+	dflt := defaultBuilder(f)
+	// The initial default value should be host for oci-enabled languages or builders.Default
+	// Includes midstream check so that S2I is not overwriten.
+	expectedDefault := ""
+	if oci.IsSupported(f.Runtime) && builders.Default != builders.S2I {
+		expectedDefault = builders.Host
+	} else {
+		expectedDefault = builders.Default
+	}
+	if dflt != expectedDefault {
+		t.Fatalf("expected default value '%v', got '%v'", expectedDefault, dflt)
 	}
 
 	// Choose the value that is not the default
 	// We must calculate this because downstream changes the default via patches.
 	var builder string
-	if builders.Default == builders.Pack {
+	if expectedDefault == builders.Pack {
 		builder = builders.S2I
 	} else {
 		builder = builders.Pack
@@ -503,7 +492,7 @@ func testFunctionContext(cmdFn commandConstructor, t *testing.T) {
 	// The command default should now take into account the function when
 	// determining the flag default
 	cmd = cmdFn(NewTestClient())
-	dflt = cmd.Flags().Lookup("builder").DefValue
+	dflt = defaultBuilder(f)
 
 	if dflt != builder {
 		t.Fatalf("expected flag default to be function's current builder '%v', got '%v'", builder, dflt)
@@ -532,7 +521,7 @@ func TestDeploy_GitArgsPersist(t *testing.T) {
 		fn.WithPipelinesProvider(mock.NewPipelinesProvider()),
 		fn.WithRegistry(TestRegistry),
 	))
-	cmd.SetArgs([]string{"--remote", "--git-url=" + url, "--git-branch=" + branch, "--git-dir=" + dir, "."})
+	cmd.SetArgs([]string{"--builder=pack", "--remote", "--git-url=" + url, "--git-branch=" + branch, "--git-dir=" + dir, "."})
 	if err := cmd.Execute(); err != nil {
 		t.Fatal(err)
 	}
@@ -564,7 +553,8 @@ func TestDeploy_GitArgsUsed(t *testing.T) {
 		dir    = "function"
 	)
 	// Create a new Function in the temp dir
-	_, err := fn.New().Init(fn.Function{Runtime: "go", Root: root})
+	f := fn.Function{Runtime: "go", Root: root, Build: fn.BuildSpec{Builder: "pack"}}
+	_, err := fn.New().Init(f)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -601,8 +591,8 @@ func TestDeploy_GitArgsUsed(t *testing.T) {
 // in the URL is equivalent to providing --git-branch
 func TestDeploy_GitURLBranch(t *testing.T) {
 	root := FromTempDirectory(t)
-
-	f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root})
+	ff := fn.Function{Runtime: "go", Root: root, Build: fn.BuildSpec{Builder: "pack"}}
+	f, err := fn.New().Init(ff)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1288,6 +1278,7 @@ func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.T) {
 		Runtime:  "go",
 		Root:     root,
 		Registry: TestRegistry,
+		Build:    fn.BuildSpec{Builder: "pack"},
 	}
 	f, err := fn.New().Init(f)
 	if err != nil {
@@ -1592,7 +1583,8 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) {
 			root := FromTempDirectory(t)
 
 			// Create a new Function in the temp directory
-			if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil {
+			f := fn.Function{Runtime: "go", Root: root, Build: fn.BuildSpec{Builder: "pack"}}
+			if _, err := fn.New().Init(f); err != nil {
 				t.Fatal(err)
 			}
 
@@ -1774,7 +1766,7 @@ func TestDeploy_UnsetFlag(t *testing.T) {
 	root := FromTempDirectory(t)
 
 	// Create a function
-	f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}
+	f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry, Build: fn.BuildSpec{Builder: "pack"}}
 	f, err := fn.New().Init(f)
 	if err != nil {
 		t.Fatal(err)

cmd/root.go

@@ -16,9 +16,11 @@ import (
 	"knative.dev/client/pkg/util"
 
 	"knative.dev/func/cmd/templates"
+	"knative.dev/func/pkg/builders"
 	"knative.dev/func/pkg/config"
 	fn "knative.dev/func/pkg/functions"
 	"knative.dev/func/pkg/k8s"
+	"knative.dev/func/pkg/oci"
 )
 
 // DefaultVersion when building source directly (bypassing the Makefile)
@@ -143,6 +145,38 @@ func registry() string {
 	return cfg.RegistryDefault()
 }
 
+// determines the default builder as 'flag default'
+// takes into account function context (if already built)
+// otherwise, will use dynamic defaulting - use 'host' for specific runtimes
+func defaultBuilder(f fn.Function) string {
+	// using function context
+	if f.Build.Builder != "" {
+		return f.Build.Builder
+	}
+
+	// global config file
+	cfg, err := config.NewDefault()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "Error loading config file at %v, using defaults", config.File())
+	}
+	if cfg.Builder != "" {
+		return cfg.Builder
+	}
+
+	// midstream should not set host builder as default
+	if builders.Default == builders.S2I {
+		return builders.S2I
+	}
+
+	// if oci enabled runtime
+	if oci.IsSupported(f.Runtime) {
+		return builders.Host
+	}
+
+	// static default
+	return builders.Default
+}
+
 // effectivePath to use is that which was provided by --path or FUNC_PATH.
 // Manually parses flags such that this can be used during (cobra/viper) flag
 // definition (prior to parsing).