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

client.go

@@ -674,7 +674,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
 	}
 
@@ -687,14 +687,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.
@@ -890,29 +905,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,
@@ -923,16 +949,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
 	}
 
@@ -942,14 +959,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.
@@ -959,6 +972,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_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.Create(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/delete_test.go

@@ -21,8 +21,8 @@ func TestDelete_Namespace(t *testing.T) {
 	t.Setenv("KUBECONFIG", filepath.Join(cwd(), "nonexistent"))
 	t.Setenv("KUBERNETES_SERVICE_HOST", "")
 	cmd := NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) {
-		if cc.Namespace != "default" {
-			t.Fatalf("expected 'default', got '%v'", cc.Namespace)
+		if cc.Namespace != "" {
+			t.Fatalf("expected '', got '%v'", cc.Namespace)
 		}
 		return fn.New(), func() {}
 	})

cmd/deploy.go

@@ -240,6 +240,11 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 	// Informative non-error messages regarding the final deployment request
 	printDeployMessages(cmd.OutOrStdout(), cfg)
 
+	// Save the function which has now been updated with flags/config
+	if err = f.Write(); err != nil { // TODO: remove when client API uses 'f'
+		return
+	}
+
 	// Client
 	// Concrete implementations (ex builder) vary  based on final effective cfg.
 	var builder fn.Builder
@@ -268,10 +273,10 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 		if f, err = client.RunPipeline(cmd.Context(), f); err != nil {
 			return
 		}
+		// TODO: remote deployments currently have no way to update the function
+		// state with values generated during the deployment process such as the
+		// ImageDigest (from pusing) or the deployed namespace (on deploy)a.
 	} else {
-		if err = f.Write(); err != nil { // TODO: remove when client API uses 'f'
-			return
-		}
 		if shouldBuild(cfg.Build, f, client) { // --build or "auto" with FS changes
 			if err = client.Build(cmd.Context(), f.Root); err != nil {
 				return
@@ -303,7 +308,7 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 // deployConfig.Validate
 func shouldBuild(buildCfg string, f fn.Function, client *fn.Client) bool {
 	if buildCfg == "auto" {
-		return !client.Built(f.Root) // first build or modified filesystem
+		return !fn.Built(f.Root) // first build or modified filesystem
 	}
 	build, _ := strconv.ParseBool(buildCfg)
 	return build
@@ -752,10 +757,11 @@ func printDeployMessages(out io.Writer, cfg deployConfig) {
 
 	// Namespace Changing
 	// -------------------
-	// If the target namespace is not active, warn because it wont be visible
-	// to other commands such as kubectl unless context namespace is switched.
+	// If the target namespace is provided but differs from active, warn because
+	// the function wont be visible to other commands such as kubectl unless
+	// context namespace is switched.
 	activeNamespace, err := k8s.GetNamespace("")
-	if err == nil && targetNamespace != activeNamespace {
+	if err == nil && targetNamespace != "" && targetNamespace != activeNamespace {
 		fmt.Fprintf(out, "Warning: namespace chosen is '%s', but currently active namespace is '%s'. Continuing with deployment to '%s'.\n", cfg.Namespace, activeNamespace, cfg.Namespace)
 	}
 }