feat: deploy command global config with function context (knative#1434)

* feat: deploy command global config function context

* fix static check errors

* fix test

* fix remote persists test

* env opaque box test

* use keyed fields (govet)

* mock deployer expansion

* ClearEnvs now in shared testing pkg

* effectivePath now uses plfags

* list directly uses config for default namespace

* Improved Namespace calculation and Caching

- Built() now a client package function
- Namespace evaluation delegeate to implementations
  This enables the func.yaml to avert dirtiness checks on rebuilds
  (caching)
- Build and Deploy update build stamp on completion
  Enables dirtiness checks to be unaffects, leading to better caching.
- CLI Namespace checks no longer require k8s env evaluation for default
- Fixed missing condition check in TestDeploy_Namespace
- Fixes some minor linter and spelling errors
- Global config does not directly set namespace - it is corrected to be
  deferred evaluation used by implementations.
- Skips writing func.yaml on f.Write if there were no modifications,
  which leads to better caching (no timestamp update means Built ==
  true)
- HasImage removed in favor of a corrected fn.Built

* linting, misspellings and docs rebuild

* warn user if git settings exist on a non-remote build

* lint error fix

* test flag unsetting persists

* allow unknown flags in preparsing of flags

* pre-submit cleanup

* update cmd to use client.Init

client.go

@@ -722,7 +722,7 @@ func (c *Client) Deploy(ctx context.Context, path string) (err error) {
 
 	// Functions must be built (have an associated image) before being deployed.
 	// Note that externally built images may be specified in the func.yaml
-	if !f.HasImage() {
+	if !Built(f.Root) {
 		return ErrNotBuilt
 	}
 
@@ -735,14 +735,29 @@ func (c *Client) Deploy(ctx context.Context, path string) (err error) {
 	// Deploy a new or Update the previously-deployed function
 	c.progressListener.Increment("⬆️  Deploying function to the cluster")
 	result, err := c.deployer.Deploy(ctx, f)
+	if err != nil {
+		fmt.Printf("deploy error: %v\n", err)
+		return
+	}
+
+	// Update the function with the namespace into which the function was
+	// deployed
+	f.Deploy.Namespace = result.Namespace
+
+	// saves namespace to function's metadata (func.yaml)
+	if err = f.Write(); err != nil {
+		return
+	}
 
 	if result.Status == Deployed {
 		c.progressListener.Increment(fmt.Sprintf("✅ Function deployed in namespace %q and exposed at URL: \n   %v", result.Namespace, result.URL))
 	} else if result.Status == Updated {
 		c.progressListener.Increment(fmt.Sprintf("✅ Function updated in namespace %q and exposed at URL: \n   %v", result.Namespace, result.URL))
 	}
 
-	return err
+	// Metadata generated from deploying (namespace) should not trigger a rebuild
+	// through a staleness check, so update the build stamp we checked earlier.
+	return updateBuildStamp(f)
 }
 
 // RunPipeline runs a Pipeline to build and deploy the function.
@@ -956,29 +971,40 @@ func (c *Client) Push(ctx context.Context, path string) (err error) {
 		return
 	}
 
-	if !f.HasImage() {
+	if !Built(f.Root) {
 		return ErrNotBuilt
 	}
 
-	imageDigest, err := c.pusher.Push(ctx, f)
-	if err != nil {
+	if f.ImageDigest, err = c.pusher.Push(ctx, f); err != nil {
 		return
 	}
 
-	// Record the Image Digest pushed.
-	f.ImageDigest = imageDigest
-	return f.Write()
+	if err = f.Write(); err != nil { // saves digest to f's metadata (func.yaml)
+		return
+	}
+
+	// Metadata generated from pushing (ImageDigest) should not trigger a rebuild
+	// through a staleness check, so update the build stamp we checked earlier.
+	return updateBuildStamp(f)
 }
 
 // Built returns true if the given path contains a function which has been
 // built without any filesystem modifications since (is not stale).
-func (c *Client) Built(path string) bool {
+func Built(path string) bool {
 	f, err := NewFunction(path)
 	if err != nil {
 		return false
 	}
 
-	// Missing a build image always means !Built (but does not satisfy staleness
+	// If there is no build stamp, it is not built.
+	// This case should be redundant with the below check for an image, but is
+	// temporarily necessary (see the long-winded caviat note below).
+	stamp := buildStamp(path)
+	if stamp == "" {
+		return false
+	}
+
+	// Missing an image name always means !Built (but does not satisfy staleness
 	// checks).
 	// NOTE: This will be updated in the future such that a build does not
 	// automatically update the function's serialized, source-controlled state,
@@ -989,16 +1015,7 @@ func (c *Client) Built(path string) bool {
 	// An example of how this bug manifests is that every rebuild of a function
 	// registers the func.yaml as being dirty for source-control purposes, when
 	// this should only happen on deploy.
-	if !f.HasImage() {
-		return false
-	}
-
-	buildstampPath := filepath.Join(path, RunDataDir, buildstamp)
-
-	// If there is no build stamp, it is also not built.
-	// This case should be redundant with the above check for an image, but is
-	// temporarily necessary (see the long-winded caviat note above).
-	if _, err := os.Stat(buildstampPath); err != nil {
+	if f.Image == "" {
 		return false
 	}
 
@@ -1008,14 +1025,10 @@ func (c *Client) Built(path string) bool {
 		fmt.Fprintf(os.Stderr, "error calculating function's fingerprint: %v\n", err)
 		return false
 	}
-	b, err := os.ReadFile(buildstampPath)
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "error reading function's fingerprint: %v\n", err)
+
+	if stamp != hash {
 		return false
 	}
-	if string(b) != hash {
-		return false // changes detected
-	}
 
 	// Function has a populated image, existing buildstamp, and the calculated
 	// fingerprint has not changed.
@@ -1025,6 +1038,20 @@ func (c *Client) Built(path string) bool {
 	return true
 }
 
+// buildStamp returns the current (last) build stamp for the function
+// at the given path, if it can be found.
+func buildStamp(path string) string {
+	buildstampPath := filepath.Join(path, RunDataDir, buildstamp)
+	if _, err := os.Stat(buildstampPath); err != nil {
+		return ""
+	}
+	b, err := os.ReadFile(buildstampPath)
+	if err != nil {
+		return ""
+	}
+	return string(b)
+}
+
 // fingerprint returns a hash of the filenames and modification timestamps of
 // the files within a function's root.
 func fingerprint(f Function) (string, error) {

client_int_test.go

@@ -37,7 +37,6 @@ import (
 // ## Cluster Cleanup
 //
 // The test cluster and most resources can be removed with:
-
 //   ./hack/delete.sh
 //
 // NOTE: Downloaded images are not removed.

client_test.go

@@ -1530,23 +1530,23 @@ func TestClient_BuiltStamps(t *testing.T) {
 	client := fn.New(fn.WithBuilder(builder), fn.WithRegistry(TestRegistry))
 
 	// paths that do not contain a function are !Built - Degenerate case
-	if client.Built(root) {
+	if fn.Built(root) {
 		t.Fatal("path not containing a function returned as being built")
 	}
 
 	// a freshly-created function should be !Built
 	if err := client.Init(fn.Function{Runtime: TestRuntime, Root: root}); err != nil {
 		t.Fatal(err)
 	}
-	if client.Built(root) {
+	if fn.Built(root) {
 		t.Fatal("newly created function returned Built==true")
 	}
 
 	// a function which was successfully built should return as being Built
 	if err := client.Build(context.Background(), root); err != nil {
 		t.Fatal(err)
 	}
-	if !client.Built(root) {
+	if !fn.Built(root) {
 		t.Fatal("freshly built function should return Built==true")
 	}
 }
@@ -1597,7 +1597,7 @@ func TestClient_BuiltDetects(t *testing.T) {
 	}
 
 	// Prior to a filesystem edit, it will be Built.
-	if !client.Built(root) {
+	if !fn.Built(root) {
 		t.Fatal("freshly built function reported Built==false (1)")
 	}
 
@@ -1612,15 +1612,15 @@ func TestClient_BuiltDetects(t *testing.T) {
 	// Release thread and wait to ensure that the clock advances even in constrained CI environments
 	time.Sleep(100 * time.Millisecond)
 
-	if client.Built(root) {
+	if fn.Built(root) {
 		t.Fatal("client did not detect file timestamp change as indicating build staleness")
 	}
 
 	// Build and double-check Built has been reset
 	if err := client.Build(ctx, root); err != nil {
 		t.Fatal(err)
 	}
-	if !client.Built(root) {
+	if !fn.Built(root) {
 		t.Fatal("freshly built function reported Built==false (2)")
 	}
 
@@ -1632,15 +1632,15 @@ func TestClient_BuiltDetects(t *testing.T) {
 	f.Close()
 
 	// The system should now detect the function is stale
-	if client.Built(root) {
+	if fn.Built(root) {
 		t.Fatal("client did not detect an added file as indicating build staleness")
 	}
 
 	// Build and double-check Built has been reset
 	if err := client.Build(ctx, root); err != nil {
 		t.Fatal(err)
 	}
-	if !client.Built(root) {
+	if !fn.Built(root) {
 		t.Fatal("freshly built function reported Built==false (3)")
 	}
 
@@ -1649,7 +1649,7 @@ func TestClient_BuiltDetects(t *testing.T) {
 	if err := os.Remove(filepath.Join(root, testfile)); err != nil {
 		t.Fatal(err)
 	}
-	if client.Built(root) {
+	if fn.Built(root) {
 		t.Fatal("client did not detect a removed file as indicating build staleness")
 	}
 }

cmd/build.go

@@ -94,11 +94,11 @@ EXAMPLES
 	// 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,
-		fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", KnownBuilders()))
+		fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s. (Env: $FUNC_BUILDER)", KnownBuilders()))
 	cmd.Flags().BoolP("confirm", "c", cfg.Confirm,
 		"Prompt to confirm all configuration options (Env: $FUNC_CONFIRM)")
 	cmd.Flags().StringP("registry", "r", cfg.Registry,
-		"Registry + namespace part of the image to build, ex 'quay.io/myuser'.  The full image name is automatically determined (Env: $FUNC_REGISTRY)")
+		"Container registry + registry namespace. (ex 'ghcr.io/myuser').  The full image name is automatically determined using this along with function name. (Env: $FUNC_REGISTRY)")
 
 	// Function-Context Flags:
 	// Options whose value is available on the function with context only
@@ -130,25 +130,26 @@ EXAMPLES
 }
 
 func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err error) {
-	if err = config.CreatePaths(); err != nil {
-		return // see docker/creds potential mutation of auth.json
+	var (
+		cfg buildConfig
+		f   fn.Function
+	)
+	if err = config.CreatePaths(); err != nil { // for possible auth.json usage
+		return
 	}
-
-	cfg, err := newBuildConfig().Prompt()
-	if err != nil {
+	if cfg, err = newBuildConfig().Prompt(); err != nil {
 		return
 	}
-
 	if err = cfg.Validate(); err != nil {
 		return
 	}
-
-	f, err := fn.NewFunction(cfg.Path)
-	if err != nil {
+	if f, err = fn.NewFunction(cfg.Path); err != nil {
 		return
 	}
 	f = cfg.Configure(f) // Updates f at path to include build request values
 
+	// TODO: this logic is duplicated with runDeploy.  Shouild be in buildConfig
+	// constructor.
 	// Checks if there is a difference between defined registry and its value used as a prefix in the image tag
 	// In case of a mismatch a new image tag is created and used for build
 	// Do not react if image tag has been changed outside configuration
@@ -249,7 +250,7 @@ func newBuildConfig() buildConfig {
 
 // Configure the given function.  Updates a function struct with all
 // configurable values.  Note that buildConfig already includes function's
-// current values, as they were passed through vi flag defaults, so overwriting
+// current values, as they were passed through via flag defaults, so overwriting
 // is a noop.
 func (c buildConfig) Configure(f fn.Function) fn.Function {
 	f = c.Global.Configure(f)