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/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) 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) + } + }) +}