fix(migrations): preserve legacy deployer intent after undeploy

cmd/delete_legacy_test.go

@@ -0,0 +1,115 @@
+package cmd
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"knative.dev/func/pkg/config"
+	fn "knative.dev/func/pkg/functions"
+	"knative.dev/func/pkg/mock"
+	. "knative.dev/func/pkg/testing"
+)
+
+func TestDelete_LegacyDeployerForRedeploy(t *testing.T) {
+	for _, tt := range []struct {
+		name           string
+		version        string
+		legacyDeployer string
+		globalDeployer string
+		flagDeployer   string
+		envDeployer    string
+		skipDelete     bool
+		want           string
+	}{
+		{name: "0.38.0 keda", version: "0.38.0", legacyDeployer: "keda", want: "keda"},
+		{name: "0.36.0 keda", version: "0.36.0", legacyDeployer: "keda", want: "keda"},
+		{name: "raw", version: "0.38.0", legacyDeployer: "raw", want: "raw"},
+		{name: "knative", version: "0.38.0", legacyDeployer: "knative", want: "knative"},
+		{name: "intent beats global default", version: "0.38.0", legacyDeployer: "keda", globalDeployer: "raw", want: "keda"},
+		{name: "explicit flag overrides", version: "0.38.0", legacyDeployer: "keda", flagDeployer: "raw", want: "raw"},
+		{name: "environment overrides", version: "0.38.0", legacyDeployer: "keda", envDeployer: "raw", want: "raw"},
+		{name: "plain redeploy control", version: "0.38.0", legacyDeployer: "keda", skipDelete: true, want: "keda"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			root := FromTempDirectory(t)
+			f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry})
+			if err != nil {
+				t.Fatal(err)
+			}
+			legacy := fmt.Sprintf("specVersion: %s\nname: %s\nruntime: go\nnamespace: myns\nregistry: %s\ncreated: %s\ndeploy:\n  namespace: myns\n  deployer: %s\n",
+				tt.version, f.Name, TestRegistry, f.Created.UTC().Format(time.RFC3339), tt.legacyDeployer)
+			if err := os.WriteFile(filepath.Join(root, fn.FunctionFile), []byte(legacy), 0644); err != nil {
+				t.Fatal(err)
+			}
+			if tt.globalDeployer != "" {
+				if err := config.CreatePaths(); err != nil {
+					t.Fatal(err)
+				}
+				if err := (config.Global{Deployer: tt.globalDeployer}).Write(config.File()); err != nil {
+					t.Fatal(err)
+				}
+			}
+			if !tt.skipDelete {
+				remover := mock.NewRemover()
+				remover.RemoveFn = func(name, namespace string) error {
+					if name != f.Name || namespace != "myns" {
+						t.Errorf("remove target = %s/%s, want myns/%s", namespace, name, f.Name)
+					}
+					return nil
+				}
+				deleteCmd := NewDeleteCmd(NewTestClient(fn.WithRemovers(remover)))
+				deleteCmd.SetArgs([]string{})
+				if err := deleteCmd.Execute(); err != nil {
+					t.Fatal(err)
+				}
+				if !remover.RemoveInvoked {
+					t.Fatal("remover not invoked")
+				}
+				loaded, err := fn.NewFunction(root)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if loaded.Deployer != tt.legacyDeployer || loaded.Deploy.Deployer != "" || loaded.Deploy.Namespace != "" {
+					t.Errorf("after delete: intent=%q observed=%q namespace=%q", loaded.Deployer, loaded.Deploy.Deployer, loaded.Deploy.Namespace)
+				}
+				if loaded.SpecVersion != fn.LastSpecVersion() {
+					t.Errorf("persisted specVersion = %q, want %q", loaded.SpecVersion, fn.LastSpecVersion())
+				}
+			}
+			if tt.envDeployer != "" {
+				t.Setenv("FUNC_DEPLOYER", tt.envDeployer)
+			}
+			deployer := mock.NewDeployer()
+			deploy := deployer.DeployFn
+			deployer.DeployFn = func(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) {
+				if f.Deployer != tt.want {
+					t.Errorf("deployer received intent %q, want %q", f.Deployer, tt.want)
+				}
+				return deploy(ctx, f)
+			}
+			deployCmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer)))
+			args := []string{}
+			if tt.flagDeployer != "" {
+				args = append(args, "--deployer", tt.flagDeployer)
+			}
+			deployCmd.SetArgs(args)
+			if err := deployCmd.Execute(); err != nil {
+				t.Fatal(err)
+			}
+			if !deployer.DeployInvoked {
+				t.Fatal("deployer not invoked")
+			}
+			loaded, err := fn.NewFunction(root)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if loaded.Deployer != tt.want || loaded.Deploy.Deployer != tt.want {
+				t.Errorf("after redeploy: intent=%q observed=%q, want %q", loaded.Deployer, loaded.Deploy.Deployer, tt.want)
+			}
+		})
+	}
+}

