Handle relative vs abs paths in cobra command for update

internal/cmdupdate/cmdupdate.go

@@ -17,6 +17,8 @@ package cmdupdate
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
 	"strings"
 
 	docs "github.com/GoogleContainerTools/kpt/internal/docs/generated/pkgdocs"
@@ -75,7 +77,13 @@ func (r *Runner) preRunE(c *cobra.Command, args []string) error {
 	if len(parts) > 2 {
 		return errors.Errorf("at most 1 version permitted")
 	}
-	r.Update.Path = parts[0]
+
+	var err error
+	r.Update.Path, r.Update.FullPackagePath, err = resolveAbsAndRelPaths(parts[0])
+	if err != nil {
+		return err
+	}
+
 	if len(parts) > 1 {
 		r.Update.Ref = parts[1]
 	}
@@ -98,3 +106,32 @@ func (r *Runner) runE(c *cobra.Command, args []string) error {
 
 	return nil
 }
+
+func resolveAbsAndRelPaths(path string) (string, string, error) {
+	cwd, err := os.Getwd()
+	if err != nil {
+		return "", "", err
+	}
+
+	var relPath string
+	var absPath string
+	if filepath.IsAbs(path) {
+		// If the provided path is absolute, we find the relative path by
+		// comparing it to the current working directory.
+		relPath, err = filepath.Rel(cwd, path)
+		if err != nil {
+			return "", "", err
+		}
+		absPath = filepath.Clean(path)
+	} else {
+		// If the provided path is relative, we find the absolute path by
+		// combining the current working directory with the relative path.
+		relPath = filepath.Clean(path)
+		absPath = filepath.Join(cwd, path)
+	}
+
+	if strings.HasPrefix(relPath, "../") {
+		return "", "", errors.Errorf("package path must be under current working directory")
+	}
+	return relPath, absPath, nil
+}

internal/cmdupdate/cmdupdate_test.go

@@ -15,8 +15,10 @@
 package cmdupdate_test
 
 import (
+	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/GoogleContainerTools/kpt/internal/cmdget"
@@ -225,3 +227,97 @@ func TestCmd_fail(t *testing.T) {
 		assert.Contains(t, err.Error(), "no such file or directory")
 	}
 }
+
+func TestCmd_path(t *testing.T) {
+	var pathPrefix string
+	if runtime.GOOS == "darwin" {
+		pathPrefix = "/private"
+	}
+
+	dir, err := ioutil.TempDir("", "")
+	if !assert.NoError(t, err) {
+		t.FailNow()
+	}
+	defer func() {
+		_ = os.RemoveAll(dir)
+	}()
+
+	testCases := []struct {
+		name                    string
+		currentWD               string
+		path                    string
+		expectedPath            string
+		expectedFullPackagePath string
+		expectedErrMsg          string
+	}{
+		{
+			name:                    "update package in current directory",
+			currentWD:               dir,
+			path:                    ".",
+			expectedPath:            ".",
+			expectedFullPackagePath: filepath.Join(pathPrefix, dir),
+		},
+		{
+			name:                    "update package in subfolder of current directory",
+			currentWD:               filepath.Dir(dir),
+			path:                    filepath.Base(dir),
+			expectedPath:            filepath.Base(dir),
+			expectedFullPackagePath: filepath.Join(pathPrefix, dir),
+		},
+		{
+			name:                    "update package with full absolute path",
+			currentWD:               filepath.Dir(dir),
+			path:                    filepath.Join(pathPrefix, dir),
+			expectedPath:            filepath.Base(dir),
+			expectedFullPackagePath: filepath.Join(pathPrefix, dir),
+		},
+		{
+			name:           "package must exist as a subdirectory of cwd",
+			currentWD:      filepath.Dir(dir),
+			path:           "/var/user/temp",
+			expectedErrMsg: "package path must be under current working directory",
+		},
+	}
+
+	for i := range testCases {
+		test := testCases[i]
+		t.Run(test.name, func(t *testing.T) {
+			currentWD, err := os.Getwd()
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+			defer func() {
+				_ = os.Chdir(currentWD)
+			}()
+			err = os.Chdir(test.currentWD)
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+
+			r := cmdupdate.NewRunner("kpt")
+			r.Command.RunE = func(cmd *cobra.Command, args []string) error {
+				if !assert.Equal(t, test.expectedPath, r.Update.Path) {
+					t.FailNow()
+				}
+				if !assert.Equal(t, test.expectedFullPackagePath, r.Update.FullPackagePath) {
+					t.FailNow()
+				}
+				return nil
+			}
+			r.Command.SetArgs([]string{test.path})
+			err = r.Command.Execute()
+
+			if test.expectedErrMsg != "" {
+				if !assert.Error(t, err) {
+					t.FailNow()
+				}
+				assert.Contains(t, err.Error(), test.expectedErrMsg)
+				return
+			}
+
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+		})
+	}
+}

internal/testutil/testutil.go

@@ -437,9 +437,16 @@ func CopyKptfile(t *testing.T, src, dest string) {
 // SetupDefaultRepoAndWorkspace handles setting up a default repo to clone, and a workspace to clone into.
 // returns a cleanup function to remove the git repo and workspace.
 func SetupDefaultRepoAndWorkspace(t *testing.T, dataset string) (*TestGitRepo, *TestWorkspace, func()) {
+	// Capture the current working directory so we can set it back to the
+	// original path after test has completed.
+	cwd, err := os.Getwd()
+	if err != nil {
+		assert.NoError(t, err)
+	}
+
 	// setup the repo to clone from
 	g := &TestGitRepo{}
-	err := g.SetupTestGitRepo(dataset)
+	err = g.SetupTestGitRepo(dataset)
 	assert.NoError(t, err)
 
 	// setup the directory to clone to
@@ -469,6 +476,7 @@ func SetupDefaultRepoAndWorkspace(t *testing.T, dataset string) (*TestGitRepo, *
 		// ignore cleanup failures
 		_ = g.RemoveAll()
 		_ = w.RemoveAll()
+		_ = os.Chdir(cwd)
 	}
 }
 

internal/util/update/update.go

@@ -18,7 +18,6 @@ package update
 import (
 	"io"
 	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/GoogleContainerTools/kpt/internal/gitutil"
@@ -41,6 +40,9 @@ type UpdateOptions struct {
 	// PackagePath is the relative path to the local package
 	PackagePath string
 
+	// AbsPackagePath is the absolute path to the local package
+	AbsPackagePath string
+
 	// DryRun configures AlphaGitPatch to print a patch rather
 	// than apply it
 	DryRun bool
@@ -105,6 +107,9 @@ type Command struct {
 	// Path is the filepath to the local package
 	Path string
 
+	// FullPackagePath is the absolute path to the local package
+	FullPackagePath string
+
 	// Ref is the ref to update to
 	Ref string
 
@@ -137,14 +142,6 @@ func (u Command) Run() error {
 		u.Output = os.Stdout
 	}
 
-	if filepath.IsAbs(u.Path) {
-		return errors.Errorf("package path must be relative")
-	}
-	u.Path = filepath.Clean(u.Path)
-	if strings.HasPrefix(u.Path, "../") {
-		return errors.Errorf("package path must be under current working directory")
-	}
-
 	kptfile, err := kptfileutil.ReadFileStrict(u.Path)
 	if err != nil {
 		return errors.Errorf("unable to read package Kptfile: %v", err)
@@ -175,15 +172,16 @@ func (u Command) Run() error {
 		return errors.Errorf("unrecognized update strategy %s", u.Strategy)
 	}
 	err = updater().Update(UpdateOptions{
-		KptFile:       kptfile,
-		ToRef:         u.Ref,
-		ToRepo:        u.Repo,
-		PackagePath:   u.Path,
-		DryRun:        u.DryRun,
-		Verbose:       u.Verbose,
-		SimpleMessage: u.SimpleMessage,
-		Output:        u.Output,
-		AutoSet:       u.AutoSet,
+		KptFile:        kptfile,
+		ToRef:          u.Ref,
+		ToRepo:         u.Repo,
+		PackagePath:    u.Path,
+		AbsPackagePath: u.FullPackagePath,
+		DryRun:         u.DryRun,
+		Verbose:        u.Verbose,
+		SimpleMessage:  u.SimpleMessage,
+		Output:         u.Output,
+		AutoSet:        u.AutoSet,
 	})
 
 	if err != nil {