Error when explicit destination of pkg get already exists - #4696
Error when explicit destination of pkg get already exists#4696gangadhar-res wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:XS
✅ 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
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.
| if explicitDest { | ||
| return "", errors.Errorf("destination directory %q already exists", v) |
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>
dd6159c to
db01bbd
Compare
| // defaulted destination does below. An existing directory without a | ||
| // Kptfile keeps working as a container to fetch the package into. | ||
| if explicitDest { | ||
| if _, err := os.Stat(filepath.Join(v, kptfilev1.KptFileName)); err == nil { |
There was a problem hiding this comment.
Is there a specific reason for checking the Kptfile rather than erroring whenever the explicit destination already exists? This could be simpler to just error unconditionally if the directory exists.
There was a problem hiding this comment.
@aravindtga
Good question. That was actually the first version of this PR: unconditionally erroring when the explicit destination exists. It failed CI because many of the existing command tests rely on fetching into an existing directory as a container, for example TestCmd_execute and most of the cmdget suite pass the existing workspace directory and expect the package to land at workspace/. Real scripts that mkdir a target and then run kpt pkg get URL target would break the same way. The Kptfile check was the narrowest rule I could find that still catches the repeated-fetch case from #2656: the second run always trips it because the first run wrote a Kptfile at that path, while container directories keep working. If you would rather error unconditionally and treat the container pattern as deprecated, I am happy to make that change and update the affected tests, but that felt like a bigger behavior decision than this bug fix should make on its own imo.
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.