use kpt git parsing in `rpkg clone` #3080

natasha41575 · 2022-05-02T23:37:31Z

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.

martinmaly · 2022-05-02T23:46:35Z

internal/cmdrpkgclone/command.go

@@ -154,11 +145,15 @@ func (r *runner) preRunE(cmd *cobra.Command, args []string) error {
 		}

 	case strings.Contains(source, "/"):
+		gitArgs, err := parse.GitParseArgs(context.Background(), args)


will this try to clone the repository locally (or try to contact it in any way), or is this purely syntax-level parsing?

From what I can tell, GitParseArgs will try to contact the repository only if there is no ref/version provided in the URL. It tries to make contact in this case to determine what the default branch is.

What is the correct behavior for kpt alpha rpkg clone if no version is provided? I can extract the syntax-level parsing out of GitParseArgs and use that here instead if we want to do something different, like throw an error or leave the ref empty.

empty ref would be ok, or we can default to main. Porch defaults to main in other places where ref is not specified.

Error may be OK too.

I actually thought (though may be wrong) that another case where we contact the server is if the .git/ part is missing and the parser can't tell where the repository path ends and directory path begins:

domain.com/some/path/here

is the some/path repo? or some ?

regardless, the rpkg commands should not interact with the repository and only operate at the level of syntax of the URL provided, even if there are cases we can't handle and that will result in errors (IMO)

empty ref would be ok, or we can default to main. Porch defaults to main in other places where ref is not specified.

I'll default to main to be consistent with other places.

I actually thought (though may be wrong) that another case where we contact the server is if the .git/ part is missing and the parser can't tell where the repository path ends and directory path begins

Ah you are correct, I missed that it contacts the repo while attempting to convert the URL to a URL containing .git.

Is it a requirement for kpt alpha rpkg clone that the URL contains .git for git URLs? I'm thinking about moving the logic in GitParseArgs that handles URLs with a .git suffix out to its own helper function so that we can use it here without worrying about communication with the repo.

Presence of the .git part of the URL is not requirement. I'd say the requirement is that the parser doesn't modify the URL.

So if customer gave us https://github.com/org/repo.git/path/to/package
we should include the .git

if they provided https://github.com/org/repo/path/to/package
we should not artificially introduce .git part into the URL.

natasha41575 · 2022-05-04T01:15:25Z

internal/cmdrpkgclone/command.go

-	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.")


I removed these defaults because otherwise it is impossible to tell whether the user set the flag explicitly or not. Since we are trying to infer the ref from the URL, we need to be able to throw an error if the user sets the flag and also includes the ref in the URL and they contradict each other. I set the default manually below in L182-L187 after this error check occurs.

good call, thank you!

(some flag libraries have a way to tell whether a flag was explicitly set, but from top of my head I don't know if Cobra is one of them; this is a good approach though)

natasha41575 · 2022-05-04T01:19:15Z

e2e/testdata/porch/rpkg-clone/config.yaml

-      - https://github.com/platkrm/test-blueprints.git
-      - --directory=basens
-      - --ref=basens/v1
+      - https://github.com/platkrm/test-blueprints.git/basens@basens/v1


Updated this test to demonstrate the new inferring behavior. There is another similar command that I haven't modified in rpkg-edit/config.yaml, that uses the --directory and --ref flags, demonstrating that the user can still use the flags when needed.

thank you! this is great!

martinmaly · 2022-05-04T15:46:47Z

e2e/testdata/porch/rpkg-clone/config.yaml

-      - https://github.com/platkrm/test-blueprints.git
-      - --directory=basens
-      - --ref=basens/v1
+      - https://github.com/platkrm/test-blueprints.git/basens@basens/v1


thank you! this is great!

martinmaly · 2022-05-04T15:49:34Z

internal/cmdrpkgclone/command.go

+    * 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


@mortent is working on rationalizing all flags and group and command names in the alpha group; one thing I noticed (and it on me) that I picked suboptimal flag names for the clone command - there are 2 repositories involved (from, to) and the flag name is not directional (only --repository) so it may be confusing to customers. That said, let's not fix it here, just something to keep in mind for the big rename Morten is working on.

martinmaly · 2022-05-04T15:51:11Z

internal/cmdrpkgclone/command.go

-	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.")


good call, thank you!

(some flag libraries have a way to tell whether a flag was explicitly set, but from top of my head I don't know if Cobra is one of them; this is a good approach though)

martinmaly · 2022-05-04T16:02:48Z

internal/util/parse/parse.go

@@ -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 {


[nit] we are actually not looking for suffix per se, but (more accurately) checking for a URL path segment that ends with '.git'.

(and, I must admit I can't come up with a better name, so feel free to leave it :)

natasha41575 · 2022-05-04T17:13:56Z

Thanks for the reviews and answering all my questions!

natasha41575 changed the title ~~use kpt git parsing in porch~~ WIP: use kpt git parsing in porch May 2, 2022

natasha41575 force-pushed the urlparse branch from ee1baa8 to 4292628 Compare May 2, 2022 23:40

use kpt git parsing in porch

65e80ba

natasha41575 force-pushed the urlparse branch from 4292628 to 65e80ba Compare May 2, 2022 23:44

martinmaly reviewed May 2, 2022

View reviewed changes

keep .git suffix on repo

0f0923f

natasha41575 force-pushed the urlparse branch from e6d9ef1 to 5168f71 Compare May 4, 2022 01:09

natasha41575 commented May 4, 2022

View reviewed changes

keep flags as optional

f4729af

natasha41575 force-pushed the urlparse branch from 5168f71 to f4729af Compare May 4, 2022 01:17

natasha41575 commented May 4, 2022

View reviewed changes

fix test failure

6256c49

natasha41575 marked this pull request as ready for review May 4, 2022 15:45

natasha41575 requested review from martinmaly and yuwenma May 4, 2022 15:45

natasha41575 changed the title ~~WIP: use kpt git parsing in porch~~ use kpt git parsing in rpkg clone May 4, 2022

martinmaly approved these changes May 4, 2022

View reviewed changes

natasha41575 merged commit 9a8998a into kptdev:main May 4, 2022

natasha41575 deleted the urlparse branch May 4, 2022 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use kpt git parsing in `rpkg clone` #3080

use kpt git parsing in `rpkg clone` #3080

natasha41575 commented May 2, 2022 •

edited

Loading

martinmaly May 2, 2022

natasha41575 May 3, 2022 •

edited

Loading

martinmaly May 3, 2022

natasha41575 May 3, 2022 •

edited

Loading

martinmaly May 3, 2022

natasha41575 May 4, 2022

martinmaly May 4, 2022

natasha41575 May 4, 2022 •

edited

Loading

martinmaly May 4, 2022

martinmaly May 4, 2022

martinmaly May 4, 2022

martinmaly May 4, 2022

martinmaly May 4, 2022

natasha41575 commented May 4, 2022

		* 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

use kpt git parsing in rpkg clone #3080

use kpt git parsing in rpkg clone #3080

Conversation

natasha41575 commented May 2, 2022 • edited Loading

Choose a reason for hiding this comment

natasha41575 May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 commented May 4, 2022

use kpt git parsing in `rpkg clone` #3080

use kpt git parsing in `rpkg clone` #3080

natasha41575 commented May 2, 2022 •

edited

Loading

natasha41575 May 3, 2022 •

edited

Loading

natasha41575 May 3, 2022 •

edited

Loading

natasha41575 May 4, 2022 •

edited

Loading