fix(update): preserve Kptfile formatting during upgrades#4427
fix(update): preserve Kptfile formatting during upgrades#4427Jaisheesh-2006 wants to merge 15 commits intokptdev:mainfrom
Conversation
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR addresses Kptfile formatting/comment drift during kpt pkg update by switching upgrade-time Kptfile mutations to an SDK-backed read/modify/write flow intended to preserve YAML structure and comments.
Changes:
- Refactors
kptopsKptfile update helpers to usekptfileutil.UpdateKptfileContentrather than re-marshalling via kyaml. - Adds SDK-based Kptfile write/update logic in
kptfileutilto better preserve formatting/comments. - Hardens tests for cross-platform behavior (CRLF normalization, environment-aware symlink assertions) and adds Kptfile preservation coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/lib/kptops/render_test.go | Normalizes CRLF/LF in golden comparisons to reduce cross-platform diffs. |
| pkg/lib/kptops/clone_test.go | Adds tests asserting UpdateUpstream/UpdateName preserve comments/formatting. |
| pkg/lib/kptops/clone.go | Routes Kptfile mutations through kptfileutil.UpdateKptfileContent to preserve formatting. |
| pkg/kptfile/kptfileutil/util_test.go | Adds tests for comment/format preservation during UpdateKptfile. |
| pkg/kptfile/kptfileutil/util.go | Introduces SDK-based Kptfile write/update utilities and updates decode flow. |
| internal/util/get/get_test.go | Makes symlink assertions conditional on whether symlinks materialize in the test environment. |
| internal/builtins/pkg_context_test.go | Normalizes CRLF/LF for deterministic output comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e6f980d to
ffd9c47
Compare
ffd9c47 to
a4335c5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5f117d1 to
d622237
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ab5870c to
9d4fa6c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil | ||
| } | ||
|
|
||
| func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { |
There was a problem hiding this comment.
| func applyTypedKptfileToSDK(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { | |
| func applyTypedKptfileToKubeObject(sdkKptfile *kptfileko.KptfileKubeObject, desired *kptfilev1.KptFile) error { |
| func normalizeLineEndings(s string) string { | ||
| return strings.ReplaceAll(s, "\r\n", "\n") | ||
| } | ||
|
|
||
| func normalizeAndTrim(s string) string { | ||
| return strings.TrimSpace(normalizeLineEndings(s)) | ||
| } |
There was a problem hiding this comment.
You could probably move these to a more central place and use them elsewhere
There was a problem hiding this comment.
Good idea. I have moved these functions to internal/util/strings/strings.go file so they can be reused across the test suite.
| expected := strings.ReplaceAll(string(exp), "\r\n", "\n") | ||
| actual := strings.ReplaceAll(out.String(), "\r\n", "\n") |
There was a problem hiding this comment.
You could use that normalizeLineEndings function here
| got := strings.ReplaceAll(output.String(), "\r\n", "\n") | ||
| want := strings.ReplaceAll(string(readFile(t, filepath.Join(testdata, test.name, test.want))), "\r\n", "\n") |
There was a problem hiding this comment.
You could use that normalizeLineEndings function here
| if !assert.NoError(t, err) { | ||
| t.FailNow() | ||
| } |
There was a problem hiding this comment.
I can see a lot of these, please use require.NoError instead
There was a problem hiding this comment.
Thank you for pointing out the best practice regarding require versus assert. I have updated all the error and nil checks to use require in the specific test files modified for this PR (executor_test.go and util_test.go).
I noticed there are many other instances of assert.NoError across the broader repository. To prevent scope creep and keep this PR easy to review, I would like to handle those in a separate, dedicated refactoring PR once this feature is merged. Let me know if that works for you!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| image: ghcr.io/kptdev/krm-functions-catalog/ref-folders | ||
| configMap: | ||
| band: Hüsker Dü | ||
| band: H\u00fcsker D\u00fc |
There was a problem hiding this comment.
This YAML test fixture uses H\u00fcsker D\u00fc as a plain scalar. YAML only interprets \uXXXX escapes inside double-quoted strings, so this value is not equivalent to the original Hüsker Dü and may not be testing the intended unicode behavior. Use the literal unicode characters again, or quote the scalar (e.g., with double quotes) if you specifically want escape handling.
| band: H\u00fcsker D\u00fc | |
| band: Hüsker Dü |
| assert.Equal(t, tc.expected, newPath) | ||
| if tc.errString != "" { | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), tc.errString) |
There was a problem hiding this comment.
TestPathRelToRoot asserts the returned path but doesn’t fail the test if err is non-nil for cases where errString is empty. This can let regressions slip through when the function returns both a correct path and an unexpected error. Add an explicit require.NoError(t, err) (or equivalent) in the success-path branch.
| assert.Contains(t, err.Error(), tc.errString) | |
| assert.Contains(t, err.Error(), tc.errString) | |
| } else { | |
| require.NoError(t, err) |
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 kptdev#4306 Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
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 <jaicodes2006@gmail.com>
Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
…ly expectation Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
…Kptfile Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if strings.HasPrefix(line, "diff --git ") { | ||
| inKptfileDiff = line == "diff --git a/Kptfile b/Kptfile" | ||
| } |
There was a problem hiding this comment.
normalizeDiff only enables the Kptfile indentation normalization when the diff header is exactly diff --git a/Kptfile b/Kptfile. Many test diffs in this repo include Kptfiles in subdirectories (e.g. diff --git a/db/Kptfile b/db/Kptfile / a/out/Kptfile b/out/Kptfile), so indentation-only drift in those Kptfiles will not be normalized and can still cause platform-dependent failures. Consider detecting Kptfile diffs by parsing the header paths and checking that both sides end with /Kptfile (or basename Kptfile) rather than matching only the repository root path.
…rtions Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
…iene Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
…harden edge cases - Add format-preserving path to WriteKptfileToFS, matching the existing WriteFile behavior. This ensures kpt fn render (via setRenderStatus in executor.go) no longer reformats the Kptfile, stripping comments and custom formatting. - Guard reflect.ValueOf(val).IsZero() with rv.IsValid() check in setOrRemoveNestedField to prevent panics on invalid reflect values. - Add round-trip idempotency test (TestWriteFile_RoundTripIdempotency) and filesystem format preservation test (TestWriteKptfileToFS_PreservesFormatting). - Fix bug in migratecmd.go where migrateKptfileToRG used err (from p.Kptfile()) instead of gFileErr (from os.Stat()) when checking ResourceGroup file existence. Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
-Fix staticcheck QF1008: simplify kf.ObjectMeta.Annotations to kf.Annotations (ObjectMeta is embedded). -Fix YAML unicode escapes in test fixture: quote scalar so \u00fc is interpreted by the YAML parser. -Add require.NoError for success path in TestPathRelToRoot to catch regressions returning unexpected errors. Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
… expectations Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
928930c to
ed019bb
Compare
Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hunkRE := regexp.MustCompile(`^@@ -\d+,\d+ \+\d+,\d+ @@`) | ||
| mapLikeYAMLRE := regexp.MustCompile(`^\s*-?\s*[A-Za-z0-9_.-]+:\s*.*$`) | ||
| var out []string |
There was a problem hiding this comment.
The Kptfile diff normalization sorts runs of “map-like” changed lines, but the regex allows an optional leading - after whitespace. That means YAML sequence entries like - image: ... match and can get sorted too, which could hide meaningful ordering changes (e.g. pipeline mutator/validator order) in Kptfile diffs. To avoid masking semantic changes, restrict the sorting to plain map entries (exclude sequence items), and/or key it on consistent indentation level/scope rather than sorting all changed lines together after stripping indentation.
| if strings.HasPrefix(line, "diff --git ") { | ||
| flushKptChangedRun() | ||
| inKptfileDiff = line == "diff --git a/Kptfile b/Kptfile" | ||
| } |
There was a problem hiding this comment.
normalizeDiff only treats diffs as “Kptfile diffs” when the header is exactly diff --git a/Kptfile b/Kptfile. Many repo diffs include Kptfiles in subdirectories (e.g. diff --git a/subpkg/Kptfile b/subpkg/Kptfile, a/out/Kptfile, etc.), so the Kptfile-specific normalization (indent trimming / map-run sorting) won’t run for those and can reintroduce cross-platform/formatting flakes. Consider detecting Kptfile diffs by checking that both paths in the diff --git line end with /Kptfile (or extracting the filenames and comparing to Kptfile).
…apshots Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
…snapshot drift Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if strings.HasPrefix(line, "diff --git ") { | ||
| flushKptChangedRun() | ||
| inKptfileDiff = line == "diff --git a/Kptfile b/Kptfile" | ||
| } |
There was a problem hiding this comment.
normalizeDiff only enables the Kptfile-specific normalization when the header line is exactly diff --git a/Kptfile b/Kptfile. Many test diffs in this repo include Kptfiles in subdirectories (e.g. diff --git a/subpkg/Kptfile b/subpkg/Kptfile or a/out/Kptfile), so the new normalization will silently not apply in those cases. Consider detecting Kptfile diffs by parsing the two paths from the diff --git line and checking whether both basenames are Kptfile (or both paths end with /Kptfile).
| var removed []string | ||
| var added []string | ||
| for _, runLine := range kptChangedRun { | ||
| switch runLine[0] { | ||
| case '-': | ||
| removed = append(removed, runLine[1:]) | ||
| case '+': | ||
| added = append(added, runLine[1:]) | ||
| } | ||
| } | ||
| normalizePayload := func(payload string) string { | ||
| payload = strings.TrimLeft(payload, " \t") | ||
| if m := doubleQuotedScalarRE.FindStringSubmatch(payload); m != nil { | ||
| payload = m[1] + m[2] | ||
| } else if m := singleQuotedScalarRE.FindStringSubmatch(payload); m != nil { | ||
| payload = m[1] + strings.ReplaceAll(m[2], "''", "'") | ||
| } | ||
| return payload | ||
| } | ||
| for i := range removed { | ||
| removed[i] = normalizePayload(removed[i]) | ||
| } | ||
| for i := range added { | ||
| added[i] = normalizePayload(added[i]) | ||
| } | ||
| sort.Strings(removed) | ||
| sort.Strings(added) | ||
| for _, line := range removed { | ||
| out = append(out, "-"+line) | ||
| } | ||
| for _, line := range added { | ||
| out = append(out, "+"+line) | ||
| } | ||
| kptChangedRun = nil |
There was a problem hiding this comment.
The Kptfile-specific normalization currently sorts all removed lines and all added lines within each contiguous changed run. This makes diffs order-insensitive even for sequence/list reordering (where order can be semantically meaningful, e.g. pipeline.mutators), so tests could miss regressions where list order changes in Kptfile. Consider narrowing the sort/normalization to clearly map-key-only lines (or specific known map blocks like status.conditions[*] fields) and leaving list-item ordering intact.
Description
This PR fixes Kptfile formatting drift during package upgrades by switching upgrade-time Kptfile handling to an SDK-backed read/update flow and removing legacy rewrite logic from
kptopshelpers.Motivation
During
kpt pkg update, Kptfile rewrites could alter user formatting/comments in ways that were not intended.Issue #4306 tracks this regression and expects upgrade behavior to preserve user-authored Kptfile structure as much as possible.
What Changed
kptfileutil.pkg/lib/kptopshelper flows, reusing the centralized updater.Scope
This change targets upgrade and related Kptfile mutation paths only, with no intended functional change to unrelated package/resource update behavior.
Testing
Validated with targeted and package-level tests, including:
go test ./pkg/kptfile/... -count=1go test ./pkg/lib/kptops -count=1go test ./commands/pkg/update -count=1go test ./internal/util/update/... -count=1Issue
Fixes #4306