Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Iteration on parseHostSpec refactor #4954
Iteration on parseHostSpec refactor #4954
Changes from all commits
fbb9458
dc05fa1
7e6cc7a
4821259
43d96ba
1501347
2591303
0699946
64d2366
ddf14ea
240282f
1a201ab
a885ee1
c288564
3134e9b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return
-1
as the index otherwise? SCP-like urls don't accept/
as a separator even if there isn't a:
(not preceded by a/
).git fetch
will fail for such a case.The error message here: https://github.com/kubernetes-sigs/kustomize/pull/4954/files#diff-2a7284bc651dfb0465d49f2270ed590fce191d14bba769751e4d16d9184a198cR97 should be
url lacks host
.This change will require some accompanying changes in
extractHost
forgit.luolix.top/path/to/repo.git
to pass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When thinking about this and the question of whether to throw an error on http(s)+username, I came back to this question: what is the point of all this parsing code? In the end, we have a URL that we're going to pass to git, and it will throw an error. Why don't we always let it do its job?
I wasn't here for the original authorship, but my answer would be: because we support some invalid formats (e.g. the
git::
prefix we strip, github URLs we "fix") and the URLs we are given ultimately point to a Kustomization root, whereas git needs the repo root. So the point is to identify the repo root, i.e the clonespec, and to fix any oddities we specifically allow for historical reasons.That led me to conclude that we should only throw errors in this code when the anomaly prevents us from achieving those two goals. When we are able to parse the intended clone spec (valid or not), we should do so, and let git throw the error (if any).
With that in mind, I think it is clear that the http(s)+username case should be allowed to fall through to git, and I've restored that behaviour. I think that is also true for the case you're highlighting here: we've found a schemeless, non-Github url that starts with a username and is missing the colon that should delimit the host. The following test passes on master, and currently on this branch:
I think we should continue to let it pass though.
After reading the above, do you agree? Are there other cases to consider around SCP-like with missing colons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense! Thank you for writing such a great explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially hoped looking for
:
before/
would decrease our chances of mistaking a relative local file path that contains an@
in the first directory for a repo url, but I realized that users can prefix such file paths with.
to look like./path
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, and also the "a relative local file path that contains an @ in the first directory" scenario is likely vanishingly rare, so I'm not too worried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concern for future PR: I find
normalizeGitHostSpec
hard to reason about because it checks for target strings out of context (withstrings.Contains
). For example, the repo urlssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git
with the following use case: https://docs.github.com/en/authentication/troubleshooting-ssh/using-ssh-over-the-https-portwould be normalized to
git@github.com:YOUR-USERNAME/YOUR-REPOSITORY.git
, which might come as a surprise to the user.Maybe we could reuse some of the logic in the existing
extractHost
to replace the call tonormalizeGitHostSpec
inparseGitURL
.At the very least, I'm glad we no longer use this function in
extractHost
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my next PR is getting rid of this entirely in favour of handling all host parsing with
extractHost
! That's a great example of changed behaviour in this PR though, and I added it as a test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One changed code path is that
gh:
could previously end up here, sogh:github.com/org/repo/foo
would get normalized... but that's non-sensical based on Anna's comment thatgh:
is assumed to be a gitconfig shorthand that gets swapped for the github URL, right? (same withfile://
, which is definitely non-sensical)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote that comment to the best of my knowledge. I couldn't find any official documentation for
gh:
on the internet, other than Github CLI. I also tried looking at code history to no avail. The only sites it popped up on denoted a.gitconfig
shorthand: https://gist.github.com/lauhakari/a48a94b60dc66f6c84e2#file-gitconfig-L143There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. What a mystery! I tried looking at the original PR too, and it doesn't help much... but it definitely shows the "url" is expected to look like
gh:org/repo
. So I think it is reasonable to considergh:github.com
non-sensical.Alternatively, maybe we can remove support for this. Your research indicates this is not a standard, and we do not have it documented anywhere. Furthermore, I don't see ANY usage of it in public Kustomizations on Github: https://github.com/search?q=%22gh%22+filename%3AKustomization.yaml&type=Code&ref=advsearch&l=&l=.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be down to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done