Repository: GoogleContainerTools/kpt
Pull request #3080 (2022-05-02 23:37:31)

use kpt git parsing in `rpkg clone`
For `kpt alpha rpkg clone`, make using the `--ref` and `--directory` flags optional when using git URLs, instead allow something like 

```
kpt alpha rpkg clone https://github.com/GoogleCloudPlatform/blueprints.git/catalog/bucket@main cloned-bucket \
  --repository=deployments \
  --namespace=default
```

Throw errors if the ref/directory are both present in the URL and set via flags, and they don't match.

natasha41575: Thanks for the reviews and answering all my questions!

Patch:
diff --git a/internal/cmdrpkgclone/command.go b/internal/cmdrpkgclone/command.go
index 60f48aba..2817319b 100644
--- a/internal/cmdrpkgclone/command.go
+++ b/internal/cmdrpkgclone/command.go
@@ -20,6 +20,7 @@ import (
 	"strings"
 
 	"github.com/GoogleContainerTools/kpt/internal/errors"
+	"github.com/GoogleContainerTools/kpt/internal/util/parse"
 	"github.com/GoogleContainerTools/kpt/internal/util/porch"
 	porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
 	"github.com/spf13/cobra"
@@ -43,6 +44,11 @@ SOURCE_PACKAGE:
     * http://git-repository.git/package-name
     * package-revision-name
 
+  With git URLs, you can either include the package directory and ref in SOURCE_PACKAGE, or
+  you can individually specify the repo, ref, and directory using flags:
+    * kpt alpha rpkg clone http://git-repository.git/package-name@main target --repository=repo
+    * kpt alpha rpkg clone http://git-repository.git target --directory=package-name --ref=main --repository=repo
+
 NAME:
   Target package revision name (downstream package)
   Example: package-name
@@ -53,7 +59,7 @@ Flags:
   Directory within the repository where the upstream package is located.
 
 --ref
-	Ref in the repository where the upstream package is located (branch, tag, SHA)
+  Ref in the repository where the upstream package is located (branch, tag, SHA). The default is 'main'.
 
 --repository
   Repository to which package will be cloned (downstream repository).
@@ -62,7 +68,7 @@ Flags:
   Revision of the downstream package.
 
 --strategy
-  Update strategy that should be used when updating this package; one of: resource-merge, fast-forward, force-delete-replace
+  Update strategy that should be used when updating this package; one of: resource-merge, fast-forward, force-delete-replace.
 `
 )
 
@@ -96,8 +102,8 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner
 
 	c.Flags().StringVar(&r.strategy, "strategy", string(porchapi.ResourceMerge),
 		"update strategy that should be used when updating this package; one of: "+strings.Join(strategies, ","))
-	c.Flags().StringVar(&r.directory, "directory", "/", "Directory within the repository where the upstream package is located.")
-	c.Flags().StringVar(&r.ref, "ref", "main", "Branch in the repository where the upstream package is located.")
+	c.Flags().StringVar(&r.directory, "directory", "", "Directory within the repository where the upstream package is located.")
+	c.Flags().StringVar(&r.ref, "ref", "", "Branch in the repository where the upstream package is located.")
 	c.Flags().StringVar(&r.repository, "repository", "", "Repository to which package will be cloned (downstream repository).")
 	c.Flags().StringVar(&r.revision, "revision", "v1", "Revision of the downstream package.")
 
@@ -154,6 +160,35 @@ func (r *runner) preRunE(cmd *cobra.Command, args []string) error {
 		}
 
 	case strings.Contains(source, "/"):
+		if parse.HasGitSuffix(source) { // extra parsing required
+			repo, dir, ref, err := parse.ParseURL(source)
+			if err != nil {
+				return err
+			}
+			// throw error if values set by flags contradict values parsed from SOURCE_PACKAGE
+			if r.directory != "" && dir != "" && r.directory != dir {
+				return errors.E(op, fmt.Errorf("directory %s specified by --directory contradicts directory %s specified by SOURCE_PACKAGE",
+					r.directory, dir))
+			}
+			if r.ref != "" && ref != "" && r.ref != ref {
+				return errors.E(op, fmt.Errorf("ref %s specified by --ref contradicts ref %s specified by SOURCE_PACKAGE",
+					r.ref, ref))
+			}
+			// grab the values parsed from SOURCE_PACKAGE
+			if r.directory == "" {
+				r.directory = dir
+			}
+			if r.ref == "" {
+				r.ref = ref
+			}
+			source = repo + ".git" // parse.ParseURL removes the git suffix, we need to add it back
+		}
+		if r.ref == "" {
+			r.ref = "main"
+		}
+		if r.directory == "" {
+			r.directory = "/"
+		}
 		r.clone.Upstream.Type = porchapi.RepositoryTypeGit
 		r.clone.Upstream.Git = &porchapi.GitPackage{
 			Repo:      source,
@@ -169,7 +204,6 @@ func (r *runner) preRunE(cmd *cobra.Command, args []string) error {
 	}
 
 	r.target = target
-
 	return nil
 }
 
diff --git a/internal/util/parse/parse.go b/internal/util/parse/parse.go
index cfaaaf18..1cd9d5ab 100644
--- a/internal/util/parse/parse.go
+++ b/internal/util/parse/parse.go
@@ -42,11 +42,7 @@ func GitParseArgs(ctx context.Context, args []string) (Target, error) {
 	}
 
 	// Simple parsing if contains .git{$|/)
-	matched, err := regexp.Match(gitSuffixRegexp, []byte(args[0]))
-	if err != nil {
-		return g, err
-	}
-	if matched {
+	if HasGitSuffix(args[0]) {
 		return targetFromPkgURL(ctx, args[0], args[1])
 	}
 
@@ -93,49 +89,51 @@ func GitParseArgs(ctx context.Context, args []string) (Target, error) {
 // targetFromPkgURL parses a pkg url and destination into kptfile git info and local destination Target
 func targetFromPkgURL(ctx context.Context, pkgURL string, dest string) (Target, error) {
 	g := Target{}
-	repo, dir, version, err := parseURL(ctx, pkgURL)
+	repo, dir, ref, err := ParseURL(pkgURL)
 	if err != nil {
 		return g, err
 	}
+	if dir == "" {
+		dir = "/"
+	}
+	if ref == "" {
+		gur, err := gitutil.NewGitUpstreamRepo(ctx, repo)
+		if err != nil {
+			return g, err
+		}
+		defaultRef, err := gur.GetDefaultBranch(ctx)
+		if err != nil {
+			return g, err
+		}
+		ref = defaultRef
+	}
 	destination, err := getDest(dest, repo, dir)
 	if err != nil {
 		return g, err
 	}
-	g.Ref = version
+	g.Ref = ref
 	g.Directory = path.Clean(dir)
 	g.Repo = repo
 	g.Destination = filepath.Clean(destination)
 	return g, nil
 }
 
-// parseURL parses a pkg url and returns the repo, directory, and version
-func parseURL(ctx context.Context, pkgURL string) (repo string, dir string, version string, err error) {
+// ParseURL parses a pkg url (must contain ".git") and returns the repo, directory, and version
+func ParseURL(pkgURL string) (repo string, dir string, ref string, err error) {
 	parts := regexp.MustCompile(gitSuffixRegexp).Split(pkgURL, 2)
 	index := strings.Index(pkgURL, parts[0])
 	repo = strings.Join([]string{pkgURL[:index], parts[0]}, "")
 	switch {
-	case len(parts) == 1:
+	case len(parts) == 1 || parts[1] == "":
 		// do nothing
 	case strings.Contains(parts[1], "@"):
 		parts := strings.Split(parts[1], "@")
-		version = strings.TrimSuffix(parts[1], "/")
+		ref = strings.TrimSuffix(parts[1], "/")
 		dir = string(filepath.Separator) + parts[0]
 	default:
 		dir = string(filepath.Separator) + parts[1]
 	}
-	if version == "" {
-		gur, err := gitutil.NewGitUpstreamRepo(ctx, repo)
-		if err != nil {
-			return "", "", "", err
-		}
-		defaultRef, err := gur.GetDefaultBranch(ctx)
-		if err != nil {
-			return "", "", "", err
-		}
-		version = defaultRef
-	}
-
-	return repo, dir, version, nil
+	return repo, dir, ref, nil
 }
 
 // pkgURLFromGHURL converts a GitHub URL into a well formed pkg url
@@ -287,3 +285,9 @@ func getDest(v, repo, subdir string) (string, error) {
 	}
 	return v, nil
 }
+
+// HasGitSuffix returns true if the provided pkgURL is a git repo containing the ".git" suffix
+func HasGitSuffix(pkgURL string) bool {
+	matched, err := regexp.Match(gitSuffixRegexp, []byte(pkgURL))
+	return matched && err == nil
+}


Test patch:
diff --git a/e2e/testdata/porch/rpkg-clone/config.yaml b/e2e/testdata/porch/rpkg-clone/config.yaml
index d235d975..8c23448f 100644
--- a/e2e/testdata/porch/rpkg-clone/config.yaml
+++ b/e2e/testdata/porch/rpkg-clone/config.yaml
@@ -11,9 +11,7 @@ commands:
       - rpkg
       - clone
       - --namespace=rpkg-clone
-      - https://github.com/platkrm/test-blueprints.git
-      - --directory=basens
-      - --ref=basens/v1
+      - https://github.com/platkrm/test-blueprints.git/basens@basens/v1
       - --repository=git
       - --revision=v0
       - basens-clone
diff --git a/internal/util/parse/parse_test.go b/internal/util/parse/parse_test.go
index cec11b66..47799d9d 100644
--- a/internal/util/parse/parse_test.go
+++ b/internal/util/parse/parse_test.go
@@ -201,7 +201,7 @@ func Test_parseURL(t *testing.T) {
 		},
 		"no ref": {
 			ghURL:    "https://github.com/GoogleContainerTools/kpt-functions-catalog.git/examples/apply-replacements-simple",
-			expected: expected{repo: "https://github.com/GoogleContainerTools/kpt-functions-catalog", dir: "/examples/apply-replacements-simple", version: "master"},
+			expected: expected{repo: "https://github.com/GoogleContainerTools/kpt-functions-catalog", dir: "/examples/apply-replacements-simple", version: ""},
 		},
 		".git appears in the middle": {
 			ghURL:    "https://my-site.com/root.gitops.git/foo@main",
@@ -211,8 +211,7 @@ func Test_parseURL(t *testing.T) {
 	for name, test := range tests {
 		test := test // capture range variable
 		t.Run(name, func(t *testing.T) {
-			ctx := printer.WithContext(context.Background(), printer.New(nil, nil))
-			repo, dir, version, err := parseURL(ctx, test.ghURL)
+			repo, dir, version, err := ParseURL(test.ghURL)
 			assert.NoError(t, err)
 			assert.Equal(t, expected{repo: repo, dir: dir, version: version}, test.expected)
 		})