fix(update): preserve Kptfile formatting during upgrades#4427
fix(update): preserve Kptfile formatting during upgrades#4427Jaisheesh-2006 wants to merge 9 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.
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>
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.
…Kptfile Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
pkg/kptfile/kptfileutil/util.go
Outdated
| 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 { |
pkg/lib/kptops/clone_test.go
Outdated
| 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
pkg/lib/kptops/render_test.go
Outdated
| 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
pkg/kptfile/kptfileutil/util_test.go
Outdated
| 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!
…rtions Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…iene Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
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