Error when explicit destination of pkg get already exists - #4696
Open
gangadhar-res wants to merge 1 commit into
Open
Error when explicit destination of pkg get already exists#4696gangadhar-res wants to merge 1 commit into
gangadhar-res wants to merge 1 commit into
Conversation
gangadhar-res
requested review from
efiacor,
kispaljr,
liamfallon and
mozesl-nokia
as code owners
August 12, 2026 23:46
dosubot
Bot
added
the
size:XS
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes repeated kpt pkg get calls incorrectly creating nested packages.
Changes:
- Rejects explicitly provided destinations that already exist.
- Adds unit coverage for destination resolution scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/lib/util/parse/parse.go |
Adds explicit-destination validation. |
pkg/lib/util/parse/parse_test.go |
Tests destination handling and errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dosubot
Bot
added
size:S
A repeated `kpt pkg get REPO_URI DEST` silently fetched the package into a nested subdirectory (DEST/<pkg-name>): getDest always defaulted to a repo-named subdirectory when the destination existed, even when the user explicitly named the directory that the first run created. Reject an explicitly named destination only when it already contains a Kptfile, which is exactly the repeated-fetch case. Existing directories without a Kptfile keep working as containers to fetch into (DEST/<pkg-name>), the '.' special case is unchanged, and omitted destinations keep the defaulting behavior, so no existing tests change. Fixes kptdev#2656 Signed-off-by: Gangadhar Chalapaka <gangadhar@resolve.ai>
gangadhar-res
force-pushed
the
fix/pkg-get-explicit-dest-exists
branch
from
August 13, 2026 10:37
dd6159c to
db01bbd
Compare
aravindtga
requested changes
Aug 13, 2026
aravindtga
approved these changes
Aug 14, 2026
Author
|
Hello CODEOWNERs @liamfallon @kispaljr @mozesl-nokia @efiacor Could one of your review and merge this code change ? |
Author
|
@efiacor @mozesl-nokia this has an lgtm from @aravindtga and CI is green. Could one of you take a look when you get a chance? It errors when an explicit pkg get destination already contains a package, instead of silently nesting a second copy inside it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2656
Problem: running kpt pkg get REPO_URI DEST a second time silently fetched the package into DEST/ instead of erroring. getDest() always defaulted to a repo-named subdirectory when the destination existed, even when the user explicitly named it.
Fix: when the destination was explicitly provided, already exists, and is not ., return the same "destination directory already exists" error the defaulted path produces. The . special case and the defaulting behavior for omitted destinations are unchanged.
Validation: ran the reproduction steps from the issue against this branch — first run fetches into packages/newDirName, second run now errors with destination directory "packages/newDirName" already exists, and no nested nginx/ directory is created. kpt pkg get with no destination still defaults to ./nginx. Added a Test_getDest table test covering explicit/defaulted/existing/./file/missing-parent cases.