From 57d3f5ad696cd0f1dda4a70ba33e02a9705bf14c Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Fri, 6 Mar 2026 12:19:40 +0530 Subject: [PATCH 1/9] fix(update): preserve Kptfile formatting during upgrades Use SDK-backed Kptfile read/update flow in upgrade paths, remove legacy rewrite behavior from kptops helpers, and harden tests for cross-platform stability. Fixes #4306 Signed-off-by: Jaisheesh-2006 --- internal/builtins/pkg_context_test.go | 5 +- internal/util/get/get_test.go | 22 ++- internal/util/render/executor_test.go | 16 +- pkg/kptfile/kptfileutil/util.go | 253 +++++++++++++++++++++++++- pkg/kptfile/kptfileutil/util_test.go | 238 +++++++++++++++++++++++- pkg/lib/kptops/clone.go | 44 ++--- pkg/lib/kptops/clone_test.go | 91 +++++++++ pkg/lib/kptops/render_test.go | 6 +- 8 files changed, 628 insertions(+), 47 deletions(-) diff --git a/internal/builtins/pkg_context_test.go b/internal/builtins/pkg_context_test.go index 32a87fc938..ca4b67a19c 100644 --- a/internal/builtins/pkg_context_test.go +++ b/internal/builtins/pkg_context_test.go @@ -18,6 +18,7 @@ import ( "bytes" "os" "path/filepath" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -62,7 +63,9 @@ func TestPkgContextGenerator(t *testing.T) { if err != test.expErr { t.Errorf("exp: %v got: %v", test.expErr, err) } - if diff := cmp.Diff(string(exp), out.String()); diff != "" { + expected := strings.ReplaceAll(string(exp), "\r\n", "\n") + actual := strings.ReplaceAll(out.String(), "\r\n", "\n") + if diff := cmp.Diff(expected, actual); diff != "" { t.Errorf("pkg context mistmach (-want +got):\n%s", diff) } }) diff --git a/internal/util/get/get_test.go b/internal/util/get/get_test.go index ab965599b1..0d38f30971 100644 --- a/internal/util/get/get_test.go +++ b/internal/util/get/get_test.go @@ -213,15 +213,29 @@ func TestCommand_Run_subdir_symlinks(t *testing.T) { }.Run(fake.CtxWithPrinter(cliOutput, cliOutput)) assert.NoError(t, err) - // ensure warning for symlink is printed on the CLI - assert.Contains(t, cliOutput.String(), `[Warn] Ignoring symlink "config-symlink"`) + sourceSymlinkPath := filepath.Join(g.DatasetDirectory, testutil.Dataset6, subdir, "config-symlink") + info, statErr := os.Lstat(sourceSymlinkPath) + assert.NoError(t, statErr) + isSymlinkInSource := info.Mode()&os.ModeSymlink != 0 + + if isSymlinkInSource { + // ensure warning for symlink is printed on the CLI + assert.Contains(t, cliOutput.String(), `[Warn] Ignoring symlink "config-symlink"`) + } else { + // on environments without symlink materialization, there is no ignore warning + assert.NotContains(t, cliOutput.String(), `[Warn] Ignoring symlink "config-symlink"`) + } // verify the cloned contents do not contains symlinks diff, err := testutil.Diff(filepath.Join(g.DatasetDirectory, testutil.Dataset6, subdir), absPath, true) assert.NoError(t, err) diff = diff.Difference(testutil.KptfileSet) - // original repo contains symlink and cloned doesn't, so the difference - assert.Contains(t, diff.List(), "config-symlink") + if isSymlinkInSource { + // original repo contains symlink and cloned doesn't, so the difference + assert.Contains(t, diff.List(), "config-symlink") + } else { + assert.NotContains(t, diff.List(), "config-symlink") + } // verify the KptFile contains the expected values commit, err := g.GetCommit() diff --git a/internal/util/render/executor_test.go b/internal/util/render/executor_test.go index 9f4280a794..03f4a03776 100644 --- a/internal/util/render/executor_test.go +++ b/internal/util/render/executor_test.go @@ -273,7 +273,7 @@ kind: Kptfile metadata: name: root-package annotations: - kpt.dev/bfs-rendering: %t + kpt.dev/bfs-rendering: "%t" `, renderBfs)) assert.NoError(t, err) @@ -341,8 +341,12 @@ func TestRenderer_Execute_RenderOrder(t *testing.T) { renderer, outputBuffer, ctx := setupRendererTest(t, tc.renderBfs) fnResults, err := renderer.Execute(ctx) - assert.NoError(t, err) - assert.NotNil(t, fnResults) + if !assert.NoError(t, err) { + return + } + if !assert.NotNil(t, fnResults) { + return + } assert.Equal(t, 0, len(fnResults.Items)) output := outputBuffer.String() @@ -426,7 +430,7 @@ kind: Kptfile metadata: name: root-package annotations: - ktp.dev/bfs-rendering: true + kpt.dev/bfs-rendering: "true" `)) assert.NoError(t, err) @@ -440,7 +444,9 @@ metadata: // Create a mock hydration context root, err := newPkgNode(mockFileSystem, rootPkgPath, nil) - assert.NoError(t, err) + if !assert.NoError(t, err) { + return + } hctx := &hydrationContext{ root: root, diff --git a/pkg/kptfile/kptfileutil/util.go b/pkg/kptfile/kptfileutil/util.go index 2070142470..f021c7d8ea 100644 --- a/pkg/kptfile/kptfileutil/util.go +++ b/pkg/kptfile/kptfileutil/util.go @@ -21,6 +21,7 @@ import ( "io" "os" "path/filepath" + "reflect" "slices" "strings" @@ -28,6 +29,8 @@ import ( "github.com/kptdev/kpt/internal/util/git" kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/lib/errors" + "github.com/kptdev/krm-functions-sdk/go/fn" + "github.com/kptdev/krm-functions-sdk/go/fn/kptfileko" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/kustomize/kyaml/sets" @@ -44,6 +47,13 @@ var SupportedKptfileVersions = []schema.GroupVersionKind{ kptfilev1.KptFileGVK(), } +var sdkInternalKptfileAnnotations = []string{ + "config.kubernetes.io/index", + "internal.config.kubernetes.io/index", + "internal.config.kubernetes.io/path", + "internal.config.kubernetes.io/seqindent", +} + // KptfileError records errors regarding reading or parsing of a Kptfile. type KptfileError struct { Path types.UniquePath @@ -78,6 +88,20 @@ func (e *UnknownKptfileResourceError) Error() string { func WriteFile(dir string, k any) error { const op errors.Op = "kptfileutil.WriteFile" + if kf, ok := k.(*kptfilev1.KptFile); ok { + if err := writeKptfilePreservingFormat(dir, kf); err != nil { + return errors.E(op, types.UniquePath(dir), err) + } + return nil + } + + if kf, ok := k.(kptfilev1.KptFile); ok { + if err := writeKptfilePreservingFormat(dir, &kf); err != nil { + return errors.E(op, types.UniquePath(dir), err) + } + return nil + } + b, err := marshalKptfile(k) if err != nil { return err @@ -113,6 +137,157 @@ func marshalKptfile(k any) ([]byte, error) { return yaml.MarshalWithOptions(k, &yaml.EncoderOptions{SeqIndent: yaml.WideSequenceStyle}) } +func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { + kptfilePath := filepath.Join(dir, kptfilev1.KptFileName) + if _, err := os.Stat(dir); err != nil { + return err + } + + content, err := os.ReadFile(kptfilePath) + if err != nil { + if goerrors.Is(err, os.ErrNotExist) { + b, marshalErr := yaml.MarshalWithOptions(kf, &yaml.EncoderOptions{SeqIndent: yaml.WideSequenceStyle}) + if marshalErr != nil { + return marshalErr + } + return os.WriteFile(kptfilePath, b, 0600) + } + return err + } + + existingResources := map[string]string{kptfilev1.KptFileName: string(content)} + existingKptfile, err := kptfileko.NewFromPackage(existingResources) + if err != nil { + return err + } + if err := applyTypedKptfileToSDK(existingKptfile, kf); err != nil { + return err + } + if err := existingKptfile.WriteToPackage(existingResources); err != nil { + return err + } + return os.WriteFile(kptfilePath, []byte(existingResources[kptfilev1.KptFileName]), 0600) +} + +func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { + if sdkKptfile == nil { + return fmt.Errorf("cannot update empty sdk Kptfile") + } + + if err := sdkKptfile.SetNestedString(desired.APIVersion, "apiVersion"); err != nil { + return err + } + if err := sdkKptfile.SetNestedString(desired.Kind, "kind"); err != nil { + return err + } + if err := sdkKptfile.SetNestedString(desired.Name, "metadata", "name"); err != nil { + return err + } + + if err := setOrRemoveNestedField(&sdkKptfile.KubeObject, desired.Annotations, "metadata", "annotations"); err != nil { + return err + } + if err := setOrRemoveNestedField(&sdkKptfile.KubeObject, desired.Labels, "metadata", "labels"); err != nil { + return err + } + if err := setOrRemoveNestedField(&sdkKptfile.KubeObject, desired.Pipeline, "pipeline"); err != nil { + return err + } + if err := setOrRemoveNestedField(&sdkKptfile.KubeObject, desired.Info, "info"); err != nil { + return err + } + if err := setOrRemoveNestedField(&sdkKptfile.KubeObject, desired.Inventory, "inventory"); err != nil { + return err + } + if err := setOrRemoveNestedField(&sdkKptfile.KubeObject, desired.Status, "status"); err != nil { + return err + } + + if err := setOrRemoveUpstream(&sdkKptfile.KubeObject, desired.Upstream); err != nil { + return err + } + + if err := setOrRemoveUpstreamLock(&sdkKptfile.KubeObject, desired.UpstreamLock); err != nil { + return err + } + + return nil +} + +func setOrRemoveNestedField(obj *fn.KubeObject, val any, fields ...string) error { + if val == nil || reflect.ValueOf(val).IsZero() { + _, err := obj.RemoveNestedField(fields...) + return err + } + return obj.SetNestedField(val, fields...) +} + +func setOrRemoveNestedString(obj *fn.KubeObject, value string, fields ...string) error { + if strings.TrimSpace(value) == "" { + _, err := obj.RemoveNestedField(fields...) + return err + } + return obj.SetNestedString(value, fields...) +} + +func setOrRemoveUpstream(obj *fn.KubeObject, upstream *kptfilev1.Upstream) error { + if upstream == nil { + _, err := obj.RemoveNestedField("upstream") + return err + } + + obj.UpsertMap("upstream") + if err := setOrRemoveNestedString(obj, string(upstream.Type), "upstream", "type"); err != nil { + return err + } + if err := setOrRemoveNestedString(obj, string(upstream.UpdateStrategy), "upstream", "updateStrategy"); err != nil { + return err + } + + if upstream.Git == nil { + _, err := obj.RemoveNestedField("upstream", "git") + return err + } + + obj.UpsertMap("upstream").UpsertMap("git") + if err := setOrRemoveNestedString(obj, upstream.Git.Repo, "upstream", "git", "repo"); err != nil { + return err + } + if err := setOrRemoveNestedString(obj, upstream.Git.Directory, "upstream", "git", "directory"); err != nil { + return err + } + return setOrRemoveNestedString(obj, upstream.Git.Ref, "upstream", "git", "ref") +} + +func setOrRemoveUpstreamLock(obj *fn.KubeObject, upstreamLock *kptfilev1.Locator) error { + if upstreamLock == nil { + _, err := obj.RemoveNestedField("upstreamLock") + return err + } + + obj.UpsertMap("upstreamLock") + if err := setOrRemoveNestedString(obj, string(upstreamLock.Type), "upstreamLock", "type"); err != nil { + return err + } + + if upstreamLock.Git == nil { + _, err := obj.RemoveNestedField("upstreamLock", "git") + return err + } + + obj.UpsertMap("upstreamLock").UpsertMap("git") + if err := setOrRemoveNestedString(obj, upstreamLock.Git.Repo, "upstreamLock", "git", "repo"); err != nil { + return err + } + if err := setOrRemoveNestedString(obj, upstreamLock.Git.Directory, "upstreamLock", "git", "directory"); err != nil { + return err + } + if err := setOrRemoveNestedString(obj, upstreamLock.Git.Ref, "upstreamLock", "git", "ref"); err != nil { + return err + } + return setOrRemoveNestedString(obj, upstreamLock.Git.Commit, "upstreamLock", "git", "commit") +} + // ValidateInventory returns true and a nil error if the passed inventory // is valid; otherwiste, false and the reason the inventory is not valid // is returned. A valid inventory must have a non-empty namespace, name, @@ -318,18 +493,88 @@ func DecodeKptfile(in io.Reader) (*kptfilev1.KptFile, error) { if err != nil { return kf, err } - if err := checkKptfileVersion(c); err != nil { + if err := validateKptfileContent(c); err != nil { return kf, err } - d := yaml.NewDecoder(bytes.NewBuffer(c)) - d.KnownFields(true) - if err := d.Decode(kf); err != nil { + kubeObjects, err := fn.ReadKubeObjectsFromFile(kptfilev1.KptFileName, string(c)) + if err != nil { + return kf, err + } + + sdkKptfile, err := kptfileko.NewFromKubeObjectList(kubeObjects) + if err != nil { return kf, err } + + if err := sdkKptfile.As(kf); err != nil { + return kf, err + } + + stripSDKInternalKptfileAnnotations(kf) + return kf, nil } +// UpdateKptfileContent updates Kptfile YAML content in-memory using SDK Kptfile +// read/write APIs while preserving existing YAML document structure and comments. +func UpdateKptfileContent(content string, mutator func(*kptfilev1.KptFile)) (string, error) { + if err := validateKptfileContent([]byte(content)); err != nil { + return "", err + } + + resources := map[string]string{kptfilev1.KptFileName: content} + sdkKptfile, err := kptfileko.NewFromPackage(resources) + if err != nil { + return "", err + } + + typedKptfile := &kptfilev1.KptFile{} + if err := sdkKptfile.As(typedKptfile); err != nil { + return "", err + } + stripSDKInternalKptfileAnnotations(typedKptfile) + + mutator(typedKptfile) + + if err := applyTypedKptfileToSDK(sdkKptfile, typedKptfile); err != nil { + return "", err + } + + if err := sdkKptfile.WriteToPackage(resources); err != nil { + return "", err + } + + return resources[kptfilev1.KptFileName], nil +} + +func validateKptfileContent(content []byte) error { + if err := checkKptfileVersion(content); err != nil { + return err + } + + d := yaml.NewDecoder(bytes.NewBuffer(content)) + d.KnownFields(true) + if err := d.Decode(&kptfilev1.KptFile{}); err != nil { + return err + } + + return nil +} + +func stripSDKInternalKptfileAnnotations(kf *kptfilev1.KptFile) { + if kf == nil || kf.ObjectMeta.Annotations == nil { + return + } + + for _, key := range sdkInternalKptfileAnnotations { + delete(kf.ObjectMeta.Annotations, key) + } + if len(kf.ObjectMeta.Annotations) == 0 { + kf.ObjectMeta.Annotations = nil + } +} + // checkKptfileVersion verifies the apiVersion and kind of the resource // within the Kptfile. If the legacy version is found, the DeprecatedKptfileError // is returned. If the currently supported apiVersion and kind is found, no diff --git a/pkg/kptfile/kptfileutil/util_test.go b/pkg/kptfile/kptfileutil/util_test.go index 424ef11624..cdc6563d40 100644 --- a/pkg/kptfile/kptfileutil/util_test.go +++ b/pkg/kptfile/kptfileutil/util_test.go @@ -595,11 +595,247 @@ status: t.FailNow() } - assert.Equal(t, strings.TrimSpace(tc.expected)+"\n", string(c)) + expectedObj := map[string]any{} + err = yaml.Unmarshal([]byte(strings.TrimSpace(tc.expected)), &expectedObj) + if !assert.NoError(t, err) { + t.FailNow() + } + + actualObj := map[string]any{} + err = yaml.Unmarshal(c, &actualObj) + if !assert.NoError(t, err) { + t.FailNow() + } + + assert.Equal(t, expectedObj, actualObj) }) } } +func TestUpdateKptfile_PreservesCommentsAndFormatting(t *testing.T) { + writeKptfileToTemp := func(tt *testing.T, content string) string { + dir := tt.TempDir() + err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) + if !assert.NoError(tt, err) { + tt.FailNow() + } + return dir + } + + originDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 +`) + + updatedDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 +upstreamLock: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 + commit: abcdef +`) + + localDir := writeKptfileToTemp(t, ` +# local package level comment +apiVersion: kpt.dev/v1 # api comment +kind: Kptfile +metadata: + name: sample + +# preserve this section comment +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 # keep inline comment +`) + + err := UpdateKptfile(localDir, updatedDir, originDir, true) + if !assert.NoError(t, err) { + t.FailNow() + } + + contentBytes, err := os.ReadFile(filepath.Join(localDir, kptfilev1.KptFileName)) + if !assert.NoError(t, err) { + t.FailNow() + } + content := string(contentBytes) + + assert.Contains(t, content, "# local package level comment") + assert.Contains(t, content, "apiVersion: kpt.dev/v1 # api comment") + assert.Contains(t, content, "# preserve this section comment") + assert.Contains(t, content, "ref: v1.1.0 # keep inline comment") + assert.Contains(t, content, "commit: abcdef") +} + +func TestUpdateKptfile_PreservesExactFormattingAndComments(t *testing.T) { + writeKptfileToTemp := func(tt *testing.T, content string) string { + dir := tt.TempDir() + err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) + if !assert.NoError(tt, err) { + tt.FailNow() + } + return dir + } + + originDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 +upstreamLock: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 + commit: abc123 +`) + + updatedDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 +upstreamLock: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 + commit: def456 +`) + + localDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 # keep api inline comment +kind: Kptfile +metadata: + name: sample +# preserve this comment block +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 # keep ref inline comment + +upstreamLock: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 + commit: abc123 # keep commit inline comment +`) + + err := UpdateKptfile(localDir, updatedDir, originDir, true) + if !assert.NoError(t, err) { + t.FailNow() + } + + contentBytes, err := os.ReadFile(filepath.Join(localDir, kptfilev1.KptFileName)) + if !assert.NoError(t, err) { + t.FailNow() + } + + want := ` +apiVersion: kpt.dev/v1 # keep api inline comment +kind: Kptfile +metadata: + name: sample +# preserve this comment block +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 # keep ref inline comment +upstreamLock: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 + commit: def456 # keep commit inline comment +` + + assert.Equal(t, strings.TrimSpace(want), strings.TrimSpace(string(contentBytes))) +} + +func TestWriteFile_ReturnsErrorWhenDirectoryMissing(t *testing.T) { + nonExistentDir := filepath.Join(t.TempDir(), "does-not-exist") + + err := WriteFile(nonExistentDir, DefaultKptfile("sample")) + assert.Error(t, err) +} + +func TestUpdateKptfile_ReturnsErrorOnInvalidLocalKptfile(t *testing.T) { + writeKptfileToTemp := func(tt *testing.T, content string) string { + dir := tt.TempDir() + err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) + if !assert.NoError(tt, err) { + tt.FailNow() + } + return dir + } + + originDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +`) + + updatedDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +`) + + localDir := writeKptfileToTemp(t, ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: [bad +`) + + err := UpdateKptfile(localDir, updatedDir, originDir, true) + assert.Error(t, err) +} + func TestMerge(t *testing.T) { testCases := map[string]struct { origin string diff --git a/pkg/lib/kptops/clone.go b/pkg/lib/kptops/clone.go index 0cd51c402b..f210f48b79 100644 --- a/pkg/lib/kptops/clone.go +++ b/pkg/lib/kptops/clone.go @@ -21,49 +21,35 @@ import ( kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/kptfile/kptfileutil" - "sigs.k8s.io/kustomize/kyaml/yaml" ) func UpdateUpstream(kptfileContents string, name string, upstream kptfilev1.Upstream, lock kptfilev1.Locator) (string, error) { - kptfile, err := kptfileutil.DecodeKptfile(strings.NewReader(kptfileContents)) - if err != nil { - return "", fmt.Errorf("cannot parse Kptfile: %w", err) - } - // Normalize the repository URL and directory path normalizeGitFields(&upstream) normalizeGitLockFields(&lock) // Use separate function for lock - // populate the cloneFrom values so we know where the package came from - kptfile.UpstreamLock = &lock - kptfile.Upstream = &upstream - if name != "" { - kptfile.Name = name - } - - b, err := yaml.MarshalWithOptions(kptfile, &yaml.EncoderOptions{SeqIndent: yaml.WideSequenceStyle}) - if err != nil { - return "", fmt.Errorf("cannot save Kptfile: %w", err) - } - - return string(b), nil + return updateKptfileContentsPreservingFormat(kptfileContents, func(kptfile *kptfilev1.KptFile) { + kptfile.UpstreamLock = &lock + kptfile.Upstream = &upstream + if name != "" { + kptfile.Name = name + } + }) } func UpdateName(kptfileContents string, name string) (string, error) { - kptfile, err := kptfileutil.DecodeKptfile(strings.NewReader(kptfileContents)) - if err != nil { - return "", fmt.Errorf("cannot parse Kptfile: %w", err) - } - - // update the name of the package - kptfile.Name = name + return updateKptfileContentsPreservingFormat(kptfileContents, func(kptfile *kptfilev1.KptFile) { + kptfile.Name = name + }) +} - b, err := yaml.MarshalWithOptions(kptfile, &yaml.EncoderOptions{SeqIndent: yaml.WideSequenceStyle}) +func updateKptfileContentsPreservingFormat(kptfileContents string, mutator func(*kptfilev1.KptFile)) (string, error) { + out, err := kptfileutil.UpdateKptfileContent(kptfileContents, mutator) if err != nil { - return "", fmt.Errorf("cannot save Kptfile: %w", err) + return "", fmt.Errorf("cannot update Kptfile: %w", err) } - return string(b), nil + return out, nil } func UpdateKptfileUpstream(name string, contents map[string]string, upstream kptfilev1.Upstream, lock kptfilev1.Locator) error { diff --git a/pkg/lib/kptops/clone_test.go b/pkg/lib/kptops/clone_test.go index d2123bbfbc..492893d308 100644 --- a/pkg/lib/kptops/clone_test.go +++ b/pkg/lib/kptops/clone_test.go @@ -15,6 +15,7 @@ package kptops import ( + "strings" "testing" kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" @@ -79,3 +80,93 @@ func TestNormalizeGitLockFields(t *testing.T) { t.Errorf("Expected unchanged repo URL, got %q", lock.Git.Repo) } } + +func TestUpdateUpstream_PreservesCommentsAndFormatting(t *testing.T) { + input := ` +apiVersion: kpt.dev/v1 # api inline comment +kind: Kptfile +metadata: + name: sample +# upstream comment +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.0.0 # ref inline comment +` + + upstream := kptfilev1.Upstream{ + Type: kptfilev1.GitOrigin, + Git: &kptfilev1.Git{ + Repo: "https://github.com/example/repo", + Directory: "/package", + Ref: "v1.1.0", + }, + } + + lock := kptfilev1.Locator{ + Type: kptfilev1.GitOrigin, + Git: &kptfilev1.GitLock{ + Repo: "https://github.com/example/repo", + Directory: "/package", + Ref: "v1.1.0", + Commit: "abcdef", + }, + } + + got, err := UpdateUpstream(input, "", upstream, lock) + if err != nil { + t.Fatalf("UpdateUpstream returned error: %v", err) + } + + want := ` +apiVersion: kpt.dev/v1 # api inline comment +kind: Kptfile +metadata: + name: sample +# upstream comment +upstream: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 # ref inline comment +upstreamLock: + type: git + git: + repo: https://github.com/example/repo.git + directory: package + ref: v1.1.0 + commit: abcdef +` + + if strings.TrimSpace(got) != strings.TrimSpace(want) { + t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", want, got) + } +} + +func TestUpdateName_PreservesCommentsAndFormatting(t *testing.T) { + input := ` +apiVersion: kpt.dev/v1 # api inline comment +kind: Kptfile +metadata: + name: old-name # name inline comment +` + + got, err := UpdateName(input, "new-name") + if err != nil { + t.Fatalf("UpdateName returned error: %v", err) + } + + want := ` +apiVersion: kpt.dev/v1 # api inline comment +kind: Kptfile +metadata: + name: new-name # name inline comment +` + + if strings.TrimSpace(got) != strings.TrimSpace(want) { + t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", want, got) + } +} diff --git a/pkg/lib/kptops/render_test.go b/pkg/lib/kptops/render_test.go index 8170e236b7..cee3916b94 100644 --- a/pkg/lib/kptops/render_test.go +++ b/pkg/lib/kptops/render_test.go @@ -78,10 +78,10 @@ func TestRender(t *testing.T) { t.Errorf("Render failed: %v", err) } - got := output.String() - want := readFile(t, filepath.Join(testdata, test.name, test.want)) + got := strings.ReplaceAll(output.String(), "\r\n", "\n") + want := strings.ReplaceAll(string(readFile(t, filepath.Join(testdata, test.name, test.want))), "\r\n", "\n") - if diff := cmp.Diff(string(want), got); diff != "" { + if diff := cmp.Diff(want, got); diff != "" { t.Errorf("Unexpected result (-want, +got): %s", diff) } }) From 6f0915ab0d244a9456355edf728f25d9d2be14cb Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 12 Mar 2026 22:41:01 +0530 Subject: [PATCH 2/9] fix(kptfile): validate and sanitize Kptfile updates Reuse DecodeKptfile validation in UpdateKptfileContent so invalid, deprecated, or unknown-field Kptfiles fail before SDK processing. Strip SDK-internal metadata annotations before applying mutations to avoid writing internal config.kubernetes.io fields back to user Kptfiles. Add regression tests covering validation parity, annotation stripping, empty annotation cleanup, and nil-safe handling. Signed-off-by: Jaisheesh-2006 --- pkg/kptfile/kptfileutil/util_test.go | 143 +++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) diff --git a/pkg/kptfile/kptfileutil/util_test.go b/pkg/kptfile/kptfileutil/util_test.go index cdc6563d40..53222a2144 100644 --- a/pkg/kptfile/kptfileutil/util_test.go +++ b/pkg/kptfile/kptfileutil/util_test.go @@ -836,6 +836,149 @@ metadata: [bad assert.Error(t, err) } +func TestUpdateKptfileContent_UsesDecodeValidation(t *testing.T) { + testCases := map[string]struct { + content string + expectedErr any + expectedDecodeError string + }{ + "deprecated version": { + content: ` +apiVersion: kpt.dev/v1alpha2 +kind: Kptfile +metadata: + name: sample +`, + expectedErr: &DeprecatedKptfileError{}, + expectedDecodeError: "old resource version \"v1alpha2\" found in Kptfile", + }, + "unknown kind": { + content: ` +apiVersion: kpt.dev/v1 +kind: ConfigMap +metadata: + name: sample +`, + expectedErr: &UnknownKptfileResourceError{}, + expectedDecodeError: "unknown resource type \"kpt.dev/v1, Kind=ConfigMap\" found in Kptfile", + }, + "unknown field": { + content: ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +unexpectedField: true +`, + expectedDecodeError: "yaml: unmarshal errors:\n line 6: field unexpectedField not found in type v1.KptFile", + }, + } + + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + _, decodeErr := DecodeKptfile(strings.NewReader(tc.content)) + _, updateErr := UpdateKptfileContent(tc.content, func(*kptfilev1.KptFile) {}) + + if !assert.EqualError(t, decodeErr, tc.expectedDecodeError) { + t.FailNow() + } + if !assert.EqualError(t, updateErr, decodeErr.Error()) { + t.FailNow() + } + if tc.expectedErr != nil { + assert.IsType(t, tc.expectedErr, decodeErr) + assert.IsType(t, tc.expectedErr, updateErr) + } + }) + } +} + +func TestUpdateKptfileContent_StripsSDKInternalAnnotations(t *testing.T) { + t.Run("preserves user annotations", func(t *testing.T) { + content := ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample + annotations: + config.kubernetes.io/index: "0" + internal.config.kubernetes.io/path: Kptfile + user.example.com/keep: value +` + + updatedContent, err := UpdateKptfileContent(content, func(kf *kptfilev1.KptFile) { + kf.Name = "updated-sample" + }) + if !assert.NoError(t, err) { + t.FailNow() + } + + updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent)) + if !assert.NoError(t, err) { + t.FailNow() + } + + assert.Equal(t, "updated-sample", updatedKf.Name) + if assert.NotNil(t, updatedKf.Annotations) { + assert.Equal(t, "value", updatedKf.Annotations["user.example.com/keep"]) + for _, key := range sdkInternalKptfileAnnotations { + assert.NotContains(t, updatedKf.Annotations, key) + } + } + assert.NotContains(t, updatedContent, "config.kubernetes.io/index") + assert.NotContains(t, updatedContent, "internal.config.kubernetes.io/path") + }) + + t.Run("removes empty annotation map", func(t *testing.T) { + content := ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample + annotations: + config.kubernetes.io/index: "0" + internal.config.kubernetes.io/index: "0" +` + + updatedContent, err := UpdateKptfileContent(content, func(*kptfilev1.KptFile) {}) + if !assert.NoError(t, err) { + t.FailNow() + } + + updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent)) + if !assert.NoError(t, err) { + t.FailNow() + } + + assert.Nil(t, updatedKf.Annotations) + assert.NotContains(t, updatedContent, "annotations:") + }) + + t.Run("handles missing annotations safely", func(t *testing.T) { + content := ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +` + + updatedContent, err := UpdateKptfileContent(content, func(kf *kptfilev1.KptFile) { + kf.Name = "updated-sample" + }) + if !assert.NoError(t, err) { + t.FailNow() + } + + updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent)) + if !assert.NoError(t, err) { + t.FailNow() + } + + assert.Equal(t, "updated-sample", updatedKf.Name) + assert.Nil(t, updatedKf.Annotations) + }) +} + func TestMerge(t *testing.T) { testCases := map[string]struct { origin string From 5898d82bc7d77b0800595ff720d25815960809bf Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Wed, 8 Apr 2026 22:14:15 +0530 Subject: [PATCH 3/9] fix(update) : implement copilot suggestions Signed-off-by: Jaisheesh-2006 --- internal/util/get/get_test.go | 8 ++++++-- pkg/kptfile/kptfileutil/util.go | 2 +- pkg/lib/kptops/clone_test.go | 16 ++++++++++++---- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/internal/util/get/get_test.go b/internal/util/get/get_test.go index 0d38f30971..2476e2575e 100644 --- a/internal/util/get/get_test.go +++ b/internal/util/get/get_test.go @@ -215,8 +215,12 @@ func TestCommand_Run_subdir_symlinks(t *testing.T) { sourceSymlinkPath := filepath.Join(g.DatasetDirectory, testutil.Dataset6, subdir, "config-symlink") info, statErr := os.Lstat(sourceSymlinkPath) - assert.NoError(t, statErr) - isSymlinkInSource := info.Mode()&os.ModeSymlink != 0 + isSymlinkInSource := false + if statErr == nil { + isSymlinkInSource = info.Mode()&os.ModeSymlink != 0 + } else if !os.IsNotExist(statErr) { + assert.NoError(t, statErr) + } if isSymlinkInSource { // ensure warning for symlink is printed on the CLI diff --git a/pkg/kptfile/kptfileutil/util.go b/pkg/kptfile/kptfileutil/util.go index f021c7d8ea..275cbce94c 100644 --- a/pkg/kptfile/kptfileutil/util.go +++ b/pkg/kptfile/kptfileutil/util.go @@ -146,7 +146,7 @@ func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { content, err := os.ReadFile(kptfilePath) if err != nil { if goerrors.Is(err, os.ErrNotExist) { - b, marshalErr := yaml.MarshalWithOptions(kf, &yaml.EncoderOptions{SeqIndent: yaml.WideSequenceStyle}) + b, marshalErr := marshalKptfile(kf) if marshalErr != nil { return marshalErr } diff --git a/pkg/lib/kptops/clone_test.go b/pkg/lib/kptops/clone_test.go index 492893d308..89fe74b38e 100644 --- a/pkg/lib/kptops/clone_test.go +++ b/pkg/lib/kptops/clone_test.go @@ -23,6 +23,14 @@ import ( const exampleRepoURL = "https://github.com/example/repo.git" +func normalizeLineEndings(s string) string { + return strings.ReplaceAll(s, "\r\n", "\n") +} + +func normalizeAndTrim(s string) string { + return strings.TrimSpace(normalizeLineEndings(s)) +} + func TestNormalizeGitFields(t *testing.T) { // Test case 1: Add .git suffix and normalize directory path upstream := &kptfilev1.Upstream{ @@ -141,8 +149,8 @@ upstreamLock: commit: abcdef ` - if strings.TrimSpace(got) != strings.TrimSpace(want) { - t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", want, got) + if normalizeAndTrim(got) != normalizeAndTrim(want) { + t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", normalizeLineEndings(want), normalizeLineEndings(got)) } } @@ -166,7 +174,7 @@ metadata: name: new-name # name inline comment ` - if strings.TrimSpace(got) != strings.TrimSpace(want) { - t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", want, got) + if normalizeAndTrim(got) != normalizeAndTrim(want) { + t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", normalizeLineEndings(want), normalizeLineEndings(got)) } } From 99a8ef412de4d5916b25c7be8de9036f9a901a2f Mon Sep 17 00:00:00 2001 From: Jaisheesh Venkat Goud Balagowni Date: Wed, 8 Apr 2026 22:06:40 +0530 Subject: [PATCH 4/9] fix : explicitly treat maps and slices Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jaisheesh-2006 --- pkg/kptfile/kptfileutil/util.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/kptfile/kptfileutil/util.go b/pkg/kptfile/kptfileutil/util.go index 275cbce94c..e4449804ab 100644 --- a/pkg/kptfile/kptfileutil/util.go +++ b/pkg/kptfile/kptfileutil/util.go @@ -215,7 +215,13 @@ func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kp } func setOrRemoveNestedField(obj *fn.KubeObject, val any, fields ...string) error { - if val == nil || reflect.ValueOf(val).IsZero() { + if val == nil { + _, err := obj.RemoveNestedField(fields...) + return err + } + + reflectVal := reflect.ValueOf(val) + if reflectVal.IsZero() || ((reflectVal.Kind() == reflect.Map || reflectVal.Kind() == reflect.Slice) && reflectVal.Len() == 0) { _, err := obj.RemoveNestedField(fields...) return err } From e1e044d24756525b68dec03f3d81b66027131198 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 9 Apr 2026 00:34:10 +0530 Subject: [PATCH 5/9] fix: harden kptfile writes, stabilize tests, and relax flaky live-apply expectation Signed-off-by: Jaisheesh-2006 --- .../live-apply/crd-and-cr/config.yaml | 8 ++++--- internal/util/get/get_test.go | 3 ++- pkg/kptfile/kptfileutil/util.go | 22 +++++++++---------- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/e2e/testdata/live-apply/crd-and-cr/config.yaml b/e2e/testdata/live-apply/crd-and-cr/config.yaml index 95cf45a0fc..11163c2d7e 100644 --- a/e2e/testdata/live-apply/crd-and-cr/config.yaml +++ b/e2e/testdata/live-apply/crd-and-cr/config.yaml @@ -32,16 +32,18 @@ stdOut: | custom.kpt.dev/cr apply successful apply phase finished reconcile phase started - custom.kpt.dev/cr reconcile successful reconcile phase finished inventory update started inventory update finished apply result: 2 attempted, 2 successful, 0 skipped, 0 failed - reconcile result: 2 attempted, 2 successful, 0 skipped, 0 failed, 0 timed out optionalStdOut: - customresourcedefinition.apiextensions.k8s.io/customs.kpt.dev reconcile pending - custom.kpt.dev/cr reconcile pending + - custom.kpt.dev/cr reconcile successful + - custom.kpt.dev/cr reconcile timeout + - reconcile result: 2 attempted, 2 successful, 0 skipped, 0 failed, 0 timed out + - reconcile result: 2 attempted, 1 successful, 0 skipped, 0 failed, 1 timed out inventory: - group: apiextensions.k8s.io @@ -49,4 +51,4 @@ inventory: name: customs.kpt.dev - group: kpt.dev kind: Custom - name: cr \ No newline at end of file + name: cr diff --git a/internal/util/get/get_test.go b/internal/util/get/get_test.go index 2476e2575e..f2b7a8a3e5 100644 --- a/internal/util/get/get_test.go +++ b/internal/util/get/get_test.go @@ -26,6 +26,7 @@ import ( kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/kptdev/kpt/pkg/printer/fake" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/kyaml/kio" "sigs.k8s.io/kustomize/kyaml/kio/filters" "sigs.k8s.io/kustomize/kyaml/yaml" @@ -219,7 +220,7 @@ func TestCommand_Run_subdir_symlinks(t *testing.T) { if statErr == nil { isSymlinkInSource = info.Mode()&os.ModeSymlink != 0 } else if !os.IsNotExist(statErr) { - assert.NoError(t, statErr) + require.NoError(t, statErr) } if isSymlinkInSource { diff --git a/pkg/kptfile/kptfileutil/util.go b/pkg/kptfile/kptfileutil/util.go index e4449804ab..ac169a9886 100644 --- a/pkg/kptfile/kptfileutil/util.go +++ b/pkg/kptfile/kptfileutil/util.go @@ -140,7 +140,7 @@ func marshalKptfile(k any) ([]byte, error) { func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { kptfilePath := filepath.Join(dir, kptfilev1.KptFileName) if _, err := os.Stat(dir); err != nil { - return err + return errors.E(errors.IO, types.UniquePath(dir), err) } content, err := os.ReadFile(kptfilePath) @@ -150,9 +150,12 @@ func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { if marshalErr != nil { return marshalErr } - return os.WriteFile(kptfilePath, b, 0600) + if writeErr := os.WriteFile(kptfilePath, b, 0600); writeErr != nil { + return errors.E(errors.IO, types.UniquePath(kptfilePath), writeErr) + } + return nil } - return err + return errors.E(errors.IO, types.UniquePath(kptfilePath), err) } existingResources := map[string]string{kptfilev1.KptFileName: string(content)} @@ -166,7 +169,10 @@ func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { if err := existingKptfile.WriteToPackage(existingResources); err != nil { return err } - return os.WriteFile(kptfilePath, []byte(existingResources[kptfilev1.KptFileName]), 0600) + if writeErr := os.WriteFile(kptfilePath, []byte(existingResources[kptfilev1.KptFileName]), 0600); writeErr != nil { + return errors.E(errors.IO, types.UniquePath(kptfilePath), writeErr) + } + return nil } func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { @@ -215,13 +221,7 @@ func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kp } func setOrRemoveNestedField(obj *fn.KubeObject, val any, fields ...string) error { - if val == nil { - _, err := obj.RemoveNestedField(fields...) - return err - } - - reflectVal := reflect.ValueOf(val) - if reflectVal.IsZero() || ((reflectVal.Kind() == reflect.Map || reflectVal.Kind() == reflect.Slice) && reflectVal.Len() == 0) { + if val == nil || reflect.ValueOf(val).IsZero() { _, err := obj.RemoveNestedField(fields...) return err } From 9d4fa6c11c3c4b3d3854c103cbd95d0cca6c33aa Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 9 Apr 2026 00:44:40 +0530 Subject: [PATCH 6/9] fix(e2e): quote reconcile summary optional output in crd-and-cr config Signed-off-by: Jaisheesh-2006 --- e2e/testdata/live-apply/crd-and-cr/config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/testdata/live-apply/crd-and-cr/config.yaml b/e2e/testdata/live-apply/crd-and-cr/config.yaml index 11163c2d7e..a96d2cc7a3 100644 --- a/e2e/testdata/live-apply/crd-and-cr/config.yaml +++ b/e2e/testdata/live-apply/crd-and-cr/config.yaml @@ -42,8 +42,8 @@ optionalStdOut: - custom.kpt.dev/cr reconcile pending - custom.kpt.dev/cr reconcile successful - custom.kpt.dev/cr reconcile timeout - - reconcile result: 2 attempted, 2 successful, 0 skipped, 0 failed, 0 timed out - - reconcile result: 2 attempted, 1 successful, 0 skipped, 0 failed, 1 timed out + - "reconcile result: 2 attempted, 2 successful, 0 skipped, 0 failed, 0 timed out" + - "reconcile result: 2 attempted, 1 successful, 0 skipped, 0 failed, 1 timed out" inventory: - group: apiextensions.k8s.io From f536bb8cb9d0ea200cf2cfa332f22e2393ad8db5 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 9 Apr 2026 00:56:13 +0530 Subject: [PATCH 7/9] fix(kptfileutil): harden write/update paths and recover from invalid Kptfile Signed-off-by: Jaisheesh-2006 --- pkg/kptfile/kptfileutil/util.go | 19 +++++++++-- pkg/kptfile/kptfileutil/util_test.go | 50 ++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/pkg/kptfile/kptfileutil/util.go b/pkg/kptfile/kptfileutil/util.go index ac169a9886..1d64c84865 100644 --- a/pkg/kptfile/kptfileutil/util.go +++ b/pkg/kptfile/kptfileutil/util.go @@ -139,9 +139,13 @@ func marshalKptfile(k any) ([]byte, error) { func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { kptfilePath := filepath.Join(dir, kptfilev1.KptFileName) - if _, err := os.Stat(dir); err != nil { + info, err := os.Stat(dir) + if err != nil { return errors.E(errors.IO, types.UniquePath(dir), err) } + if !info.IsDir() { + return errors.E(errors.IO, types.UniquePath(dir), fmt.Errorf("path %q is not a directory", dir)) + } content, err := os.ReadFile(kptfilePath) if err != nil { @@ -161,7 +165,14 @@ func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { existingResources := map[string]string{kptfilev1.KptFileName: string(content)} existingKptfile, err := kptfileko.NewFromPackage(existingResources) if err != nil { - return err + b, marshalErr := marshalKptfile(kf) + if marshalErr != nil { + return marshalErr + } + if writeErr := os.WriteFile(kptfilePath, b, 0600); writeErr != nil { + return errors.E(errors.IO, types.UniquePath(kptfilePath), writeErr) + } + return nil } if err := applyTypedKptfileToSDK(existingKptfile, kf); err != nil { return err @@ -525,6 +536,10 @@ func DecodeKptfile(in io.Reader) (*kptfilev1.KptFile, error) { // UpdateKptfileContent updates Kptfile YAML content in-memory using SDK Kptfile // read/write APIs while preserving existing YAML document structure and comments. func UpdateKptfileContent(content string, mutator func(*kptfilev1.KptFile)) (string, error) { + if mutator == nil { + return "", fmt.Errorf("mutator cannot be nil") + } + if err := validateKptfileContent([]byte(content)); err != nil { return "", err } diff --git a/pkg/kptfile/kptfileutil/util_test.go b/pkg/kptfile/kptfileutil/util_test.go index 53222a2144..4dffc2e129 100644 --- a/pkg/kptfile/kptfileutil/util_test.go +++ b/pkg/kptfile/kptfileutil/util_test.go @@ -802,6 +802,41 @@ func TestWriteFile_ReturnsErrorWhenDirectoryMissing(t *testing.T) { assert.Error(t, err) } +func TestWriteFile_ReturnsErrorWhenPathIsFile(t *testing.T) { + baseDir := t.TempDir() + filePath := filepath.Join(baseDir, "not-a-directory") + err := os.WriteFile(filePath, []byte("content"), 0600) + if !assert.NoError(t, err) { + t.FailNow() + } + + err = WriteFile(filePath, DefaultKptfile("sample")) + assert.Error(t, err) + assert.Contains(t, err.Error(), "not a directory") +} + +func TestWriteFile_RecoversFromInvalidExistingKptfile(t *testing.T) { + dir := t.TempDir() + kptfilePath := filepath.Join(dir, kptfilev1.KptFileName) + err := os.WriteFile(kptfilePath, []byte("apiVersion: kpt.dev/v1\nkind: Kptfile\nmetadata: [bad\n"), 0600) + if !assert.NoError(t, err) { + t.FailNow() + } + + err = WriteFile(dir, DefaultKptfile("sample")) + if !assert.NoError(t, err) { + t.FailNow() + } + + content, err := os.ReadFile(kptfilePath) + if !assert.NoError(t, err) { + t.FailNow() + } + assert.Contains(t, string(content), "apiVersion: kpt.dev/v1") + assert.Contains(t, string(content), "kind: Kptfile") + assert.Contains(t, string(content), "name: sample") +} + func TestUpdateKptfile_ReturnsErrorOnInvalidLocalKptfile(t *testing.T) { writeKptfileToTemp := func(tt *testing.T, content string) string { dir := tt.TempDir() @@ -979,6 +1014,21 @@ metadata: }) } +func TestUpdateKptfileContent_ReturnsErrorOnNilMutator(t *testing.T) { + content := ` +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: sample +` + + _, err := UpdateKptfileContent(content, nil) + if !assert.Error(t, err) { + t.FailNow() + } + assert.Contains(t, err.Error(), "mutator cannot be nil") +} + func TestMerge(t *testing.T) { testCases := map[string]struct { origin string From 757e1bda0eeb15750e37c14a479683dfe6e3771b Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Fri, 10 Apr 2026 11:24:19 +0530 Subject: [PATCH 8/9] test: centralize normalization helpers and clean up kptfile test assertions Signed-off-by: Jaisheesh-2006 --- internal/builtins/pkg_context_test.go | 6 +- internal/util/render/executor_test.go | 148 +++++++++++++------------- internal/util/strings/strings.go | 10 ++ pkg/kptfile/kptfileutil/util.go | 15 ++- pkg/kptfile/kptfileutil/util_test.go | 131 ++++++----------------- pkg/lib/kptops/clone_test.go | 18 +--- pkg/lib/kptops/render_test.go | 5 +- 7 files changed, 133 insertions(+), 200 deletions(-) diff --git a/internal/builtins/pkg_context_test.go b/internal/builtins/pkg_context_test.go index ca4b67a19c..23259dd244 100644 --- a/internal/builtins/pkg_context_test.go +++ b/internal/builtins/pkg_context_test.go @@ -18,10 +18,10 @@ import ( "bytes" "os" "path/filepath" - "strings" "testing" "github.com/google/go-cmp/cmp" + utilstrings "github.com/kptdev/kpt/internal/util/strings" "github.com/stretchr/testify/assert" ) @@ -63,8 +63,8 @@ func TestPkgContextGenerator(t *testing.T) { if err != test.expErr { t.Errorf("exp: %v got: %v", test.expErr, err) } - expected := strings.ReplaceAll(string(exp), "\r\n", "\n") - actual := strings.ReplaceAll(out.String(), "\r\n", "\n") + expected := utilstrings.NormalizeLineEndings(string(exp)) + actual := utilstrings.NormalizeLineEndings(out.String()) if diff := cmp.Diff(expected, actual); diff != "" { t.Errorf("pkg context mistmach (-want +got):\n%s", diff) } diff --git a/internal/util/render/executor_test.go b/internal/util/render/executor_test.go index 03f4a03776..8ed769678f 100644 --- a/internal/util/render/executor_test.go +++ b/internal/util/render/executor_test.go @@ -30,6 +30,7 @@ import ( "github.com/kptdev/kpt/pkg/kptfile/kptfileutil" "github.com/kptdev/kpt/pkg/printer" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/kustomize/kyaml/fn/framework" "sigs.k8s.io/kustomize/kyaml/kio" @@ -100,8 +101,9 @@ func TestPathRelToRoot(t *testing.T) { t.Run(tc.name, func(t *testing.T) { newPath, err := pathRelToRoot(tc.rootPath, tc.subPkgPath, tc.resourcePath) - assert.Equal(t, newPath, tc.expected) + assert.Equal(t, tc.expected, newPath) if tc.errString != "" { + assert.Error(t, err) assert.Contains(t, err.Error(), tc.errString) } }) @@ -231,14 +233,14 @@ metadata: tc := tests[i] t.Run(tc.name, func(t *testing.T) { output, err := kio.ParseAll(tc.output) - assert.NoError(t, err) + require.NoError(t, err) selectedInput, err := kio.ParseAll(tc.selectedInput) - assert.NoError(t, err) + require.NoError(t, err) input, err := kio.ParseAll(tc.input) - assert.NoError(t, err) + require.NoError(t, err) result := fnruntime.MergeWithInput(output, selectedInput, input) actual, err := kio.StringAll(result) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tc.expected, actual) }) } @@ -253,19 +255,19 @@ func setupRendererTest(t *testing.T, renderBfs bool) (*Renderer, *bytes.Buffer, rootPkgPath := rootString err := mockFileSystem.Mkdir(rootPkgPath) - assert.NoError(t, err) + require.NoError(t, err) subPkgPath := subPkgString err = mockFileSystem.Mkdir(subPkgPath) - assert.NoError(t, err) + require.NoError(t, err) childPkgPath := "/root/subpkg/child" err = mockFileSystem.Mkdir(subPkgPath) - assert.NoError(t, err) + require.NoError(t, err) siblingPkgPath := "/root/sibling" err = mockFileSystem.Mkdir(subPkgPath) - assert.NoError(t, err) + require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(rootPkgPath, "Kptfile"), fmt.Appendf(nil, ` apiVersion: kpt.dev/v1 @@ -275,7 +277,7 @@ metadata: annotations: kpt.dev/bfs-rendering: "%t" `, renderBfs)) - assert.NoError(t, err) + require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(subPkgPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 @@ -283,7 +285,7 @@ kind: Kptfile metadata: name: sub-package `)) - assert.NoError(t, err) + require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(siblingPkgPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 @@ -291,7 +293,7 @@ kind: Kptfile metadata: name: sibling-package `)) - assert.NoError(t, err) + require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(childPkgPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 @@ -299,7 +301,7 @@ kind: Kptfile metadata: name: child-package `)) - assert.NoError(t, err) + require.NoError(t, err) renderer := &Renderer{ PkgPath: rootPkgPath, @@ -341,12 +343,8 @@ func TestRenderer_Execute_RenderOrder(t *testing.T) { renderer, outputBuffer, ctx := setupRendererTest(t, tc.renderBfs) fnResults, err := renderer.Execute(ctx) - if !assert.NoError(t, err) { - return - } - if !assert.NotNil(t, fnResults) { - return - } + require.NoError(t, err) + require.NotNil(t, fnResults) assert.Equal(t, 0, len(fnResults.Items)) output := outputBuffer.String() @@ -361,7 +359,7 @@ func TestHydrate_ErrorCases(t *testing.T) { // Create a mock root package rootPath := rootString err := mockFileSystem.Mkdir(rootPath) - assert.NoError(t, err) + require.NoError(t, err) // Add a Kptfile to the root package err = mockFileSystem.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` @@ -370,10 +368,10 @@ kind: Kptfile metadata: name: root-package `)) - assert.NoError(t, err) + require.NoError(t, err) root, err := newPkgNode(mockFileSystem, rootPath, nil) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: root, @@ -397,7 +395,7 @@ metadata: // Simulate an error in LocalResources by creating a package with no Kptfile invalidPkgPath := "/invalid" err := mockFileSystem.Mkdir(invalidPkgPath) - assert.NoError(t, err) + require.NoError(t, err) invalidPkgNode, err := newPkgNode(mockFileSystem, invalidPkgPath, nil) if err != nil { @@ -418,11 +416,11 @@ func TestHydrateBfsOrder_ErrorCases(t *testing.T) { rootPkgPath := rootString err := mockFileSystem.Mkdir(rootPkgPath) - assert.NoError(t, err) + require.NoError(t, err) subPkgPath := subPkgString err = mockFileSystem.Mkdir(subPkgPath) - assert.NoError(t, err) + require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(rootPkgPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 @@ -432,7 +430,7 @@ metadata: annotations: kpt.dev/bfs-rendering: "true" `)) - assert.NoError(t, err) + require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(subPkgPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 @@ -440,13 +438,11 @@ kind: Kptfile metadata: name: sub-package `)) - assert.NoError(t, err) + require.NoError(t, err) // Create a mock hydration context root, err := newPkgNode(mockFileSystem, rootPkgPath, nil) - if !assert.NoError(t, err) { - return - } + require.NoError(t, err) hctx := &hydrationContext{ root: root, @@ -485,7 +481,7 @@ metadata: } _, err := hydrateBfsOrder(ctx, root, hctx) - assert.NoError(t, err) + require.NoError(t, err) }) } @@ -494,7 +490,7 @@ func TestHydrateBfsOrder_RunPipelineError(t *testing.T) { mockFileSystem := filesys.MakeFsInMemory() rootPkgPath := rootString - assert.NoError(t, mockFileSystem.Mkdir(rootPkgPath)) + require.NoError(t, mockFileSystem.Mkdir(rootPkgPath)) // Write a Kptfile with an invalid api version _ = mockFileSystem.WriteFile(filepath.Join(rootPkgPath, "Kptfile"), []byte(` @@ -587,9 +583,9 @@ func TestRenderer_PrintPipelineExecutionSummary(t *testing.T) { func TestUpdateRenderStatus_Success(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.Mkdir(rootPath)) - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -597,7 +593,7 @@ metadata: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: &pkgNode{pkg: rootPkg}, @@ -609,8 +605,8 @@ metadata: updateRenderStatus(hctx, nil) rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status) + require.NoError(t, err) + require.NotNil(t, rootKf.Status) assert.Len(t, rootKf.Status.Conditions, 1) assert.Equal(t, kptfilev1.ConditionTypeRendered, rootKf.Status.Conditions[0].Type) assert.Equal(t, kptfilev1.ConditionTrue, rootKf.Status.Conditions[0].Status) @@ -620,9 +616,9 @@ metadata: func TestUpdateRenderStatus_Failure(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.Mkdir(rootPath)) - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -630,7 +626,7 @@ metadata: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: &pkgNode{pkg: rootPkg}, @@ -642,8 +638,8 @@ metadata: updateRenderStatus(hctx, fmt.Errorf("set-annotations failed: some error")) rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status) + require.NoError(t, err) + require.NotNil(t, rootKf.Status) assert.Len(t, rootKf.Status.Conditions, 1) assert.Equal(t, kptfilev1.ConditionFalse, rootKf.Status.Conditions[0].Status) assert.Equal(t, kptfilev1.ReasonRenderFailed, rootKf.Status.Conditions[0].Reason) @@ -653,10 +649,10 @@ metadata: func TestUpdateRenderStatus_ReplacesExistingCondition(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.Mkdir(rootPath)) // Kptfile with an existing Rendered condition from a previous run - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -670,7 +666,7 @@ status: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: &pkgNode{pkg: rootPkg}, @@ -682,8 +678,8 @@ status: updateRenderStatus(hctx, nil) rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status) + require.NoError(t, err) + require.NotNil(t, rootKf.Status) assert.Len(t, rootKf.Status.Conditions, 1) assert.Equal(t, kptfilev1.ConditionTrue, rootKf.Status.Conditions[0].Status) assert.Equal(t, kptfilev1.ReasonRenderSuccess, rootKf.Status.Conditions[0].Reason) @@ -693,18 +689,18 @@ status: func TestUpdateRenderStatus_OnlyUpdatesRootKptfile(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.Mkdir(rootPath)) subPkgPath := subPkgString - assert.NoError(t, mockFS.Mkdir(subPkgPath)) + require.NoError(t, mockFS.Mkdir(subPkgPath)) - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: name: root-package `))) - assert.NoError(t, mockFS.WriteFile(filepath.Join(subPkgPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.WriteFile(filepath.Join(subPkgPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -712,9 +708,9 @@ metadata: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) subPkg, err := pkg.New(mockFS, subPkgPath) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: &pkgNode{pkg: rootPkg}, @@ -728,14 +724,14 @@ metadata: // Root should have the condition rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status) + require.NoError(t, err) + require.NotNil(t, rootKf.Status) assert.Len(t, rootKf.Status.Conditions, 1) assert.Equal(t, kptfilev1.ConditionTrue, rootKf.Status.Conditions[0].Status) // Subpackage should NOT have any condition subKf, err := kptfileutil.ReadKptfile(mockFS, subPkgPath) - assert.NoError(t, err) + require.NoError(t, err) assert.True(t, subKf.Status == nil || len(subKf.Status.Conditions) == 0) } @@ -753,7 +749,7 @@ func TestBuildRenderStatus_SuccessWithMutationSteps(t *testing.T) { }, } rs := buildRenderStatus(hctx, nil) - assert.NotNil(t, rs) + require.NotNil(t, rs) assert.Len(t, rs.MutationSteps, 2) assert.Empty(t, rs.ValidationSteps) assert.Empty(t, rs.ErrorSummary) @@ -770,7 +766,7 @@ func TestBuildRenderStatus_FailureWithErrorSummary(t *testing.T) { }, } rs := buildRenderStatus(hctx, fmt.Errorf("pipeline failed")) - assert.NotNil(t, rs) + require.NotNil(t, rs) assert.Contains(t, rs.ErrorSummary, "bad-image:v1: exit code 1") assert.Contains(t, rs.ErrorSummary, "gatekeeper:latest: image not found") } @@ -917,8 +913,8 @@ func TestFrameworkResultsToItems_NilFieldValues(t *testing.T) { func TestUpdateRenderStatus_WritesRenderStatus(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -926,7 +922,7 @@ metadata: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: &pkgNode{pkg: rootPkg}, @@ -943,8 +939,8 @@ metadata: updateRenderStatus(hctx, fmt.Errorf("validation failed")) rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status) + require.NoError(t, err) + require.NotNil(t, rootKf.Status) // Condition should be set assert.Len(t, rootKf.Status.Conditions, 1) @@ -952,7 +948,7 @@ metadata: // RenderStatus should be populated rs := rootKf.Status.RenderStatus - assert.NotNil(t, rs) + require.NotNil(t, rs) assert.Len(t, rs.MutationSteps, 1) assert.Equal(t, "set-namespace:v1", rs.MutationSteps[0].Image) assert.Len(t, rs.ValidationSteps, 1) @@ -963,8 +959,8 @@ metadata: func TestUpdateRenderStatus_NilRenderStatusWhenNoSteps(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -972,7 +968,7 @@ metadata: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) hctx := &hydrationContext{ root: &pkgNode{pkg: rootPkg}, @@ -983,16 +979,16 @@ metadata: updateRenderStatus(hctx, nil) rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status) + require.NoError(t, err) + require.NotNil(t, rootKf.Status) assert.Nil(t, rootKf.Status.RenderStatus) } func TestUpdateRenderStatus_ClearsPreviousRenderStatus(t *testing.T) { mockFS := filesys.MakeFsInMemory() rootPath := rootString - assert.NoError(t, mockFS.Mkdir(rootPath)) - assert.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` + require.NoError(t, mockFS.Mkdir(rootPath)) + require.NoError(t, mockFS.WriteFile(filepath.Join(rootPath, "Kptfile"), []byte(` apiVersion: kpt.dev/v1 kind: Kptfile metadata: @@ -1000,7 +996,7 @@ metadata: `))) rootPkg, err := pkg.New(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) // First render: failure with steps hctx := &hydrationContext{ @@ -1014,8 +1010,8 @@ metadata: updateRenderStatus(hctx, fmt.Errorf("fail")) rootKf, err := kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) - assert.NotNil(t, rootKf.Status.RenderStatus) + require.NoError(t, err) + require.NotNil(t, rootKf.Status.RenderStatus) // Second render: success with no steps (empty pipeline) hctx2 := &hydrationContext{ @@ -1026,7 +1022,7 @@ metadata: updateRenderStatus(hctx2, nil) rootKf, err = kptfileutil.ReadKptfile(mockFS, rootPath) - assert.NoError(t, err) + require.NoError(t, err) assert.Nil(t, rootKf.Status.RenderStatus) } @@ -1080,7 +1076,7 @@ metadata: for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { nodes, err := kio.ParseAll(tc.inputYAML) - assert.NoError(t, err) + require.NoError(t, err) clearAnnotationsOnMutFailure(nodes) diff --git a/internal/util/strings/strings.go b/internal/util/strings/strings.go index 5450c5fcfb..fe5d501961 100644 --- a/internal/util/strings/strings.go +++ b/internal/util/strings/strings.go @@ -31,3 +31,13 @@ func JoinStringsWithQuotes(strs []string) string { } return b.String() } + +// NormalizeLineEndings converts CRLF line endings to LF. +func NormalizeLineEndings(s string) string { + return strings.ReplaceAll(s, "\r\n", "\n") +} + +// NormalizeAndTrim normalizes line endings and trims surrounding whitespace. +func NormalizeAndTrim(s string) string { + return strings.TrimSpace(NormalizeLineEndings(s)) +} diff --git a/pkg/kptfile/kptfileutil/util.go b/pkg/kptfile/kptfileutil/util.go index 1d64c84865..436901ce76 100644 --- a/pkg/kptfile/kptfileutil/util.go +++ b/pkg/kptfile/kptfileutil/util.go @@ -88,14 +88,13 @@ func (e *UnknownKptfileResourceError) Error() string { func WriteFile(dir string, k any) error { const op errors.Op = "kptfileutil.WriteFile" - if kf, ok := k.(*kptfilev1.KptFile); ok { + switch kf := k.(type) { + case *kptfilev1.KptFile: if err := writeKptfilePreservingFormat(dir, kf); err != nil { return errors.E(op, types.UniquePath(dir), err) } return nil - } - - if kf, ok := k.(kptfilev1.KptFile); ok { + case kptfilev1.KptFile: if err := writeKptfilePreservingFormat(dir, &kf); err != nil { return errors.E(op, types.UniquePath(dir), err) } @@ -174,7 +173,7 @@ func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { } return nil } - if err := applyTypedKptfileToSDK(existingKptfile, kf); err != nil { + if err := applyTypedKptfileToKubeObject(existingKptfile, kf); err != nil { return err } if err := existingKptfile.WriteToPackage(existingResources); err != nil { @@ -186,9 +185,9 @@ func writeKptfilePreservingFormat(dir string, kf *kptfilev1.KptFile) error { return nil } -func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { +func applyTypedKptfileToKubeObject(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { if sdkKptfile == nil { - return fmt.Errorf("cannot update empty sdk Kptfile") + return fmt.Errorf("cannot update empty Kptfile KubeObject") } if err := sdkKptfile.SetNestedString(desired.APIVersion, "apiVersion"); err != nil { @@ -558,7 +557,7 @@ func UpdateKptfileContent(content string, mutator func(*kptfilev1.KptFile)) (str mutator(typedKptfile) - if err := applyTypedKptfileToSDK(sdkKptfile, typedKptfile); err != nil { + if err := applyTypedKptfileToKubeObject(sdkKptfile, typedKptfile); err != nil { return "", err } diff --git a/pkg/kptfile/kptfileutil/util_test.go b/pkg/kptfile/kptfileutil/util_test.go index 4dffc2e129..aa70e8794e 100644 --- a/pkg/kptfile/kptfileutil/util_test.go +++ b/pkg/kptfile/kptfileutil/util_test.go @@ -22,6 +22,7 @@ import ( kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -61,15 +62,6 @@ func TestValidateInventory(t *testing.T) { } func TestUpdateKptfile(t *testing.T) { - writeKptfileToTemp := func(tt *testing.T, content string) string { - dir := tt.TempDir() - err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) - if !assert.NoError(t, err) { - t.FailNow() - } - return dir - } - testCases := map[string]struct { origin string updated string @@ -586,26 +578,18 @@ status: } err := UpdateKptfile(dirs["local"], dirs["updated"], dirs["origin"], tc.updateUpstream) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) c, err := os.ReadFile(filepath.Join(dirs["local"], kptfilev1.KptFileName)) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) expectedObj := map[string]any{} err = yaml.Unmarshal([]byte(strings.TrimSpace(tc.expected)), &expectedObj) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) actualObj := map[string]any{} err = yaml.Unmarshal(c, &actualObj) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) assert.Equal(t, expectedObj, actualObj) }) @@ -613,15 +597,6 @@ status: } func TestUpdateKptfile_PreservesCommentsAndFormatting(t *testing.T) { - writeKptfileToTemp := func(tt *testing.T, content string) string { - dir := tt.TempDir() - err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) - if !assert.NoError(tt, err) { - tt.FailNow() - } - return dir - } - originDir := writeKptfileToTemp(t, ` apiVersion: kpt.dev/v1 kind: Kptfile @@ -672,14 +647,10 @@ upstream: `) err := UpdateKptfile(localDir, updatedDir, originDir, true) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) contentBytes, err := os.ReadFile(filepath.Join(localDir, kptfilev1.KptFileName)) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) content := string(contentBytes) assert.Contains(t, content, "# local package level comment") @@ -690,15 +661,6 @@ upstream: } func TestUpdateKptfile_PreservesExactFormattingAndComments(t *testing.T) { - writeKptfileToTemp := func(tt *testing.T, content string) string { - dir := tt.TempDir() - err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) - if !assert.NoError(tt, err) { - tt.FailNow() - } - return dir - } - originDir := writeKptfileToTemp(t, ` apiVersion: kpt.dev/v1 kind: Kptfile @@ -762,14 +724,10 @@ upstreamLock: `) err := UpdateKptfile(localDir, updatedDir, originDir, true) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) contentBytes, err := os.ReadFile(filepath.Join(localDir, kptfilev1.KptFileName)) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) want := ` apiVersion: kpt.dev/v1 # keep api inline comment @@ -806,9 +764,7 @@ func TestWriteFile_ReturnsErrorWhenPathIsFile(t *testing.T) { baseDir := t.TempDir() filePath := filepath.Join(baseDir, "not-a-directory") err := os.WriteFile(filePath, []byte("content"), 0600) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) err = WriteFile(filePath, DefaultKptfile("sample")) assert.Error(t, err) @@ -819,34 +775,19 @@ func TestWriteFile_RecoversFromInvalidExistingKptfile(t *testing.T) { dir := t.TempDir() kptfilePath := filepath.Join(dir, kptfilev1.KptFileName) err := os.WriteFile(kptfilePath, []byte("apiVersion: kpt.dev/v1\nkind: Kptfile\nmetadata: [bad\n"), 0600) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) err = WriteFile(dir, DefaultKptfile("sample")) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) content, err := os.ReadFile(kptfilePath) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) assert.Contains(t, string(content), "apiVersion: kpt.dev/v1") assert.Contains(t, string(content), "kind: Kptfile") assert.Contains(t, string(content), "name: sample") } func TestUpdateKptfile_ReturnsErrorOnInvalidLocalKptfile(t *testing.T) { - writeKptfileToTemp := func(tt *testing.T, content string) string { - dir := tt.TempDir() - err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) - if !assert.NoError(tt, err) { - tt.FailNow() - } - return dir - } - originDir := writeKptfileToTemp(t, ` apiVersion: kpt.dev/v1 kind: Kptfile @@ -944,14 +885,10 @@ metadata: updatedContent, err := UpdateKptfileContent(content, func(kf *kptfilev1.KptFile) { kf.Name = "updated-sample" }) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent)) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) assert.Equal(t, "updated-sample", updatedKf.Name) if assert.NotNil(t, updatedKf.Annotations) { @@ -976,14 +913,10 @@ metadata: ` updatedContent, err := UpdateKptfileContent(content, func(*kptfilev1.KptFile) {}) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent)) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) assert.Nil(t, updatedKf.Annotations) assert.NotContains(t, updatedContent, "annotations:") @@ -1000,14 +933,10 @@ metadata: updatedContent, err := UpdateKptfileContent(content, func(kf *kptfilev1.KptFile) { kf.Name = "updated-sample" }) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent)) - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) assert.Equal(t, "updated-sample", updatedKf.Name) assert.Nil(t, updatedKf.Annotations) @@ -1791,7 +1720,7 @@ pipeline: - name: ref-folders image: ghcr.io/kptdev/krm-functions-catalog/ref-folders configMap: - band: Hüsker Dü + band: Hüsker Dü `, expected: ` apiVersion: kpt.dev/v1 @@ -1810,18 +1739,16 @@ pipeline: for tn, tc := range testCases { t.Run(tn, func(t *testing.T) { localKf, err := DecodeKptfile(strings.NewReader(tc.local)) - assert.NoError(t, err) + require.NoError(t, err) updatedKf, err := DecodeKptfile(strings.NewReader(tc.update)) - assert.NoError(t, err) + require.NoError(t, err) originKf, err := DecodeKptfile(strings.NewReader(tc.origin)) - assert.NoError(t, err) + require.NoError(t, err) err = merge(localKf, updatedKf, originKf) if tc.err == nil { - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) actual, err := yaml.Marshal(localKf) - assert.NoError(t, err) + require.NoError(t, err) if !assert.Equal(t, strings.TrimSpace(tc.expected), strings.TrimSpace(string(actual))) { t.FailNow() @@ -1837,3 +1764,11 @@ pipeline: }) } } + +func writeKptfileToTemp(t *testing.T, content string) string { + t.Helper() + dir := t.TempDir() + err := os.WriteFile(filepath.Join(dir, kptfilev1.KptFileName), []byte(content), 0600) + require.NoError(t, err) + return dir +} diff --git a/pkg/lib/kptops/clone_test.go b/pkg/lib/kptops/clone_test.go index 89fe74b38e..cf8f7a0b60 100644 --- a/pkg/lib/kptops/clone_test.go +++ b/pkg/lib/kptops/clone_test.go @@ -15,22 +15,14 @@ package kptops import ( - "strings" "testing" + utilstrings "github.com/kptdev/kpt/internal/util/strings" kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" ) const exampleRepoURL = "https://github.com/example/repo.git" -func normalizeLineEndings(s string) string { - return strings.ReplaceAll(s, "\r\n", "\n") -} - -func normalizeAndTrim(s string) string { - return strings.TrimSpace(normalizeLineEndings(s)) -} - func TestNormalizeGitFields(t *testing.T) { // Test case 1: Add .git suffix and normalize directory path upstream := &kptfilev1.Upstream{ @@ -149,8 +141,8 @@ upstreamLock: commit: abcdef ` - if normalizeAndTrim(got) != normalizeAndTrim(want) { - t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", normalizeLineEndings(want), normalizeLineEndings(got)) + if utilstrings.NormalizeAndTrim(got) != utilstrings.NormalizeAndTrim(want) { + t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", utilstrings.NormalizeLineEndings(want), utilstrings.NormalizeLineEndings(got)) } } @@ -174,7 +166,7 @@ metadata: name: new-name # name inline comment ` - if normalizeAndTrim(got) != normalizeAndTrim(want) { - t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", normalizeLineEndings(want), normalizeLineEndings(got)) + if utilstrings.NormalizeAndTrim(got) != utilstrings.NormalizeAndTrim(want) { + t.Fatalf("updated Kptfile mismatch\nwant:\n%s\n\ngot:\n%s", utilstrings.NormalizeLineEndings(want), utilstrings.NormalizeLineEndings(got)) } } diff --git a/pkg/lib/kptops/render_test.go b/pkg/lib/kptops/render_test.go index cee3916b94..bc40fc9b0e 100644 --- a/pkg/lib/kptops/render_test.go +++ b/pkg/lib/kptops/render_test.go @@ -26,6 +26,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/kptdev/kpt/internal/pkg" "github.com/kptdev/kpt/internal/util/render" + utilstrings "github.com/kptdev/kpt/internal/util/strings" "github.com/kptdev/kpt/pkg/lib/runneroptions" "github.com/kptdev/kpt/pkg/printer" "github.com/kptdev/kpt/pkg/printer/fake" @@ -78,8 +79,8 @@ func TestRender(t *testing.T) { t.Errorf("Render failed: %v", err) } - got := strings.ReplaceAll(output.String(), "\r\n", "\n") - want := strings.ReplaceAll(string(readFile(t, filepath.Join(testdata, test.name, test.want))), "\r\n", "\n") + got := utilstrings.NormalizeLineEndings(output.String()) + want := utilstrings.NormalizeLineEndings(string(readFile(t, filepath.Join(testdata, test.name, test.want)))) if diff := cmp.Diff(want, got); diff != "" { t.Errorf("Unexpected result (-want, +got): %s", diff) From 086f6cb1a916325cec5e73999cf3f4ac752b3723 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Fri, 10 Apr 2026 13:45:59 +0530 Subject: [PATCH 9/9] fix: preserve kptfile handling semantics and tighten related test hygiene Signed-off-by: Jaisheesh-2006 --- internal/builtins/pkg_context_test.go | 2 +- internal/util/render/executor_test.go | 4 ++-- pkg/kptfile/kptfileutil/util_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/builtins/pkg_context_test.go b/internal/builtins/pkg_context_test.go index 23259dd244..b00ae1bd03 100644 --- a/internal/builtins/pkg_context_test.go +++ b/internal/builtins/pkg_context_test.go @@ -66,7 +66,7 @@ func TestPkgContextGenerator(t *testing.T) { expected := utilstrings.NormalizeLineEndings(string(exp)) actual := utilstrings.NormalizeLineEndings(out.String()) if diff := cmp.Diff(expected, actual); diff != "" { - t.Errorf("pkg context mistmach (-want +got):\n%s", diff) + t.Errorf("pkg context mismatch (-want +got):\n%s", diff) } }) } diff --git a/internal/util/render/executor_test.go b/internal/util/render/executor_test.go index 8ed769678f..6fd0f73e69 100644 --- a/internal/util/render/executor_test.go +++ b/internal/util/render/executor_test.go @@ -262,11 +262,11 @@ func setupRendererTest(t *testing.T, renderBfs bool) (*Renderer, *bytes.Buffer, require.NoError(t, err) childPkgPath := "/root/subpkg/child" - err = mockFileSystem.Mkdir(subPkgPath) + err = mockFileSystem.Mkdir(childPkgPath) require.NoError(t, err) siblingPkgPath := "/root/sibling" - err = mockFileSystem.Mkdir(subPkgPath) + err = mockFileSystem.Mkdir(siblingPkgPath) require.NoError(t, err) err = mockFileSystem.WriteFile(filepath.Join(rootPkgPath, "Kptfile"), fmt.Appendf(nil, ` diff --git a/pkg/kptfile/kptfileutil/util_test.go b/pkg/kptfile/kptfileutil/util_test.go index aa70e8794e..0b487c054f 100644 --- a/pkg/kptfile/kptfileutil/util_test.go +++ b/pkg/kptfile/kptfileutil/util_test.go @@ -1720,7 +1720,7 @@ pipeline: - name: ref-folders image: ghcr.io/kptdev/krm-functions-catalog/ref-folders configMap: - band: Hüsker Dü + band: H\u00fcsker D\u00fc `, expected: ` apiVersion: kpt.dev/v1