feat: on run, build only when filesystem changed (#1031)

* feat: detect built image staleness and utilize on run command

* write directly to sha256 rather than buffer

* client test cleanup

* remove superfluous test println

* ensure runtime dir

* close file opened when testing

* fix typos

* typos and missing test case

client.go

@@ -2,9 +2,11 @@ package function
 
 import (
 	"context"
+	"crypto/sha256"
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -36,6 +38,15 @@ const (
 
 	// DefaultBuildType is the default build type for a Function
 	DefaultBuildType = BuildTypeLocal
+
+	// RunDataDir holds transient runtime metadata
+	// By default it is excluded from source control.
+	RunDataDir = ".func"
+
+	// buildstamp is the name of the file within the run data directory whose
+	// existence indicates the Function has been built, and whose content is
+	// a fingerprint of the filesystem at the time of the build.
+	buildstamp = "built"
 )
 
 // Client for managing Function instances.
@@ -538,7 +549,7 @@ func (c *Client) Create(cfg Function) (err error) {
 	f := NewFunctionWith(cfg)
 
 	// Create a .func diretory which is also added to a .gitignore
-	if err = createRuntimeDir(f); err != nil {
+	if err = ensureRuntimeDir(f); err != nil {
 		return
 	}
 
@@ -560,12 +571,31 @@ func (c *Client) Create(cfg Function) (err error) {
 	return
 }
 
-// createRuntimeDir creates a .func directory in the root of the given
+// Tag the Function as having been built
+// This is locally-scoped data, only indicating there presumably exists
+// a container image in the cache of the the configured builder, thus this info
+// is placed in a .func (non-source controlled) local metadata directory, which
+// is not stritly required to exist, so it is created if needed.
+func updateBuildStamp(f Function) (err error) {
+	if err = ensureRuntimeDir(f); err != nil {
+		return err
+	}
+	hash, err := fingerprint(f)
+	if err != nil {
+		return err
+	}
+	if err = os.WriteFile(filepath.Join(f.Root, RunDataDir, buildstamp), []byte(hash), os.ModePerm); err != nil {
+		return err
+	}
+	return
+}
+
+// ensureRuntimeDir creates a .func directory in the root of the given
 // Function which is also registered as ignored in .gitignore
 // TODO: Mutate extant .gitignore file if it exists rather than failing
 // if present (see contentious files in function.go), such that a user
 // can `git init` a directory prior to `func init` in the same directory).
-func createRuntimeDir(f Function) error {
+func ensureRuntimeDir(f Function) error {
 	if err := os.MkdirAll(filepath.Join(f.Root, RunDataDir), os.ModePerm); err != nil {
 		return err
 	}
@@ -611,6 +641,11 @@ func (c *Client) Build(ctx context.Context, path string) (err error) {
 		return
 	}
 
+	// Tag the function as having been built
+	if err = updateBuildStamp(f); err != nil {
+		return
+	}
+
 	// TODO: create a status structure and return it here for optional
 	// use by the cli for user echo (rather than rely on verbose mode here)
 	message := fmt.Sprintf("🙌 Function image built: %v", f.Image)
@@ -663,7 +698,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.Built() {
+	if !f.HasImage() {
 		return ErrNotBuilt
 	}
 
@@ -859,7 +894,7 @@ func (c *Client) Push(ctx context.Context, path string) (err error) {
 		return
 	}
 
-	if !f.Built() {
+	if !f.HasImage() {
 		return ErrNotBuilt
 	}
 
@@ -873,6 +908,79 @@ func (c *Client) Push(ctx context.Context, path string) (err error) {
 	return f.Write()
 }
 
+// Built returns true if the given path contains a Function which has been
+// built without any filesystem modificaitons since (is not stale).
+func (c *Client) 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
+	// checks).
+	// NOTE: This will be updated in the future such that a build does not
+	// automatically update the Function's serialized, source-controlled state,
+	// because merely building does not indicate the Function has changed, but
+	// rather that field should be populated on deploy.  I.e. the Image name
+	// and image stamp should reside as transient data in .func until such time
+	// as the given image has been deployed.
+	// 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 {
+		return false
+	}
+
+	// Calculate the Function's Filesystem hash and see if it has changed.
+	hash, err := fingerprint(f)
+	if err != nil {
+		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)
+		return false
+	}
+	if string(b) != hash {
+		return false // changes detected
+	}
+
+	// Function has a populated image, existing buildstamp, and the calculated
+	// fingerprint has not changed.
+	// It's a pretty good chance the thing doesn't need to be rebuilt, though
+	// of course filesystem racing conditions do exist, including both direct
+	// source code modifications or changes to the image cache.
+	return true
+}
+
+// fingerprint returns a hash of the filenames and modification timestamps of
+// the files within a Function's root.
+func fingerprint(f Function) (string, error) {
+	h := sha256.New()
+	err := filepath.Walk(f.Root, func(path string, info fs.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		// Always ignore .func, .git (TODO: .funcignore)
+		if info.IsDir() && (info.Name() == RunDataDir || info.Name() == ".git") {
+			return filepath.SkipDir
+		}
+		fmt.Fprintf(h, "%v:%v:", path, info.ModTime().UnixNano())
+		return nil
+	})
+	return fmt.Sprintf("%x", h.Sum(nil)), err
+}
+
 // DEFAULTS
 // ---------
 

client_test.go

@@ -362,7 +362,6 @@ func TestClient_New_RegistryRequired(t *testing.T) {
 	if err = client.New(context.Background(), fn.Function{Root: root}); err == nil {
 		t.Fatal("did not receive expected error creating a Function without specifying Registry")
 	}
-	fmt.Println(err)
 }
 
 // TestClient_New_ImageNameDerived ensures that the full image (tag) of the resultant OCI
@@ -1288,3 +1287,106 @@ func TestClient_Instances(t *testing.T) {
 		t.Fatalf("Expected endpoint '%v', got '%v'", expectedEndpoint, instance.Route)
 	}
 }
+
+// TestClient_BuiltStamps ensures that the client creates and considers a
+// buildstamp on build which reports whether or not a given path contains a built
+// Function.
+func TestClient_BuiltStamps(t *testing.T) {
+	root, rm := Mktemp(t)
+	defer rm()
+	builder := mock.NewBuilder()
+	client := fn.New(fn.WithBuilder(builder), fn.WithRegistry(TestRegistry))
+
+	// paths that do not contain a Function are !Built - Degenerate case
+	if client.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) {
+		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) {
+		t.Fatal("freshly built Function should return Built==true")
+	}
+}
+
+// TestClient_BuiltDetects ensures that the client's Built command detects
+// filesystem changes as indicating the Function is no longer Built (aka stale)
+// This includes modifying timestamps, removing or adding files.
+func TestClient_BuiltDetects(t *testing.T) {
+	var (
+		ctx      = context.Background()
+		builder  = mock.NewBuilder()
+		client   = fn.New(fn.WithBuilder(builder), fn.WithRegistry(TestRegistry))
+		testfile = "example.go"
+		root, rm = Mktemp(t)
+	)
+	defer rm()
+
+	// Create and build a Function
+	if err := client.Create(fn.Function{Runtime: TestRuntime, Root: root}); err != nil {
+		t.Fatal(err)
+	}
+	if err := client.Build(ctx, root); err != nil {
+		t.Fatal(err)
+	}
+
+	// Prior to a filesystem edit, it will be Built.
+	if !client.Built(root) {
+		t.Fatal("feshly built Function reported Built==false (1)")
+	}
+
+	// Edit the filesystem by touching a file (updating modified timestamp)
+	if err := os.Chtimes(filepath.Join(root, "func.yaml"), time.Now(), time.Now()); err != nil {
+		fmt.Println(err)
+	}
+	if client.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) {
+		t.Fatal("freshly built Function reported Built==false (2)")
+	}
+
+	// Edit the Function's filesystem by adding a file.
+	f, err := os.Create(filepath.Join(root, testfile))
+	if err != nil {
+		t.Fatal(err)
+	}
+	f.Close()
+
+	// The system should now detect the Function is stale
+	if client.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) {
+		t.Fatal("freshly built Function reported Built==false (3)")
+	}
+
+	// Remove the testfile, which should result in the client reporting that
+	// the Function is no longer Built (stale)
+	if err := os.Remove(filepath.Join(root, testfile)); err != nil {
+		t.Fatal(err)
+	}
+	if client.Built(root) {
+		t.Fatal("client did not detect a removed file as indicating build staleness")
+	}
+}

cmd/deploy.go

@@ -95,7 +95,7 @@ func runDeploy(cmd *cobra.Command, _ []string, newClient ClientFactory) (err err
 		return
 	}
 
-	function.Envs, err = mergeEnvs(function.Envs, config.EnvToUpdate, config.EnvToRemove)
+	function.Envs, _, err = mergeEnvs(function.Envs, config.EnvToUpdate, config.EnvToRemove)
 	if err != nil {
 		return
 	}

cmd/root.go

@@ -287,15 +287,18 @@ func envFromCmd(cmd *cobra.Command) (*util.OrderedMap, []string, error) {
 	return util.NewOrderedMap(), []string{}, nil
 }
 
-func mergeEnvs(envs []fn.Env, envToUpdate *util.OrderedMap, envToRemove []string) ([]fn.Env, error) {
+func mergeEnvs(envs []fn.Env, envToUpdate *util.OrderedMap, envToRemove []string) ([]fn.Env, int, error) {
 	updated := sets.NewString()
 
+	var counter int
+
 	for i := range envs {
 		if envs[i].Name != nil {
 			value, present := envToUpdate.GetString(*envs[i].Name)
 			if present {
 				envs[i].Value = &value
 				updated.Insert(*envs[i].Name)
+				counter++
 			}
 		}
 	}
@@ -306,6 +309,7 @@ func mergeEnvs(envs []fn.Env, envToUpdate *util.OrderedMap, envToRemove []string
 			n := name
 			v := value
 			envs = append(envs, fn.Env{Name: &n, Value: &v})
+			counter++
 		}
 	}
 
@@ -313,6 +317,7 @@ func mergeEnvs(envs []fn.Env, envToUpdate *util.OrderedMap, envToRemove []string
 		for i, envVar := range envs {
 			if *envVar.Name == name {
 				envs = append(envs[:i], envs[i+1:]...)
+				counter++
 				break
 			}
 		}
@@ -320,10 +325,10 @@ func mergeEnvs(envs []fn.Env, envToUpdate *util.OrderedMap, envToRemove []string
 
 	errMsg := fn.ValidateEnvs(envs)
 	if len(errMsg) > 0 {
-		return []fn.Env{}, fmt.Errorf(strings.Join(errMsg, "\n"))
+		return []fn.Env{}, 0, fmt.Errorf(strings.Join(errMsg, "\n"))
 	}
 
-	return envs, nil
+	return envs, counter, nil
 }
 
 // setPathFlag ensures common text/wording when the --path flag is used

cmd/root_test.go

@@ -146,7 +146,7 @@ func TestRoot_mergeEnvMaps(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := mergeEnvs(tt.args.envs, tt.args.toUpdate, tt.args.toRemove)
+			got, _, err := mergeEnvs(tt.args.envs, tt.args.toUpdate, tt.args.toRemove)
 			if err != nil {
 				t.Errorf("mergeEnvs() for initial vars %v and toUpdate %v and toRemove %v got error %v",
 					tt.args.envs, tt.args.toUpdate, tt.args.toRemove, err)

cmd/run.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strconv"
 
 	"github.com/ory/viper"
 	"github.com/spf13/cobra"
@@ -19,14 +20,27 @@ func NewRunCmd(newClient ClientFactory) *cobra.Command {
 		Long: `Run the function locally
 
 Runs the function locally in the current directory or in the directory
-specified by --path flag. The function must already have been built with the 'build' command.
+specified by --path flag.
+
+Building
+By default the Function will be built if never built, or if changes are detected
+to the Function's source.  Use --build to override this behavior.
+
 `,
 		Example: `
-# Build function's image first
-{{.Name}} build
-
-# Run it locally as a container
+# Run the Function locally, building if necessary
 {{.Name}} run
+
+# Run the Function, forcing a rebuild of the image.
+#   This is useful when the Function's image was manually deleted, necessitating
+#   A rebuild even when no changes have been made the Function's source.
+{{.Name}} run --build
+
+# Run the Function's existing image, disabling auto-build.
+#   This is useful when filesystem changes have been made, but one wishes to
+#   run the previously built image without rebuilding.
+{{.Name}} run --build=false
+
 `,
 		SuggestFor: []string{"rnu"},
 		PreRunE:    bindEnv("build", "path", "registry"),
@@ -36,7 +50,8 @@ specified by --path flag. The function must already have been built with the 'bu
 		"Environment variable to set in the form NAME=VALUE. "+
 			"You may provide this flag multiple times for setting multiple environment variables. "+
 			"To unset, specify the environment variable name followed by a \"-\" (e.g., NAME-).")
-	cmd.Flags().BoolP("build", "b", false, "Build the function only if the function has not been built before")
+	cmd.Flags().StringP("build", "b", "auto", "Build the function. [auto|true|false].")
+	cmd.Flags().Lookup("build").NoOptDefVal = "true" // --build is equivalient to --build=true
 	cmd.Flags().StringP("registry", "r", GetDefaultRegistry(), "Registry + namespace part of the image if building, ex 'quay.io/myuser' (Env: $FUNC_REGISTRY)")
 	setPathFlag(cmd)
 
@@ -60,14 +75,16 @@ func runRun(cmd *cobra.Command, args []string, newClient ClientFactory) (err err
 		return
 	}
 
-	function.Envs, err = mergeEnvs(function.Envs, config.EnvToUpdate, config.EnvToRemove)
+	var updated int
+	function.Envs, updated, err = mergeEnvs(function.Envs, config.EnvToUpdate, config.EnvToRemove)
 	if err != nil {
 		return
 	}
-
-	err = function.Write()
-	if err != nil {
-		return
+	if updated > 0 {
+		err = function.Write()
+		if err != nil {
+			return
+		}
 	}
 
 	// Check if the Function has been initialized
@@ -81,11 +98,32 @@ func runRun(cmd *cobra.Command, args []string, newClient ClientFactory) (err err
 	client, done := newClient(ClientConfig{Verbose: config.Verbose}, fn.WithRegistry(config.Registry))
 	defer done()
 
-	// Build if not built and --build
-	if config.Build && !function.Built() {
-		if err = client.Build(cmd.Context(), config.Path); err != nil {
-			return
+	// Build?
+	// If --build was set to 'auto', only build if client detects the Function
+	// is stale (has either never been built or has had filesystem modifications
+	// since the last build).
+	if config.Build == "auto" {
+		if !client.Built(function.Root) {
+			if err = client.Build(cmd.Context(), config.Path); err != nil {
+				return
+			}
 		}
+		fmt.Println("Detected Function was already built.  Use --build to override this behavior.")
+		// Otherwise, --build should parse to a truthy value which indicates an explicit
+		// override.
+	} else {
+		build, err := strconv.ParseBool(config.Build)
+		if err != nil {
+			return fmt.Errorf("invalid value for --build '%v'.  accepts 'auto', 'true' or 'false' (or similarly truthy value)", build)
+		}
+		if build {
+			if err = client.Build(cmd.Context(), config.Path); err != nil {
+				return err
+			}
+		} else {
+			fmt.Println("Function build disabled.  Skipping build.")
+		}
+
 	}
 
 	// Run the Function at path
@@ -122,8 +160,9 @@ type runConfig struct {
 	// Envs passed via cmd to removed
 	EnvToRemove []string
 
-	// Perform build if function hasn't been built yet
-	Build bool
+	// Perform build.  Acceptable values are the keyword 'auto', or a truthy
+	// value such as 'true', 'false, '1' or '0'.
+	Build string
 
 	// Registry for the build tag if building
 	Registry string
@@ -136,7 +175,7 @@ func newRunConfig(cmd *cobra.Command) (c runConfig, err error) {
 	}
 
 	return runConfig{
-		Build:       viper.GetBool("build"),
+		Build:       viper.GetString("build"),
 		Path:        viper.GetString("path"),
 		Verbose:     viper.GetBool("verbose"), // defined on root
 		Registry:    viper.GetString("registry"),

cmd/run_test.go

@@ -6,7 +6,6 @@ import (
 	"os"
 	"testing"
 
-	"github.com/ory/viper"
 	fn "knative.dev/kn-plugin-func"
 	"knative.dev/kn-plugin-func/mock"
 	. "knative.dev/kn-plugin-func/testing"
@@ -14,57 +13,85 @@ import (
 
 func TestRun_Run(t *testing.T) {
 	tests := []struct {
-		name         string // name of the test
-		desc         string // description of the test
-		funcState    string // Function state, as described in func.yaml
-		buildFlag    bool   // value to which the --build flag should be set
-		buildError   error  // Set the builder to yield this error
-		runError     error  // Set the runner to yield this error
-		buildInvoked bool   // should Builder.Build be invoked?
-		runInvoked   bool   // should Runner.Run be invoked?
+		name         string   // name of the test
+		desc         string   // description of the test
+		funcState    string   // Function state, as described in func.yaml
+		args         []string // args for the test case
+		buildError   error    // Set the builder to yield this error
+		runError     error    // Set the runner to yield this error
+		buildInvoked bool     // should Builder.Build be invoked?
+		runInvoked   bool     // should Runner.Run be invoked?
 	}{
 		{
-			name: "run when not building",
-			desc: "Should run when build is not enabled",
+			name: "run and build by default",
+			desc: "Should run and build when build flag is not specified",
 			funcState: `name: test-func
 runtime: go
 created: 2009-11-10 23:00:00`,
-			buildFlag:    false,
-			buildInvoked: false,
-			runInvoked:   true,
-		},
-		{
-			name: "run and build",
-			desc: "Should run and build when build is enabled and there is no image",
-			funcState: `name: test-func
-runtime: go
-created: 2009-11-10 23:00:00`,
-			buildFlag:    true,
+			args:         []string{},
 			buildInvoked: true,
 			runInvoked:   true,
 		},
 		{
-			name: "skip rebuild",
-			desc: "Built image doesn't get built again",
-			// TODO: this might be improved by checking if the user provided
-			// the --build=true flag, allowing an override to force rebuild.
-			// This could be accomplished by adding a 'provideBuildFlag' struct
-			// member.
+			name: "run and build flag",
+			desc: "Should run and build when build is merely provided (defaults to true on presence)",
+			funcState: `name: test-func
+runtime: go
+created: 2009-11-10 23:00:00`,
+			args:         []string{"--build"},
+			buildInvoked: true,
+			runInvoked:   true,
+		},
+		{
+			name: "run and build",
+			desc: "Should run and build when build is specifically requested",
+			funcState: `name: test-func
+runtime: go
+created: 2009-11-10 23:00:00`,
+			args:         []string{"--build=true"},
+			buildInvoked: true,
+			runInvoked:   true,
+		},
+		{
+			name: "run without build when disabled",
+			desc: "Should run but not build when build is expressly disabled",
+			funcState: `name: test-func
+runtime: go
+created: 2009-11-10 23:00:00`,
+			args:         []string{"--build=false"}, // can be any truthy value: 0, 'false' etc.
+			buildInvoked: false,
+			runInvoked:   true,
+		},
+		{
+			name: "run and build on auto",
+			desc: "Should run and buil when build flag set to auto",
+			funcState: `name: test-func
+runtime: go
+created: 2009-11-10 23:00:00`,
+			args:         []string{"--build=auto"}, // can be any truthy value: 0, 'false' etc.
+			buildInvoked: true,
+			runInvoked:   true,
+		},
+		{
+			name: "image existence builds",
+			desc: "Should build when image tag exists",
+			// The existence of an image tag value does not mean the Function
+			// is built; that is the purvew of the buld stamp staleness check.
 			funcState: `name: test-func
 runtime: go
 image: exampleimage
 created: 2009-11-10 23:00:00`,
-			buildFlag:    true,
-			buildInvoked: false,
+			args:         []string{},
+			buildInvoked: true,
 			runInvoked:   true,
 		},
 		{
 			name: "Build errors return",
 			desc: "Errors building cause an immediate return with error",
 			funcState: `name: test-func
-runtime: go
-created: 2009-11-10 23:00:00`,
-			buildFlag:    true,
+			runtime: go
+			created: 2009-11-10 23:00:00`,
+			args:         []string{},
 			buildError:   fmt.Errorf("generic build error"),
 			buildInvoked: true,
 			runInvoked:   false,
@@ -95,10 +122,7 @@ created: 2009-11-10 23:00:00`,
 					fn.WithRegistry("ghcr.com/reg"),
 				)
 			}))
-			cmd.SetArgs([]string{}) // Do not use test command args
-
-			// set test case's build
-			viper.SetDefault("build", tt.buildFlag)
+			cmd.SetArgs(tt.args) // Do not use test command args
 
 			// set test case's func.yaml
 			if err := os.WriteFile("func.yaml", []byte(tt.funcState), os.ModePerm); err != nil {

function.go

@@ -313,7 +313,7 @@ func (f Function) Initialized() bool {
 // image indicated actually exists, just that it _should_ exist based off
 // the current state of the Function object, in particular the value of
 // the Image and ImageDiget fields.
-func (f Function) Built() bool {
+func (f Function) HasImage() bool {
 	// If Image (the override) and ImageDigest (the most recent build stamp) are
 	// both empty, the Function is considered unbuilt.
 	// TODO: upgrade to a "build complete" timestamp.

job.go

@@ -6,12 +6,6 @@ import (
 	"path/filepath"
 )
 
-const (
-	// RunDataDir holds transient runtime metadata
-	// By default it is excluded from source control.
-	RunDataDir = ".func"
-)
-
 // Job represents a running Function job (presumably started by this process'
 // Runner instance.
 type Job struct {