fix(deploy): don't write func.yaml before remote pipeline runs (#3679)

cmd/deploy.go

@@ -318,12 +318,11 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 
 	// Deploy
 	if cfg.Remote {
-		// Write func.yaml before the pipeline uploads sources to the PVC,
-		// so that the on-cluster deploy step sees the latest config
-		// (e.g. --image-pull-secret, --service-account, --deployer).
-		if err = f.Write(); err != nil {
-			return
-		}
+		// NOTE: func.yaml is intentionally NOT written here. The remote
+		// pipeline's source upload injects the in-memory func.yaml (with
+		// any CLI overrides) into the tar stream, so the on-cluster deploy
+		// step sees the latest config without dirtying the working tree
+		// before a successful deploy (issue #3679).
 		var url string
 		// Invoke a remote build/push/deploy pipeline
 		// Returned is the function with fields like Registry, f.Deploy.Image &

cmd/deploy_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 	"reflect"
 	"strings"
@@ -1912,9 +1913,12 @@ func TestDeploy_ImagePullSecretFromEnv(t *testing.T) {
 	}
 }
 
-// TestDeploy_ImagePullSecretRemote ensures that when deploying remotely,
-// func.yaml is written to disk before the pipeline starts, so the on-cluster
-// deploy step picks up the --image-pull-secret value.
+// TestDeploy_ImagePullSecretRemote ensures that when deploying remotely the
+// --image-pull-secret value reaches the pipeline via the in-memory function,
+// and that func.yaml is NOT written to disk before the pipeline runs (the
+// on-disk invariant from issue #3679). The actual byte-level injection into
+// the upload tar stream is covered by TestSourcesAsTarStream; the mock
+// pipeline provider here bypasses sourcesAsTarStream entirely.
 func TestDeploy_ImagePullSecretRemote(t *testing.T) {
 	root := FromTempDirectory(t)
 
@@ -1926,14 +1930,19 @@ func TestDeploy_ImagePullSecretRemote(t *testing.T) {
 
 	pipelinesProvider := mock.NewPipelinesProvider()
 	pipelinesProvider.RunFn = func(f fn.Function) (string, fn.Function, error) {
-		// Inside the pipeline Run, func.yaml on disk should already
-		// have the image pull secret written.
+		// The in-memory function passed to the pipeline must carry the
+		// CLI override so the on-cluster deploy step picks it up.
+		if f.Deploy.ImagePullSecret != "my-remote-secret" {
+			t.Fatalf("expected in-memory function to have imagePullSecret 'my-remote-secret', got '%v'", f.Deploy.ImagePullSecret)
+		}
+		// func.yaml on disk must NOT have been mutated before the pipeline
+		// runs: it should still reflect the pristine init state.
 		diskFn, err := fn.NewFunction(root)
 		if err != nil {
 			t.Fatalf("failed to load func.yaml during pipeline Run: %v", err)
 		}
-		if diskFn.Deploy.ImagePullSecret != "my-remote-secret" {
-			t.Fatalf("expected func.yaml on disk to have imagePullSecret 'my-remote-secret', got '%v'", diskFn.Deploy.ImagePullSecret)
+		if diskFn.Deploy.ImagePullSecret != "" {
+			t.Fatalf("expected func.yaml on disk to be unmodified before the pipeline runs, but it has imagePullSecret '%v'", diskFn.Deploy.ImagePullSecret)
 		}
 		f.Deploy.Namespace = "default"
 		if f.Deploy.Image, err = f.ImageName(); err != nil {
@@ -1956,6 +1965,55 @@ func TestDeploy_ImagePullSecretRemote(t *testing.T) {
 	}
 }
 
+// TestDeploy_RemoteDeployFailureKeepsWorkTreeClean reproduces issue #3679:
+// a failed `func deploy --remote` must NOT mutate func.yaml on disk. The
+// on-disk func.yaml is supposed to reflect deployed state only; a one-off
+// CLI flag (here --image-pull-secret) on a failed deploy should leave the
+// working tree clean so the user can retry with different flags.
+//
+// On current code this FAILS because cmd/deploy.go calls f.Write() before
+// the pipeline runs (regression introduced by #3663).
+func TestDeploy_RemoteDeployFailureKeepsWorkTreeClean(t *testing.T) {
+	root := FromTempDirectory(t)
+
+	f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}
+	if _, err := fn.New().Init(f); err != nil {
+		t.Fatal(err)
+	}
+
+	// Snapshot func.yaml exactly as it sits on disk after init.
+	funcYamlPath := filepath.Join(root, fn.FunctionFile)
+	before, err := os.ReadFile(funcYamlPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// A pipeline that fails, simulating any remote deploy failure.
+	pipelinesProvider := mock.NewPipelinesProvider()
+	pipelinesProvider.RunFn = func(f fn.Function) (string, fn.Function, error) {
+		return "", f, errors.New("simulated remote pipeline failure")
+	}
+
+	cmd := NewDeployCmd(NewTestClient(
+		fn.WithPipelinesProvider(pipelinesProvider),
+		fn.WithRegistry(TestRegistry),
+	))
+	cmd.SetArgs([]string{"--remote", "--image-pull-secret=my-remote-secret", "--namespace=default"})
+	if err := cmd.Execute(); err == nil {
+		t.Fatal("expected the remote deploy to fail, but it succeeded")
+	}
+
+	after, err := os.ReadFile(funcYamlPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if string(before) != string(after) {
+		t.Fatalf("issue #3679 reproduced: func.yaml on disk was mutated by a FAILED "+
+			"remote deploy.\n--- before ---\n%s\n--- after ---\n%s", before, after)
+	}
+}
+
 // Test_ValidateBuilder tests that the builder validation accepts the
 // set of known builders, and spot-checks an error is thrown for unknown.
 func Test_ValidateBuilder(t *testing.T) {

pkg/functions/function.go

@@ -459,9 +459,9 @@ func (f Function) Write() (err error) {
 		return
 	}
 
-	// Write
+	// Serialize the function to its on-disk func.yaml representation
 	var bb []byte
-	if bb, err = yaml.Marshal(&f); err != nil {
+	if bb, err = f.MarshalYAML(); err != nil {
 		return
 	}
 	// TODO: open existing file for writing, such that existing permissions
@@ -472,18 +472,7 @@ func (f Function) Write() (err error) {
 	}
 	defer rwFile.Close()
 
-	schemaURI := funcYamlSchemaURI()
-
-	// Write schema header
-	schemaHeader := fmt.Sprintf(`# $schema: %s
-# yaml-language-server: $schema=%s
-`, schemaURI, schemaURI)
-
-	if _, err = rwFile.WriteString(schemaHeader); err != nil {
-		return err
-	}
-
-	// Write function data
+	// Write function data (schema header + serialized struct)
 	if _, err = rwFile.Write(bb); err != nil {
 		return err
 	}
@@ -507,6 +496,26 @@ func (f Function) Write() (err error) {
 	return
 }
 
+// MarshalYAML returns the function serialized into its on-disk func.yaml
+// representation: the schema header followed by the YAML-marshaled struct.
+// This is the single source of truth for func.yaml's byte content, shared
+// by Write (persisting to disk after a successful deploy) and by the remote
+// pipeline source upload (injecting the in-memory config into the tar stream
+// without dirtying the working tree).
+func (f Function) MarshalYAML() ([]byte, error) {
+	bb, err := yaml.Marshal(&f)
+	if err != nil {
+		return nil, err
+	}
+
+	schemaURI := funcYamlSchemaURI()
+	schemaHeader := fmt.Sprintf(`# $schema: %s
+# yaml-language-server: $schema=%s
+`, schemaURI, schemaURI)
+
+	return append([]byte(schemaHeader), bb...), nil
+}
+
 func funcYamlSchemaURI() string {
 	var (
 		ref  = "main"

pkg/pipelines/tekton/pipelines_provider.go

@@ -299,7 +299,40 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader {
 	go func() {
 		tw := tar.NewWriter(pw)
 
-		err := tw.WriteHeader(&tar.Header{
+		// Serialize the in-memory function (with any one-off CLI overrides
+		// applied) and inject it into the stream as func.yaml, rather than
+		// reading the on-disk file. This lets the on-cluster deploy step see
+		// the latest config without dirtying the user's working tree before
+		// the deploy succeeds (issue #3679).
+		funcYamlBytes, err := f.MarshalYAML()
+		if err != nil {
+			_ = pw.CloseWithError(fmt.Errorf("error while serializing func.yaml for tar stream: %w", err))
+			return
+		}
+
+		writeFuncYaml := func(mode int64) error {
+			hdr := &tar.Header{
+				Typeflag: tar.TypeReg,
+				Name:     path.Join("source", fn.FunctionFile),
+				Size:     int64(len(funcYamlBytes)),
+				Mode:     mode,
+				ModTime:  time.Now(),
+				Uid:      nobodyID,
+				Gid:      nobodyID,
+				Uname:    "nobody",
+				Gname:    "nobody",
+			}
+			if err := tw.WriteHeader(hdr); err != nil {
+				return fmt.Errorf("cannot write func.yaml header to tar stream: %w", err)
+			}
+			if _, err := tw.Write(funcYamlBytes); err != nil {
+				return fmt.Errorf("cannot write func.yaml content to tar stream: %w", err)
+			}
+			return nil
+		}
+		funcYamlInjected := false
+
+		err = tw.WriteHeader(&tar.Header{
 			Typeflag: tar.TypeDir,
 			Name:     "source/",
 			Mode:     0777,
@@ -330,6 +363,16 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader {
 				return nil
 			}
 
+			// Replace the on-disk func.yaml with the in-memory serialization
+			// so CLI overrides reach the on-cluster deploy step (issue #3679).
+			if relp == fn.FunctionFile && fi.Mode().IsRegular() {
+				if err := writeFuncYaml(int64(fi.Mode().Perm())); err != nil {
+					return err
+				}
+				funcYamlInjected = true
+				return nil
+			}
+
 			lnk := ""
 			if fi.Mode()&fs.ModeSymlink != 0 {
 				lnk, err = os.Readlink(p)
@@ -381,6 +424,10 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader {
 			}
 			return nil
 		})
+		if err == nil && !funcYamlInjected {
+			// Function was never persisted to disk: inject func.yaml explicitly.
+			err = writeFuncYaml(0o644)
+		}
 		if err != nil {
 			_ = pw.CloseWithError(fmt.Errorf("error while creating tar stream from sources: %w", err))
 		} else {

pkg/pipelines/tekton/pipelines_provider_test.go

@@ -81,6 +81,93 @@ func TestSourcesAsTarStream(t *testing.T) {
 	}
 }
 
+// readFuncYamlFromStream returns the content of source/func.yaml from the
+// tar stream, or fails if it is absent.
+func readFuncYamlFromStream(t *testing.T, rc io.ReadCloser) []byte {
+	t.Helper()
+	tr := tar.NewReader(rc)
+	for {
+		hdr, err := tr.Next()
+		if err != nil {
+			if errors.Is(err, io.EOF) {
+				break
+			}
+			t.Fatal(err)
+		}
+		if hdr.Name == "source/"+fn.FunctionFile {
+			b, err := io.ReadAll(tr)
+			if err != nil {
+				t.Fatal(err)
+			}
+			return b
+		}
+	}
+	t.Fatalf("source/%s missing from tar stream", fn.FunctionFile)
+	return nil
+}
+
+// TestSourcesAsTarStream_FuncYamlInjection reproduces the fix for issue
+// #3679: the func.yaml placed into the upload tar stream must reflect the
+// in-memory function (with CLI overrides applied), NOT the stale on-disk
+// file, and streaming must not mutate the on-disk file.
+func TestSourcesAsTarStream_FuncYamlInjection(t *testing.T) {
+	t.Run("overrides on-disk func.yaml", func(t *testing.T) {
+		root := t.TempDir()
+
+		// Stale on-disk func.yaml (as if from a previous deploy).
+		onDisk := fn.Function{Root: root, Runtime: "go", Name: "fn"}
+		onDisk.Deploy.ImagePullSecret = "stale-on-disk"
+		if err := onDisk.Write(); err != nil {
+			t.Fatal(err)
+		}
+		before, err := os.ReadFile(filepath.Join(root, fn.FunctionFile))
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// In-memory function with a one-off CLI override.
+		f := fn.Function{Root: root, Runtime: "go", Name: "fn"}
+		f.Deploy.ImagePullSecret = "from-cli"
+
+		rc := sourcesAsTarStream(f)
+		t.Cleanup(func() { _ = rc.Close() })
+
+		streamed := readFuncYamlFromStream(t, rc)
+		if !strings.Contains(string(streamed), "imagePullSecret: from-cli") {
+			t.Fatalf("tar func.yaml did not reflect in-memory override; got:\n%s", streamed)
+		}
+		if strings.Contains(string(streamed), "stale-on-disk") {
+			t.Fatalf("tar func.yaml contains stale on-disk value; got:\n%s", streamed)
+		}
+
+		// The on-disk file must be untouched by streaming.
+		after, err := os.ReadFile(filepath.Join(root, fn.FunctionFile))
+		if err != nil {
+			t.Fatal(err)
+		}
+		if string(before) != string(after) {
+			t.Fatalf("sourcesAsTarStream mutated on-disk func.yaml.\nbefore:\n%s\nafter:\n%s", before, after)
+		}
+	})
+
+	t.Run("injects when func.yaml absent on disk", func(t *testing.T) {
+		root := t.TempDir()
+		f := fn.Function{Root: root, Runtime: "go", Name: "fn"}
+		f.Deploy.ImagePullSecret = "from-cli"
+
+		rc := sourcesAsTarStream(f)
+		t.Cleanup(func() { _ = rc.Close() })
+
+		streamed := readFuncYamlFromStream(t, rc)
+		if !strings.Contains(string(streamed), "imagePullSecret: from-cli") {
+			t.Fatalf("tar func.yaml not injected for never-saved function; got:\n%s", streamed)
+		}
+		if _, err := os.Stat(filepath.Join(root, fn.FunctionFile)); !os.IsNotExist(err) {
+			t.Fatalf("sourcesAsTarStream created func.yaml on disk; err=%v", err)
+		}
+	})
+}
+
 func Test_createPipelinePersistentVolumeClaim(t *testing.T) {
 	type mockType func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error)