`kpt pkg get`: revisit "container directory" behaviour for explicit destinations

[aravindtga]

Background

When kpt pkg get REPO dest is called and dest already exists as a directory, getDest() in pkg/lib/util/parse/parse.go silently appends the package name as a subdirectory (e.g.dest/nginx). This was an intentional design from v0.1.0 mimicking cp semantics, but it creates confusing behaviour:

• If dest doesn't exist: package is written directly to dest
• If dest exists: package is written to dest/<pkg-name>

Same command, same explicit argument, different interpretation depending on filesystem state at invocation time.

Steps

# Setup
mkdir -p /tmp/kpt-container-test && cd /tmp/kpt-container-test
git init

# Case 1: destination does NOT exist
# Package is written directly to "my-package"
kpt pkg get https://github.com/kptdev/kpt.git/package-examples/nginx@main my-package
ls my-package/
# Output: Kptfile  deployment.yaml  svc.yaml

# Cleanup
rm -rf my-package

# Case 2: destination DOES exist (empty directory)
# Package is NOT written to "my-package" — instead it goes to "my-package/nginx"
mkdir my-package
kpt pkg get https://github.com/kptdev/kpt.git/package-examples/nginx@main my-package
ls my-package/
# Output: nginx/
ls my-package/nginx/
# Output: Kptfile  deployment.yaml  svc.yaml

The user typed the same destination (my-package) in both cases, but got different results based on whether the directory existed beforehand.

Problems

1. User intent is ambiguous. When a user explicitly types dest, they most likely mean "put the package here", not "use this as a parent directory". The meaning shouldn't change based on whether they ran mkdir first.
2. Undocumented. The original v0.1.0 docs explained the three cases clearly, but the current documentation at reference/cli/pkg/get/_index.md only says "The local directory to write the package to" - the container behaviour is not mentioned.
3. Source of bugs. Issue kpt pkg get duplicates a package as a subdirectory when attempting to override the local directory name #2656 was caused by this inconsistency, a first fetch with a non-existing path takes the "direct" route, then a repeat fetch finds the directory exists and switches to container logic, silently creating a nested duplicate.

Proposal

Consider deprecating the container pattern for explicit destinations:

• If the user provides a destination and it already exists (non-empty), error unconditionally
• If the user provides a destination and it doesn't exist, create it and fetch into it (current behaviour)
• If no destination is provided, default to ./ in the current directory (current behaviour)

This would align pkg get with the rest of kpt and with git clone semantics.

[gangadhar-res]

Worth doing, and I have a data point from #4696 that might be useful here.

I originally wrote that fix as the blanket version, error whenever an explicit destination already exists, which is close to what you're proposing. CI didn't like it. Several of the TestCmd_ suites fetch into a directory that already exists and rely on the container behaviour, so the blanket rule broke them. That's what pushed me to the narrower Kptfile check that's in the PR now: an existing directory that already holds a package errors, an existing directory without one still works as a container.

So the gap between what's in #4696 and what you're proposing here is exactly the non-empty directory with no Kptfile in it. That's still container behaviour today, and at minimum kpt's own tests depend on it, which makes me suspect some users do too.

That doesn't mean the current semantics are right. I think your ambiguity argument holds, the same command giving a different result depending on whether someone ran mkdir first is hard to defend. It just means removing it is a real behaviour change that probably wants a deprecation rather than a straight switch. Warning when an explicit destination takes the container path, then removing it a release later, would at least give people a signal before their scripts quietly change shape.

One part of this is separable and uncontroversial though, which is the documentation. reference/cli/pkg/get/_index.md currently says only "The local directory to write the package to. Defaults to a subdirectory of the current working directory named after the upstream package", so the container behaviour isn't mentioned anywhere. I'm happy to send a PR documenting the three cases as they actually behave today, independent of whatever you decide about changing them. That way the surprise is at least written down while the bigger question gets settled.