Refactor parseGitURL by KnVerey · Pull Request #4983 · kubernetes-sigs/kustomize

KnVerey · 2023-01-14T05:40:25Z

Part of #4866
Refactors the code that splits the RepoPath from the KustRootPath, and fixes inconsistencies and bugs in doing so:

Uses a common path splitting strategy for all URLs--no more completely separate parsing for .git, _git, local etc.
Eliminates the special case where URLs containing _git didn't even use extractHost
Makes the documented // delimiter work for all URLs (we had skipped tests showing this was broken in some cases)
Removes the pseudo-validation of the path segment using url.Parse (see this inline comment)

TIL while working on PR that the error messages NewRepoSpecFromURL never seem to make it to the user AFAICT. Instead, the return of an error is interpreted as "this is probably not a URL at all". For example,(fl *fileLoader) New then tries to load it as a local file, and (fl *fileLoader) httpClientGetContent returns an HTTP error instead of a "this was a remote repo" error. Stuff like this is (part of) why our error messages are so terrible. This PR does not attempt to change this, but the unusual implication of an error return value is good to keep in mind for review.

Remaining follow-ups:

(fairly simple) Stop messing with the conventional .git suffix entirely (originally included here but removed for clarity: see this comment)
(simple) Add arbitrary username support
(unlikely to make the release) Fix the overloading of the error return value? "This URL is invalid" (what we're giving) is not the same as "This doesn't look like a URL" (what the callers seem to need). Could the callers be using something that just validates the URL generically? I'd like to experiment with a dedicated function that does that, and compare it to the results of our error and smoke RepoSpec tests. I think this has a big potential for impact on Kustomize's most confusing error messages.

k8s-ci-robot · 2023-01-14T05:40:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

KnVerey · 2023-01-14T06:01:19Z


-func parsePath(n string) string {
-	parsed, err := url.Parse(n)
-	// TODO(annasong): decide how to handle error, i.e. return error, empty string, etc.


This PR is suggesting removing this parsing entirely. I noticed that no tests fail if we do so, and so I was wondering what this is intended to catch. I looked back in the history, and it was introduced by #3900. IIUC that PR intended it as a more reliable way (vs the former regex) to separate query parts from the path. We still use it for that today; since #4863 we just do it a bit earlier in the parsing flow. At that earlier point, we're passing the whole raw value to the parser, whereas originally (in valid cases anyway) we were passing just the kust root path + query. So the Path value we would get back at our new invocation is not usable.

The question is, do we then need to continue to put the path through the URL parser at the end of the parsing flow, now on its own (the query long since removed)? I don't think so, because IIUC the original intent was not to validate the path; it was just convenient to take the value returned when we separated a query from it. If you go back one more step in the history, peelQuery was just splitting a query chunk off of n when present.

This also relates to the stance I explained on my last PR re: passing things through to git if they're not interfering with our ability to parse out the repo root. In this case, we've necessarily already done that.

LMK if you agree.

KnVerey · 2023-01-14T06:08:09Z

+	}
+	var host, rest = n, ""
+	if sepIndex := findPathSeparator(n, acceptSCP); sepIndex > 0 {
+		host, rest = n[:sepIndex+1], n[sepIndex+1:]


Before, when we didn't find a separator, the whole thing was considered a path. Now it's considered a host. I think this makes more sense, and it maintains the current error messages in some cases. Notably, ssh://git.example.com should complain about orgRepo, not host, missing. Previously all parsing would complete before any errors would return, and for whatever reason orgRepo was checked first, resulting in the correct message for that case despite host also being empty.

k8s-ci-robot · 2023-01-16T22:17:06Z

@KnVerey: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

KnVerey · 2023-01-16T22:44:25Z


-	if strings.Contains(n, gitDelimiter) {
-		index := strings.Index(n, gitDelimiter)
-		// Adding _git/ to host


I don't know why it was originally added to the host segment in #1271. It's part of the path to to the repo, not the host (our new variable names are nice here!). At the end of the day, correcting this doesn't affect the clone spec.

KnVerey · 2023-01-17T00:07:00Z

-	if pathElements := strings.Split(cleanedPath, string(filepath.Separator)); len(pathElements) > 0 &&
-		pathElements[0] == filesys.ParentDir {
-		return nil, fmt.Errorf("url path exits repo: %s", n)
-	}


I initially only moved the RepoPath and Host validations into their respective parser methods, but then I noticed that this one is effectively just a KustRootPath validation, so I moved it too. Then all I had left here was the IsAbs check, so I deleted this and renamed parseGitURL to NewRepoSpecFromURL (FYI because that's not what it looks like on Github).

KnVerey · 2023-01-17T01:38:13Z

-// the parts.
+// the different parts of URL, set into a RepoSpec object and return RepoSpec object.
+// It MUST return an error if the input is not a git-like URL, as this is used by some code paths
+// to distinguish between local and remote paths.


As of today, this seems to be the ONLY thing the errors are used for: to judge whether the string is a URL.

KnVerey · 2023-01-17T01:48:05Z

 				Host:         "[email protected]:",
 				RepoPath:     "company/project",
-				KustRootPath: "/path",
+				KustRootPath: "path",


I don't think it matters whether we end up with a leading slash here, but previously we were inconsistent. Github URLs (which generally are best tested/supported) did not have the prefix previously, so I took them all in that direction.

KnVerey · 2023-01-17T01:51:33Z

 			},
 		},
-		{
-			name:      "unsupported protocol after username (invalid but currently passed through to git)",


This is a behaviour change vs before all our refactoring, though at the end of the day this URL could never work. This PR would require adding code to continue accepting this particular input, and since it isn't valid anyway, I didn't think it was worthwhile.

KnVerey · 2023-01-17T01:53:09Z

-			name: "triple slash absolute path",
-			kustomization: `
-resources:
- file:///$ROOT/simple.git


It turns out $ROOT already contains a slash, so this test case used four slashes not three, and that's not valid.

koba1t · 2023-01-17T09:54:18Z

 		{
-			name:      "t8",
+			name:      "_git host delimiter in non-github url",
 			input:     "https://itfs.mycompany.com/collection/project/_git/somerepos",


mycompany.com is already a used domain by an irrelevant company.
https://www.whois.com/whois/mycompany.com

Can we use a better example domain like example.com?
https://www.rfc-editor.org/rfc/rfc2606#section-3

That's a good callout, and thank you for providing the references. TIL about that RFC! In this case, the test suite is just for the parsing step, so there's no chance of us actually trying to use these URLs--the names are for use case illustration purposes only. It uses a variety of invalid/irrelevant domains currently, and I think I'll leave them alone for the purposes of this PR.

koba1t · 2023-01-17T09:56:17Z

+			name:      "double-slash path delimiter https",
 			input:     "https://fake-git-hosting.org/path/to/repo//examples/multibases/dev",
-			cloneSpec: "https://fake-git-hosting.org/path/to.git",
+			cloneSpec: "https://fake-git-hosting.org/path/to/repo.git",


I think fake-git-hosting.org needs to change too.

koba1t · 2023-01-17T10:31:01Z

I think this PR looks good!
I comment on a few nit points. If you don't think need to fix that, please ignore that comments

natasha41575

Very cleanly and nicely done! Thanks. I see @koba1t left some comments, so I'm adding a hold so you can take a look at those comments. Feel free to remove the hold when you want to merge.

/lgtm
/hold

k8s-ci-robot · 2023-01-17T20:17:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, natasha41575

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [KnVerey,natasha41575]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

KnVerey · 2023-01-18T01:14:02Z

/unhold

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2023

k8s-ci-robot requested review from annasong20 and yuwenma January 14, 2023 05:40

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2023

KnVerey commented Jan 14, 2023

View reviewed changes

KnVerey commented Jan 16, 2023

View reviewed changes

Comment thread api/internal/git/repospec.go Resolved

KnVerey commented Jan 16, 2023

View reviewed changes

KnVerey force-pushed the repospec_separators branch from 8e0e801 to 4d8aed0 Compare January 17, 2023 00:04

KnVerey commented Jan 17, 2023

View reviewed changes

KnVerey force-pushed the repospec_separators branch from 4d8aed0 to abbeb64 Compare January 17, 2023 00:08

Refactor parseGitURL to consolidate code paths, improve error handling

0020839

KnVerey force-pushed the repospec_separators branch from abbeb64 to 0020839 Compare January 17, 2023 01:36

KnVerey commented Jan 17, 2023

View reviewed changes

KnVerey changed the title ~~Refactor parseGitURL to consolidate code paths, improve error handling~~ Refactor parseGitURL Jan 17, 2023

KnVerey marked this pull request as ready for review January 17, 2023 02:18

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2023

k8s-ci-robot requested review from koba1t and natasha41575 January 17, 2023 02:18

koba1t reviewed Jan 17, 2023

View reviewed changes

natasha41575 approved these changes Jan 17, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2023

k8s-ci-robot assigned natasha41575 Jan 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2023

k8s-ci-robot merged commit dbc11a4 into kubernetes-sigs:master Jan 18, 2023

KnVerey deleted the repospec_separators branch January 18, 2023 01:43

coderabbitai Bot mentioned this pull request Jun 9, 2026

feat(platform): migrate to etcd-operator v1alpha2 cozystack/cozystack#2859

Merged

Conversation

KnVerey commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 16, 2023

Uh oh!

KnVerey Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koba1t commented Jan 17, 2023

Uh oh!

natasha41575 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 17, 2023

Uh oh!

KnVerey commented Jan 18, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KnVerey commented Jan 14, 2023 •

edited

Loading

KnVerey Jan 16, 2023 •

edited

Loading

natasha41575 left a comment •

edited

Loading