pkg/functions/client_test.go

@@ -2516,6 +2516,10 @@ func TestFingerprint_FuncIgnoreComments(t *testing.T) {
 	if err := os.WriteFile(filepath.Join(root, "notes.txt"), []byte("notes"), 0644); err != nil {
 		t.Fatal(err)
 	}
+	notes, err := os.Stat(filepath.Join(root, "notes.txt"))
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// Ignore notes.txt — fingerprint should change
 	if err := os.WriteFile(filepath.Join(root, ".funcignore"), []byte("notes.txt\n"), 0644); err != nil {
@@ -2542,6 +2546,11 @@ func TestFingerprint_FuncIgnoreComments(t *testing.T) {
 	if err := os.WriteFile(filepath.Join(root, "notes.txt"), []byte("changed"), 0644); err != nil {
 		t.Fatal(err)
 	}
+	// Fingerprint tracks mtimes, which rapid writes may not advance.
+	modified := notes.ModTime().Add(2 * time.Second)
+	if err := os.Chtimes(filepath.Join(root, "notes.txt"), modified, modified); err != nil {
+		t.Fatal(err)
+	}
 	hashModified, _, err := fn.Fingerprint(root)
 	if err != nil {
 		t.Fatal(err)

pkg/functions/function_migrations.go

@@ -127,6 +127,7 @@ var migrations = []migration{
 	{"0.36.0", migratePersistentVolumeTypoFixup},
 	{"0.37.0", migrateGitToSource},
 	{"0.38.0", migrateScaleToTopLevel},
+	{"0.39.0", migrateDeployerIntent},
 	// New Migrations Here.
 }
 
@@ -487,6 +488,22 @@ func migrateScaleToTopLevel(f Function, raw []byte, m migration) (Function, erro
 	return f, nil
 }
 
+// migrateDeployerIntent recovers deployer intent for legacy functions which
+// recorded the deployer only under deploy.deployer (the top-level deployer
+// intent field did not exist before #3953). Without it, deleting such a
+// function clears the observed Deploy.Deployer and the next deploy falls
+// back to the default deployer. An explicitly recorded top-level deployer
+// always wins: the migration only fills in missing intent, never overwriting
+// it. The observed deploy.deployer keeps its YAML tag and stays populated
+// by the primary unmarshal, so observed state is preserved.
+func migrateDeployerIntent(f Function, _ []byte, m migration) (Function, error) {
+	if f.Deployer == "" && f.Deploy.Deployer != "" {
+		f.Deployer = f.Deploy.Deployer
+	}
+	f.SpecVersion = m.version
+	return f, nil
+}
+
 // The pertinent aspects of the Function's schema prior the 1.0.0 version migrations
 type migrateToSpecs_previousFunction struct {
 

pkg/functions/function_migrations_unit_test.go

@@ -575,12 +575,12 @@ deploy:
 		}
 	})
 
-	t.Run("deploy.deployer/deploy.expose survive migration without becoming intent", func(t *testing.T) {
+	t.Run("deploy.deployer/deploy.expose survive migration; intent recovered by the deployer-intent migration", func(t *testing.T) {
 		// A pre-#3953 legacy file recorded the deployer only under the old
 		// deploy.deployer key. Those observed-state fields must survive the
-		// migration, but it must NOT promote the legacy value to f.Deployer
-		// (intent) -- that recovery was removed as an unrelated, pre-existing
-		// concern (see the follow-up ticket).
+		// migration, and the deployer-intent migration (#4054) promotes the
+		// legacy value to f.Deployer (intent) so a later delete-then-redeploy
+		// no longer falls back to the default deployer.
 		migrated := newFn(t, `specVersion: "0.36.0"
 name: testfn
 runtime: go
@@ -594,17 +594,16 @@ deploy:
 		if migrated.Deploy.Expose != "route" {
 			t.Errorf("Deploy.Expose = %q, want route", migrated.Deploy.Expose)
 		}
-		// Intent is left empty: the migration no longer recovers the legacy
-		// deploy.deployer value as f.Deployer.
-		if migrated.Deployer != "" {
-			t.Errorf("Deployer = %q, want empty (legacy intent recovery removed)", migrated.Deployer)
+		// Intent is recovered from the legacy observed field.
+		if migrated.Deployer != "keda" {
+			t.Errorf("Deployer = %q, want keda (legacy intent recovered)", migrated.Deployer)
 		}
 	})
 
-	t.Run("migration never touches f.Deployer intent", func(t *testing.T) {
-		// The migration must leave the intent field exactly as it was loaded --
-		// it neither invents intent from the legacy observed field nor overrides
-		// an already-present one.
+	t.Run("migration never overrides an explicitly recorded f.Deployer intent", func(t *testing.T) {
+		// The migration must leave an already-present intent field exactly as
+		// it was loaded -- it only fills in missing intent, never overriding
+		// an explicit one.
 		migrated := newFn(t, `specVersion: "0.36.0"
 name: testfn
 runtime: go
@@ -631,3 +630,92 @@ runtime: go
 		}
 	})
 }
+
+// TestMigrateDeployerIntent covers issue #4054: a legacy pre-#3953 func.yaml
+// which recorded the deployer only under deploy.deployer must have that value
+// promoted to the top-level deployer intent, so that a later func delete
+// (which clears the observed Deploy.Deployer) no longer drops the deployer
+// and the next deploy falls back to the default. Drives the real load path
+// via NewFunction, like TestMigrateScaleToTopLevel.
+func TestMigrateDeployerIntent(t *testing.T) {
+	newFn := func(t *testing.T, funcYaml string) Function {
+		t.Helper()
+		root := t.TempDir()
+		if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil {
+			t.Fatal(err)
+		}
+		f, err := NewFunction(root)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return f
+	}
+
+	t.Run("0.38.0 file recovers intent on direct upgrade", func(t *testing.T) {
+		// Direct upgrade from the immediately preceding schema version:
+		// only the 0.39.0 deployer-intent migration applies.
+		migrated := newFn(t, `specVersion: "0.38.0"
+name: testfn
+runtime: go
+created: 2024-01-01T00:00:00Z
+deploy:
+  deployer: keda
+`)
+		if migrated.Deployer != "keda" {
+			t.Errorf("Deployer = %q, want keda", migrated.Deployer)
+		}
+		if migrated.Deploy.Deployer != "keda" {
+			t.Errorf("Deploy.Deployer = %q, want keda (observed state untouched)", migrated.Deploy.Deployer)
+		}
+		if migrated.SpecVersion != LastSpecVersion() {
+			t.Errorf("SpecVersion = %q, want %q", migrated.SpecVersion, LastSpecVersion())
+		}
+	})
+
+	t.Run("0.36.0 legacy file recovers intent", func(t *testing.T) {
+		migrated := newFn(t, `specVersion: "0.36.0"
+name: testfn
+runtime: go
+created: 2024-01-01T00:00:00Z
+deploy:
+  deployer: keda
+`)
+		if migrated.Deployer != "keda" {
+			t.Errorf("Deployer = %q, want keda", migrated.Deployer)
+		}
+		if migrated.Deploy.Deployer != "keda" {
+			t.Errorf("Deploy.Deployer = %q, want keda (observed state untouched)", migrated.Deploy.Deployer)
+		}
+		if migrated.SpecVersion != LastSpecVersion() {
+			t.Errorf("SpecVersion = %q, want %q", migrated.SpecVersion, LastSpecVersion())
+		}
+	})
+
+	t.Run("explicit top-level deployer wins", func(t *testing.T) {
+		migrated := newFn(t, `specVersion: "0.38.0"
+name: testfn
+runtime: go
+created: 2024-01-01T00:00:00Z
+deployer: raw
+deploy:
+  deployer: keda
+`)
+		if migrated.Deployer != "raw" {
+			t.Errorf("Deployer = %q, want raw (explicit intent must win)", migrated.Deployer)
+		}
+		if migrated.Deploy.Deployer != "keda" {
+			t.Errorf("Deploy.Deployer = %q, want keda", migrated.Deploy.Deployer)
+		}
+	})
+
+	t.Run("no deployer anywhere stays empty", func(t *testing.T) {
+		migrated := newFn(t, `specVersion: "0.38.0"
+name: testfn
+runtime: go
+created: 2024-01-01T00:00:00Z
+`)
+		if migrated.Deployer != "" {
+			t.Errorf("Deployer = %q, want empty", migrated.Deployer)
+		}
+	})
+}