Error when explicit destination of pkg get already exists

A repeated `kpt pkg get REPO_URI DEST` silently fetched the package
into a nested subdirectory (DEST/<pkg-name>) because getDest always
defaulted to a repo-named subdirectory when the destination existed,
even when the user explicitly named it. Return the existing
'destination directory already exists' error instead when the
destination was explicitly provided (keeping the '.' special case and
the defaulting behavior for omitted destinations).

Fixes #2656

Signed-off-by: Gangadhar Chalapaka <gangadhar@resolve.ai>

Author: gangadhar-res

pkg/lib/util/parse/parse.go

@@ -276,6 +276,14 @@ func getDest(v, repo, subdir string, explicitDest bool) (string, error) {
 		return v, nil
 	}
 
+	// The user explicitly named a destination that already exists: error out
+	// instead of silently defaulting to a nested subdirectory inside it, so a
+	// repeated `kpt pkg get REPO_URI DEST` fails the same way the defaulted
+	// destination does below.
+	if explicitDest {
+		return "", errors.Errorf("destination directory %q already exists", v)
+	}
+
 	// default the location to a new subdirectory matching the pkg URI base
 	repo = strings.TrimSuffix(repo, "/")
 	repo = strings.TrimSuffix(repo, ".git")

pkg/lib/util/parse/parse_test.go

@@ -17,6 +17,8 @@ package parse
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1"
@@ -264,3 +266,79 @@ func Test_GitParseArgs(t *testing.T) {
 		})
 	}
 }
+
+func Test_getDest(t *testing.T) {
+	repo := "https://github.com/kptdev/kpt"
+	subdir := "package-examples/nginx"
+
+	tests := map[string]struct {
+		dest         func(t *testing.T) string
+		explicitDest bool
+		want         func(dest string) string
+		errS         string
+	}{
+		"explicit destination that does not exist yet": {
+			dest: func(t *testing.T) string {
+				return filepath.Join(t.TempDir(), "newDirName")
+			},
+			explicitDest: true,
+			want:         func(dest string) string { return dest },
+		},
+		"explicit destination that already exists": {
+			dest: func(t *testing.T) string {
+				return t.TempDir()
+			},
+			explicitDest: true,
+			errS:         "already exists",
+		},
+		"explicit current directory": {
+			dest:         func(t *testing.T) string { return "." },
+			explicitDest: true,
+			want:         func(string) string { return "." },
+		},
+		"defaulted destination inside an existing directory": {
+			dest: func(t *testing.T) string {
+				return t.TempDir()
+			},
+			explicitDest: false,
+			want:         func(dest string) string { return filepath.Join(dest, "nginx") },
+		},
+		"defaulted destination whose subdirectory already exists": {
+			dest: func(t *testing.T) string {
+				dir := t.TempDir()
+				require.NoError(t, os.Mkdir(filepath.Join(dir, "nginx"), 0o700))
+				return dir
+			},
+			explicitDest: false,
+			errS:         "already exists",
+		},
+		"destination parent does not exist": {
+			dest: func(t *testing.T) string {
+				return filepath.Join(t.TempDir(), "missing-parent", "dest")
+			},
+			explicitDest: true,
+			errS:         "does not exist",
+		},
+		"destination is a file": {
+			dest: func(t *testing.T) string {
+				f := filepath.Join(t.TempDir(), "file")
+				require.NoError(t, os.WriteFile(f, []byte("x"), 0o600))
+				return f
+			},
+			explicitDest: true,
+			errS:         "must be a directory",
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			dest := test.dest(t)
+			actual, err := getDest(dest, repo, subdir, test.explicitDest)
+			if test.errS != "" {
+				require.ErrorContains(t, err, test.errS)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, test.want(dest), actual)
+		})
+	}
+}