diff --git a/pkg/lib/util/parse/parse.go b/pkg/lib/util/parse/parse.go index 7ef16eeecd..f45a10229f 100644 --- a/pkg/lib/util/parse/parse.go +++ b/pkg/lib/util/parse/parse.go @@ -276,6 +276,17 @@ func getDest(v, repo, subdir string, explicitDest bool) (string, error) { return v, nil } + // The user explicitly named a destination that already holds a fetched + // package: error out instead of silently nesting another copy inside it, + // so a repeated `kpt pkg get REPO_URI DEST` fails the same way the + // 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 { + return "", errors.Errorf("destination directory %q already exists and contains a package", v) + } + } + // default the location to a new subdirectory matching the pkg URI base repo = strings.TrimSuffix(repo, "/") repo = strings.TrimSuffix(repo, ".git") diff --git a/pkg/lib/util/parse/parse_test.go b/pkg/lib/util/parse/parse_test.go index f360d206a8..b5657893ce 100644 --- a/pkg/lib/util/parse/parse_test.go +++ b/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,88 @@ 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 contains a package": { + dest: func(t *testing.T) string { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "Kptfile"), []byte("apiVersion: kpt.dev/v1"), 0o600)) + return dir + }, + explicitDest: true, + errS: "already exists and contains a package", + }, + "explicit destination that exists without a package defaults to a subdirectory": { + dest: func(t *testing.T) string { + return t.TempDir() + }, + explicitDest: true, + want: func(dest string) string { return filepath.Join(dest, "nginx") }, + }, + "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) + }) + } +}