From 90037fc561616aa1a49e024691a6603c72722e48 Mon Sep 17 00:00:00 2001 From: Elvand Lie Nababan Date: Sun, 11 Oct 2026 20:07:16 +0700 Subject: [PATCH 1/2] test(functions): make funcignore comment test deterministic Fingerprint hashes file paths and modification timestamps. Rapid writes can retain the same mtime, making the comment-pattern test fail even when the file is correctly tracked. Capture the original mtime and explicitly advance it by two seconds after rewriting notes.txt. Keep both inclusion and modification assertions and avoid sleeps or production fingerprint changes. Assisted-by: OpenAI Codex and Claude Code Signed-off-by: Elvand Lie Nababan --- pkg/functions/client_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index 4ead697b58..0b4b63fbe9 100644 --- a/pkg/functions/client_test.go +++ b/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) From c1bc9264c87958928bde7e0695898408f7f10bcb Mon Sep 17 00:00:00 2001 From: Elvand Lie Nababan Date: Sat, 10 Oct 2026 02:20:40 +0700 Subject: [PATCH 2/2] fix(migrations): recover legacy deployer intent at 0.39.0 Legacy func.yaml files stored the chosen deployer only in observed state. Deleting clears that state, causing redeploy to fall back to the configured default. Recover missing top-level intent during migration while preserving explicit intent and observed state. Exercise real YAML loading and CLI delete, reload and redeploy for legacy 0.36.0 and 0.38.0 files, all deployers, global defaults, and flag and environment overrides. Keep plain redeploy as a control. Fixes knative/func#4054 Assisted-by: OpenAI Codex and Claude Code Signed-off-by: Elvand Lie Nababan --- cmd/delete_legacy_test.go | 115 ++++++++++++++++++ pkg/functions/function_migrations.go | 17 +++ .../function_migrations_unit_test.go | 112 +++++++++++++++-- 3 files changed, 232 insertions(+), 12 deletions(-) create mode 100644 cmd/delete_legacy_test.go diff --git a/cmd/delete_legacy_test.go b/cmd/delete_legacy_test.go new file mode 100644 index 0000000000..3dace6c5ed --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/functions/function_migrations.go b/pkg/functions/function_migrations.go index 80124abf22..da9103fe99 100644 --- a/pkg/functions/function_migrations.go +++ b/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 { diff --git a/pkg/functions/function_migrations_unit_test.go b/pkg/functions/function_migrations_unit_test.go index 989f49b20e..c4332e4944 100644 --- a/pkg/functions/function_migrations_unit_test.go +++ b/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) + } + }) +}