feat: effective path (#1353)

* effective path

* function context for build builder

* code review suggestions

- fix misspelled 'precedence' throughout
- remove superfluous command execution from test
- remove debug statements
- add FUNC_PATH precedence check with short-flag in effectivePath test

* rebase and update to NewTestClient

client.go

@@ -812,7 +812,7 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error
 		<-ctx.Done()
 		c.progressListener.Stopping()
 	}()
-	// If name is provided, it takes precidence.
+	// If name is provided, it takes precedence.
 	// Otherwise load the function defined at root.
 	functionName := cfg.Name
 	if cfg.Name == "" {

client_test.go

@@ -848,7 +848,7 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) {
 }
 
 // TestClient_Remove_ByName ensures that the remover is invoked to remove the function
-// of the name provided, with precidence over a provided root path.
+// of the name provided, with precedence over a provided root path.
 func TestClient_Remove_ByName(t *testing.T) {
 	var (
 		root         = "testdata/example.com/testRemoveByName"
@@ -952,7 +952,7 @@ func TestClient_List_OutsideRoot(t *testing.T) {
 // TestClient_Deploy_Image ensures that initially the function's image
 // member has no value (not initially deployed); the value is populated
 // upon deployment with a value derived from the function's name and currently
-// effective client registry; that the value of f.Image will take precidence
+// effective client registry; that the value of f.Image will take precedence
 // over .Registry, which is used to calculate a default value for image.
 func TestClient_Deploy_Image(t *testing.T) {
 	root, rm := Mktemp(t)
@@ -998,7 +998,7 @@ func TestClient_Deploy_Image(t *testing.T) {
 		t.Fatalf("expected registry '%v', got '%v'", expected, f.Registry)
 	}
 
-	// The value of .Image always takes precidence
+	// The value of .Image always takes precedence
 	f.Image = "registry2.example.com/bob/myfunc:latest"
 	if err = f.Write(); err != nil {
 		t.Fatal(err)
@@ -1029,7 +1029,7 @@ func TestClient_Deploy_Image(t *testing.T) {
 // TestClient_Pipelines_Deploy_Image ensures that initially the function's image
 // member has no value (not initially deployed); the value is populated
 // upon pipeline run execution with a value derived from the function's name and currently
-// effective client registry; that the value of f.Image will take precidence
+// effective client registry; that the value of f.Image will take precedence
 // over .Registry, which is used to calculate a default value for image.
 func TestClient_Pipelines_Deploy_Image(t *testing.T) {
 	root, rm := Mktemp(t)
@@ -1076,7 +1076,7 @@ func TestClient_Pipelines_Deploy_Image(t *testing.T) {
 		t.Fatalf("expected registry '%v', got '%v'", expected, f.Registry)
 	}
 
-	// The value of .Image always takes precidence
+	// The value of .Image always takes precedence
 	f.Image = "registry2.example.com/bob/myfunc:latest"
 	if err = f.Write(); err != nil {
 		t.Fatal(err)
@@ -1202,7 +1202,7 @@ func TestClient_New_BuildpacksPersisted(t *testing.T) {
 // TestClient_Runtimes ensures that the total set of runtimes are returned.
 func TestClient_Runtimes(t *testing.T) {
 	// TODO: test when a specific repo override is indicated
-	// (remote repo which takes precidence over embedded and extended)
+	// (remote repo which takes precedence over embedded and extended)
 
 	client := fn.New(fn.WithRepositoriesPath("testdata/repositories"))
 

cmd/build.go

@@ -57,7 +57,27 @@ and the image name is stored in the configuration file.
 		fmt.Fprintf(cmd.OutOrStdout(), "error loading config at '%v'. %v\n", config.File(), err)
 	}
 
-	cmd.Flags().StringP("builder", "b", cfg.Builder, fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", KnownBuilders()))
+	// Function Context
+	// Load the value of the builder from the function at the effective path
+	// if it exists.
+	// This value takes precedence over the global config value, which encapsulates
+	// both the static default (builders.default) and any extant user setting in
+	// their global config file.
+	// The defaulting of path to cwd() can be removed when the open PR #
+	// is merged which updates the system to treat an empty path as indicating
+	// CWD by default.
+	builder := cfg.Builder
+	path := effectivePath()
+	if path == "" {
+		path = cwd()
+	}
+	if f, err := fn.NewFunction(path); err == nil && f.Build.Builder != "" {
+		// no errors loading the function at path, and it has a builder specified:
+		// The "function with context" takes precedence determining flag defaults
+		builder = f.Build.Builder
+	}
+
+	cmd.Flags().StringP("builder", "b", builder, fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", KnownBuilders()))
 	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", cfg.Confirm, "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)")

cmd/build_test.go

@@ -8,6 +8,7 @@ import (
 	"gotest.tools/v3/assert"
 
 	fn "knative.dev/func"
+	"knative.dev/func/builders"
 	"knative.dev/func/mock"
 )
 
@@ -224,3 +225,56 @@ func TestBuild_RegistryHandling(t *testing.T) {
 		assert.Assert(t, f.Image == tc.expImage, fmt.Sprintf("Test case %d: expected image to be '"+tc.expImage+"', but got '%s'", tci, f.Image))
 	}
 }
+
+// TestBuild_FunctionContext ensures that the function contectually relevant
+// to the current command execution is loaded and used for flag defaults by
+// spot-checking the builder setting.
+func TestBuild_FunctionContext(t *testing.T) {
+	root := fromTempDirectory(t)
+
+	if err := fn.New().Create(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}); err != nil {
+		t.Fatal(err)
+	}
+
+	// Build the function explicitly setting the builder to !builders.Default
+	cmd := NewBuildCmd(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)
+	}
+
+	// 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 {
+		builder = builders.S2I
+	} else {
+		builder = builders.Pack
+	}
+
+	// Build with the other
+	cmd.SetArgs([]string{"--builder", builder})
+	if err := cmd.Execute(); err != nil {
+		t.Fatal(err)
+	}
+
+	// The function should now have the builder set to the new builder
+	f, err := fn.NewFunction(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if f.Build.Builder != builder {
+		t.Fatalf("expected function to have new builder '%v', got '%v'", builder, f.Build.Builder)
+	}
+
+	// The command default should now take into account the function when
+	// determining the flag default
+	cmd = NewBuildCmd(NewTestClient())
+	dflt = cmd.Flags().Lookup("builder").DefValue
+
+	if dflt != builder {
+		t.Fatalf("expected flag default to be function's current builder '%v', got '%v'", builder, dflt)
+	}
+}

cmd/create.go

@@ -118,7 +118,7 @@ func runCreate(cmd *cobra.Command, args []string, newClient ClientFactory) (err
 
 	// Client
 	// From environment variables, flags, arguments, and user prompts if --confirm
-	// (in increasing levels of precidence)
+	// (in increasing levels of precedence)
 	client, done := newClient(
 		ClientConfig{Verbose: cfg.Verbose},
 		fn.WithRepository(cfg.Repository))
@@ -258,7 +258,7 @@ func newCreateConfig(cmd *cobra.Command, args []string, newClient ClientFactory)
 
 	// Confirming, but noninteractive
 	// Print out the final values as a confirmation.  Only show Repository or
-	// Repositories, not both (repository takes precidence) in order to avoid
+	// Repositories, not both (repository takes precedence) in order to avoid
 	// likely confusion if both are displayed and one is empty.
 	// be removed and both displayed.
 	fmt.Printf("Path:         %v\n", cfg.Path)

cmd/deploy_test.go

@@ -628,7 +628,7 @@ func TestDeploy_Namespace(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Ensure an explicit name (a flag) is taken with highest precidence
+	// Ensure an explicit name (a flag) is taken with highest precedence
 	expectedNamespace = "flagValueNamespace"
 	cmd = NewDeployCmd(NewTestClient(fn.WithDeployer(deployer)))
 	cmd.SetArgs([]string{"--namespace", expectedNamespace})
@@ -812,7 +812,7 @@ func TestDeploy_NamespaceDefaults(t *testing.T) {
 
 // TestDeploy_NamespaceUpdateWarning ensures that, deploying a Function
 // to a new namespace issues a warning.
-// Also implicitly checks that the --namespace flag takes precidence over
+// Also implicitly checks that the --namespace flag takes precedence over
 // the namespace of a previously deployed Function.
 func TestDeploy_NamespaceUpdateWarning(t *testing.T) {
 	root := fromTempDirectory(t)

cmd/describe.go

@@ -78,7 +78,7 @@ func runDescribe(cmd *cobra.Command, args []string, newClient ClientFactory) (er
 		if !f.Initialized() {
 			return fmt.Errorf("the given path '%v' does not contain an initialized function.", cfg.Path)
 		}
-		// Use Function's Namespace with precidence
+		// Use Function's Namespace with precedence
 		//
 		// Unless the namespace flag was explicitly provided (not the default),
 		// use the function's current namespace.

cmd/invoke.go

@@ -326,7 +326,7 @@ func (c invokeConfig) prompt() (invokeConfig, error) {
 	// Prompt for the next set of values, with defaults set first by the function
 	// as it exists on disk, followed by environment variables, and finally flags.
 	// user interactive prompts therefore are the last applied, and thus highest
-	// precidence values.
+	// precedence values.
 	qs = []*survey.Question{
 		{
 			Name: "ID",

cmd/root.go

@@ -1,7 +1,9 @@
 package cmd
 
 import (
+	"flag"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -135,6 +137,30 @@ func registry() string {
 	return cfg.RegistryDefault()
 }
 
+// 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).
+func effectivePath() (path string) {
+	var (
+		env   = os.Getenv("FUNC_PATH")
+		fs    = flag.NewFlagSet("", flag.ContinueOnError)
+		long  = fs.String("path", "", "")
+		short = fs.String("p", "", "")
+	)
+	fs.SetOutput(io.Discard)
+	_ = fs.Parse(os.Args[1:])
+	if env != "" {
+		path = env
+	}
+	if *short != "" {
+		path = *short
+	}
+	if *long != "" {
+		path = *long
+	}
+	return path
+}
+
 // interactiveTerminal returns whether or not the currently attached process
 // terminal is interactive.  Used for determining whether or not to
 // interactively prompt the user to confirm default choices, etc.

cmd/root_test.go

@@ -261,6 +261,59 @@ func TestVerbose(t *testing.T) {
 	}
 }
 
+// TestRoot_effectivePath ensures that the path method returns the effective path
+// to use with the following precedence:  empty by default, then FUNC_PATH
+// environment variable, -p flag, or finally --path with the highest precedence.
+func TestRoot_effectivePath(t *testing.T) {
+
+	args := os.Args
+	t.Cleanup(func() { os.Args = args })
+
+	t.Run("default", func(t *testing.T) {
+		if effectivePath() != "" {
+			t.Fatalf("the default path should be '.', got '%v'", effectivePath())
+		}
+	})
+
+	t.Run("FUNC_PATH", func(t *testing.T) {
+		t.Setenv("FUNC_PATH", "p1")
+		if effectivePath() != "p1" {
+			t.Fatalf("the effetive path did not load the environment variable.  Expected 'p1', got '%v'", effectivePath())
+		}
+	})
+
+	t.Run("--path", func(t *testing.T) {
+		os.Args = []string{"test", "--path=p2"}
+		if effectivePath() != "p2" {
+			t.Fatalf("the effective path did not load the --path flag.  Expected 'p2', got '%v'", effectivePath())
+		}
+	})
+
+	t.Run("-p", func(t *testing.T) {
+		os.Args = []string{"test", "-p=p3"}
+		if effectivePath() != "p3" {
+			t.Fatalf("the effective path did not load the -p flag.  Expected 'p3', got '%v'", effectivePath())
+		}
+	})
+
+	t.Run("short flag precedence", func(t *testing.T) {
+		t.Setenv("FUNC_PATH", "p1")
+		os.Args = []string{"test", "-p=p3"}
+		if effectivePath() != "p3" {
+			t.Fatalf("the effective path did not load the -p flag with precedence over FUNC_PATH.  Expected 'p3', got '%v'", effectivePath())
+		}
+	})
+
+	t.Run("--path highest precedence", func(t *testing.T) {
+		t.Setenv("FUNC_PATH", "p1")
+		os.Args = []string{"test", "--path=p2", "-p=p3"}
+		if effectivePath() != "p2" {
+			t.Fatalf("the effective path did not take --path with highest precedence over -p and FUNC_PATH.  Expected 'p2', got '%v'", effectivePath())
+		}
+	})
+
+}
+
 // Helpers
 // -------
 

config/config.go

@@ -121,7 +121,7 @@ func Dir() (path string) {
 		path = filepath.Join(home, ".config", "func")
 	}
 
-	// 'XDG_CONFIG_HOME/func' takes precidence if defined
+	// 'XDG_CONFIG_HOME/func' takes precedence if defined
 	if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" {
 		path = filepath.Join(xdg, "func")
 	}

repository.go

@@ -169,7 +169,7 @@ func NewRepository(name, uri string) (r Repository, err error) {
 	if err != nil {
 		return
 	}
-	if name != "" { // If provided, the explicit name takes precidence
+	if name != "" { // If provided, the explicit name takes precedence
 		r.Name = name
 	}
 	r.Runtimes, err = repositoryRuntimes(fs, r.Name, repoConfig) // load templates grouped by runtime
@@ -355,11 +355,11 @@ func runtimeTemplates(fs Filesystem, templatesPath, repoName, runtimeName string
 // deriving a name from the URI, which if empty then falls back to the
 // statically defined default DefaultRepositoryName.
 func repositoryDefaultName(name, uri string) (string, error) {
-	// explicit name takes precidence
+	// explicit name takes precedence
 	if name != "" {
 		return name, nil
 	}
-	// URI-derived is second precidence
+	// URI-derived is second precedence
 	if uri != "" {
 		parsed, err := url.Parse(uri)
 		if err != nil {