Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Iteration on parseHostSpec refactor #4954

Merged
merged 15 commits into from
Jan 10, 2023
Merged

Iteration on parseHostSpec refactor #4954

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fbb9458
Refactor parseHostSpec
annasong20 Dec 15, 2022
dc05fa1
Additional repospec refactoring
KnVerey Jan 3, 2023
7e6cc7a
Review feedback
KnVerey Jan 5, 2023
4821259
Stop handling mysterious gh: prefix in remote URLs
KnVerey Jan 6, 2023
43d96ba
Address feedback on correctness of SCP/username validations
KnVerey Jan 6, 2023
1501347
Show change vs master on accepted but invalid ssh-like urls
KnVerey Jan 6, 2023
2591303
Add test case from github docs
KnVerey Jan 7, 2023
0699946
Make Github less special
KnVerey Jan 7, 2023
64d2366
Naming changes from rebase
KnVerey Jan 9, 2023
ddf14ea
Deprecate git:: stripping
KnVerey Jan 9, 2023
240282f
Allow file paths that look like usernames
KnVerey Jan 9, 2023
1a201ab
do not validate against http+usernames after all
KnVerey Jan 9, 2023
a885ee1
Add test for behaviour of scp-like non-github that incorrectly uses s…
KnVerey Jan 9, 2023
c288564
Remove unused param
KnVerey Jan 10, 2023
3134e9b
Better SCP colon-must-be-after-slash test
KnVerey Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
170 changes: 133 additions & 37 deletions api/internal/git/repospec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package git

import (
"fmt"
"log"
"net/url"
"path/filepath"
"strconv"
Expand Down Expand Up @@ -128,7 +129,7 @@ func parseGitURL(n string) *RepoSpec {
repoSpec.KustRootPath = parsePath(n[index+len(gitDelimiter)+len(repoSpec.RepoPath):])
return repoSpec
}
repoSpec.Host, n = parseHostSpec(n)
repoSpec.Host, n = extractHost(n)
isLocal := strings.HasPrefix(repoSpec.Host, "file://")
if !isLocal {
repoSpec.GitSuffix = gitSuffix
Expand Down Expand Up @@ -227,56 +228,151 @@ func parsePath(n string) string {
return parsed.Path
}

func parseHostSpec(n string) (string, string) {
var host string
// Start accumulating the host part.
for _, p := range []string{
// Order matters here.
"git::", "gh:", "ssh://", "https://", "http://", "file://",
"git@", "github.com:", "github.com/"} {
if len(p) < len(n) && strings.ToLower(n[:len(p)]) == p {
n = n[len(p):]
host += p
func extractHost(n string) (string, string) {
n = ignoreForcedGitProtocol(n)
scheme, n := extractScheme(n)
username, n := extractUsername(n)
stdGithub := isStandardGithubHost(n)
acceptSCP := acceptSCPStyle(scheme, username, stdGithub)

// Validate the username and scheme before attempting host/path parsing, because if the parsing
// so far has not succeeded, we will not be able to extract the host and path correctly.
if err := validateUsernameAndScheme(username, scheme, acceptSCP); err != nil {
// TODO: return this error instead.
KnVerey marked this conversation as resolved.
Show resolved Hide resolved
return "", n
}

// Now that we have extracted a valid scheme+username, we can parse host itself.

// The file protocol specifies an absolute path to a local git repo. There is no host.
if scheme == "file://" {
return scheme, n
}
sepIndex := findPathSeparator(n, acceptSCP)
host, rest := n[:sepIndex+1], n[sepIndex+1:]

// Github URLs are strictly normalized in a way that may discard scheme and username components.
if stdGithub {
scheme, username, host = normalizeGithubHostParts(scheme, username)
}

// Host is required, so do not concat the scheme and username if we didn't find one.
if host == "" {
// TODO: This should return an error.
return "", n
}
return scheme + username + host, rest
}

// ignoreForcedGitProtocol strips the "git::" prefix from URLs.
// We used to use go-getter to handle our urls: https://github.com/hashicorp/go-getter.
// The git:: prefix signaled go-getter to use the git protocol to fetch the url's contents.
// We silently strip this prefix to allow these go-getter-style urls to continue to work,
// although the git protocol (which is insecure and unsupported on many platforms, including Github)
// will not actually be used as intended.
func ignoreForcedGitProtocol(n string) string {
n, found := trimPrefixIgnoreCase(n, "git::")
if found {
log.Println("Warning: Forcing the git protocol using the 'git::' URL prefix is not supported. " +
"Kustomize currently strips this invalid prefix, but will stop doing so in a future release. " +
"Please remove the 'git::' prefix from your configuration.")
}
return n
}

// acceptSCPStyle returns true if the scheme and username indicate potential use of an SCP-style URL.
// With this style, the scheme is not explicit and the path is delimited by a colon.
// Strictly speaking the username is optional in SCP-like syntax, but Kustomize has always
// required it for non-Github URLs.
// Example: user@host.xz:path/to/repo.git/
func acceptSCPStyle(scheme, username string, isGithubURL bool) bool {
return scheme == "" && (username != "" || isGithubURL)
}

func validateUsernameAndScheme(username, scheme string, acceptSCPStyle bool) error {
// see https://git-scm.com/docs/git-fetch#_git_urls for info relevant to these validations
switch scheme {
case "":
// Empty scheme is only ok if it's a Github URL or if it looks like SCP-style syntax
if !acceptSCPStyle {
return fmt.Errorf("no username or scheme found")
}
case "ssh://":
// usernames are optional for ssh protocol
return nil
case "https://", "http://", "file://":
// usernames are not supported by http or file protocols
if username != "" {
return fmt.Errorf("username %q specified, but %s does not support usernames", username, scheme)
}
default:
// At time of writing, we should never end up here because we do not parse out
// unsupported schemes to begin with.
return fmt.Errorf("unsupported scheme %q", scheme)
}
if host == "git@" {
KnVerey marked this conversation as resolved.
Show resolved Hide resolved
i := strings.Index(n, "/")
if i > -1 {
host += n[:i+1]
n = n[i+1:]
} else {
i = strings.Index(n, ":")
if i > -1 {
host += n[:i+1]
n = n[i+1:]
}
return nil
}

func extractScheme(s string) (string, string) {
for _, prefix := range []string{"ssh://", "https://", "http://", "file://"} {
if rest, found := trimPrefixIgnoreCase(s, prefix); found {
return prefix, rest
}
return host, n
}
return "", s
}

func extractUsername(s string) (string, string) {
KnVerey marked this conversation as resolved.
Show resolved Hide resolved
if trimmed, found := trimPrefixIgnoreCase(s, gitUsername); found {
return gitUsername, trimmed
}
return "", s
}

// If host is a http(s) or ssh URL, grab the domain part.
for _, p := range []string{
"ssh://", "https://", "http://"} {
if strings.HasSuffix(host, p) {
i := strings.Index(n, "/")
if i > -1 {
host += n[0 : i+1]
n = n[i+1:]
}
break
func isStandardGithubHost(s string) bool {
lowerCased := strings.ToLower(s)
return strings.HasPrefix(lowerCased, "github.com/") ||
strings.HasPrefix(lowerCased, "github.com:")
}

// trimPrefixIgnoreCase returns the rest of s and true if prefix, ignoring case, prefixes s.
// Otherwise, trimPrefixIgnoreCase returns s and false.
func trimPrefixIgnoreCase(s, prefix string) (string, bool) {
if len(prefix) <= len(s) && strings.ToLower(s[:len(prefix)]) == prefix {
return s[len(prefix):], true
}
return s, false
}

func findPathSeparator(hostPath string, acceptSCP bool) int {
sepIndex := strings.Index(hostPath, "/")
if acceptSCP {
// The colon acts as a delimiter in scp-style ssh URLs only if not prefixed by '/'.
if colonIndex := strings.Index(hostPath, ":"); colonIndex > 0 && colonIndex < sepIndex {
sepIndex = colonIndex
}
Comment on lines +346 to 348
Copy link
Contributor

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 for github.com/path/to/repo.git to pass.

Copy link
Contributor Author

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:

		{
			name:      "non-github_scp incorrectly using slash (invalid but currently passed through to git)",
			input:     "git@bitbucket.org/company/project.git//path?ref=branch",
			cloneSpec: "git@bitbucket.org/company/project.git",
			absPath:   notCloned.Join("path"),
			repoSpec: RepoSpec{
				Host:         "git@bitbucket.org/",
				RepoPath:     "company/project",
				KustRootPath: "/path",
				Ref:          "branch",
				GitSuffix:    ".git",
			},
		},

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?

Copy link
Contributor

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!

Copy link
Contributor

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.

Copy link
Contributor Author

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.

}
return sepIndex
}

const normalizedHTTPGithub = "https://github.com/"
const gitUsername = "git@"
const normalizedSCPGithub = gitUsername + "github.com:"

return normalizeGitHostSpec(host), n
func normalizeGithubHostParts(scheme, username string) (string, string, string) {
if strings.HasPrefix(scheme, "ssh://") || username != "" {
return "", username, "github.com:"
}
return "https://", "", "github.com/"
}

func normalizeGitHostSpec(host string) string {
Copy link
Contributor

@annasong20 annasong20 Jan 6, 2023 •
edited

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 (with strings.Contains). For example, the repo url ssh://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-port

would 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 to normalizeGitHostSpec in parseGitURL.

At the very least, I'm glad we no longer use this function in extractHost.

Copy link
Contributor Author

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.

s := strings.ToLower(host)
if strings.Contains(s, "github.com") {
Copy link
Contributor Author

@KnVerey KnVerey Jan 5, 2023 •
edited

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, so gh:github.com/org/repo/foo would get normalized... but that's non-sensical based on Anna's comment that gh: is assumed to be a gitconfig shorthand that gets swapped for the github URL, right? (same with file://, which is definitely non-sensical)

Copy link
Contributor

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-L143

Copy link
Contributor Author

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 consider gh: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=.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if strings.Contains(s, "git@") || strings.Contains(s, "ssh:") {
host = "git@github.com:"
if strings.Contains(s, gitUsername) || strings.Contains(s, "ssh:") {
host = normalizedSCPGithub
} else {
host = "https://github.com/"
host = normalizedHTTPGithub
}
}
if strings.HasPrefix(s, "git::") {
Expand Down
105 changes: 97 additions & 8 deletions api/internal/git/repospec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ func TestNewRepoSpecFromUrl_Permute(t *testing.T) {
// we probably stil don't want to break backwards compatibility for things
// that are unintentionally supported.
var schemeAuthority = []struct{ raw, normalized string }{
{"gh:", "gh:"},
{"GH:", "gh:"},
{"gitHub.com/", "https://github.com/"},
{"github.com:", "https://github.com/"},
{"http://github.com/", "https://github.com/"},
Expand All @@ -34,6 +32,7 @@ func TestNewRepoSpecFromUrl_Permute(t *testing.T) {
{"git::https://git.example.com/", "https://git.example.com/"},
{"git@github.com:", "git@github.com:"},
{"git@github.com/", "git@github.com:"},
{"git::git@github.com:", "git@github.com:"},
}
var repoPaths = []string{"someOrg/someRepo", "kubernetes/website"}
var pathNames = []string{"README.md", "foo/krusty.txt", ""}
Expand Down Expand Up @@ -81,6 +80,10 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) {
"/tmp",
"uri looks like abs path",
},
"relative path": {
"../../tmp",
"url lacks host",
},
"no_slashes": {
"iauhsdiuashduas",
"url lacks repoPath",
Expand All @@ -89,6 +92,10 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) {
"htxxxtp://github.com/",
"url lacks host",
},
"bad_scp": {
"git@local/path:file/system",
"url lacks repoPath",
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into TestNewRepoSpecFromUrl_Smoke? This test originally checked that extractHost failed on not being able to find a host delimiter because : is behind a /. Now that we use / as the delimiter instead, this test no longer makes sense.

If we still want to document the behavior that we only detect :s not preceded by /, the TestNewRepoSpecFromUrl_Smoke test would look something like this:

		{
			name:      "colon behind slash not scp delimiter",
			input:     "git@gitlab.com/user:name/YOUR-REPOSITORY.git",
			cloneSpec: "git@gitlab.com",
			absPath:   notCloned.String(),
			repoSpec: RepoSpec{
				Host:      "git@gitlab.com/",
				OrgRepo:   "user:name/YOUR-REPOSITORY",
				Path:      "",
				GitSuffix: ".git",
			},
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that's much better! Updated.

"no_org_repo": {
"ssh://git.example.com",
"url lacks repoPath",
Expand All @@ -105,6 +112,26 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) {
"https://github.com/org/repo.git//path/../../exits/repo",
"url path exits repo",
},
"bad github separator": {
"github.com!org/repo.git//path",
"url lacks host",
},
"mysterious gh: prefix previously supported is no longer handled": {
"gh:org/repo",
"url lacks repoPath",
},
"username unsupported with http": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that all three of these would be accepted on master, so the errors here are a behaviour change. I'm not 100% sure about this yet; will ponder more before we merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it helps, I believe file git urls are technically allowed to have @ in their repo paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I updated the code and tests to allow it. As commented elsewhere, I also decided against this validation more generally.

"http://git@foo.com/path/to/repo",
"url lacks host",
},
"username unsupported with https": {
"https://git@foo.com/path/to/repo",
"url lacks host",
},
"username unsupported with file": {
"file://git@/path/to/repo",
"url lacks repoPath",
},
}

for name, testCase := range badData {
Expand Down Expand Up @@ -191,24 +218,37 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) {
},
{
name: "t6",
input: "git@gitlab2.sqtools.ru:10022/infra/kubernetes/thanos-base.git?ref=v0.1.0",
cloneSpec: "git@gitlab2.sqtools.ru:10022/infra/kubernetes/thanos-base.git",
input: "git@gitlab2.sqtools.ru:infra/kubernetes/thanos-base.git?ref=v0.1.0",
cloneSpec: "git@gitlab2.sqtools.ru:infra/kubernetes/thanos-base.git",
absPath: notCloned.String(),
repoSpec: RepoSpec{
Host: "git@gitlab2.sqtools.ru:10022/",
Host: "git@gitlab2.sqtools.ru:",
RepoPath: "infra/kubernetes/thanos-base",
Ref: "v0.1.0",
GitSuffix: ".git",
},
},
{
name: "t7",
name: "non-github_scp",
input: "git@bitbucket.org:company/project.git//path?ref=branch",
cloneSpec: "git@bitbucket.org:company/project.git",
absPath: notCloned.Join("path"),
repoSpec: RepoSpec{
Host: "git@bitbucket.org:company/",
RepoPath: "project",
Host: "git@bitbucket.org:",
RepoPath: "company/project",
KustRootPath: "/path",
Ref: "branch",
GitSuffix: ".git",
},
},
{
name: "non-github_git-user_ssh",
input: "ssh://git@bitbucket.org/company/project.git//path?ref=branch",
cloneSpec: "ssh://git@bitbucket.org/company/project.git",
absPath: notCloned.Join("path"),
repoSpec: RepoSpec{
Host: "ssh://git@bitbucket.org/",
RepoPath: "company/project",
KustRootPath: "/path",
Ref: "branch",
GitSuffix: ".git",
Expand Down Expand Up @@ -459,6 +499,55 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) {
GitSuffix: ".git",
},
},
{
name: "non-git username with non-github host",
input: "ssh://myusername@bitbucket.org/ourteamname/ourrepositoryname.git//path?ref=branch",
cloneSpec: "ssh://myusername@bitbucket.org/ourteamname/ourrepositoryname.git",
absPath: notCloned.Join("path"),
repoSpec: RepoSpec{
Host: "ssh://myusername@bitbucket.org/",
RepoPath: "ourteamname/ourrepositoryname",
KustRootPath: "/path",
Ref: "branch",
GitSuffix: ".git",
},
},
{
name: "unsupported protocol after username (invalid and will be rejected by git)",
input: "git@scp://github.com/org/repo.git//path",
cloneSpec: "git@scp://github.com/org/repo.git",
absPath: notCloned.Join("path"),
repoSpec: RepoSpec{
Host: "git@scp:",
RepoPath: "//github.com/org/repo",
KustRootPath: "/path",
GitSuffix: ".git",
},
},
{
name: "supported protocol after username (invalid and will be rejected by git)",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two cases have slightly different parsing results on master. Basically a slash moves. But the clonespec stays the same, and git will reject it anyway, so I don't think this matters a ton.

		{
			name:      "unsupported protocol after username",
			input:     "git@scp://github.com/org/repo.git//path",
			cloneSpec: "git@scp://github.com/org/repo.git",
			absPath:   notCloned.Join("path"),
			repoSpec: RepoSpec{
				Host:      "git@scp:/",
				OrgRepo:   "/github.com/org/repo",
				Path:      "/path",
				GitSuffix: ".git",
			},
		},
		{
			name:      "supported protocol after username",
			input:     "git@ssh://github.com/org/repo.git//path",
			cloneSpec: "git@ssh://github.com/org/repo.git",
			absPath:   notCloned.Join("path"),
			repoSpec: RepoSpec{
				Host:      "git@ssh:/",
				OrgRepo:   "/github.com/org/repo",
				Path:      "/path",
				GitSuffix: ".git",
			},
		},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not request for change: This test case made me ponder the prioritization of delimiters in parseGitURL. Currently, we always prioritize .git over //. Given that we advertise // as the repo path delimiter, we could alternatively look for .git, only if it isn't preceded by //. Otherwise, we could also document the .git delimiter more clearly in https://github.com/kubernetes-sigs/kustomize/blob/master/examples/remoteBuild.md.

Regardless, this'd be work for a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my next PR is tackling parseGitURL, so I'll consider this then.

input: "git@ssh://github.com/org/repo.git//path",
cloneSpec: "git@ssh://github.com/org/repo.git",
absPath: notCloned.Join("path"),
repoSpec: RepoSpec{
Host: "git@ssh:",
RepoPath: "//github.com/org/repo",
KustRootPath: "/path",
GitSuffix: ".git",
},
},
{
name: "complex github ssh url from docs",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running this on master shows that the current result is even worse than you thought. Here's what passes there:

		{
			name:      "complex github ssh url from docs",
			input:     "ssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git",
			cloneSpec: "ssh://git@ssh.git",
			absPath:   notCloned.String(),
			repoSpec: RepoSpec{
				Host:      "ssh://git@",
				OrgRepo:   "ssh",
				Path:      "",
				GitSuffix: ".git",
			},
		},

Totally mangled clonespec!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future PR: Maybe we could check that delimiters like .git are succeeded by a /: https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/git/repospec.go#L136? I believe this test case reveals that we're looking for such delimiters out of context.

input: "ssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git",
cloneSpec: "ssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git",
absPath: notCloned.String(),
repoSpec: RepoSpec{
Host: "ssh://git@ssh.github.com:443/",
RepoPath: "YOUR-USERNAME/YOUR-REPOSITORY",
KustRootPath: "",
GitSuffix: ".git",
},
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
Expand Down
4 changes: 0 additions & 4 deletions api/internal/localizer/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,6 @@ func TestLocRootPath_URLComponents(t *testing.T) {
urlf: "file:///var/run/repo//%s?ref=value",
path: simpleJoin(t, FileSchemeDir, "var", "run", "repo", "value"),
},
"gh_shorthand": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing this!

urlf: "gh:org/repo//%s?ref=value",
path: simpleJoin(t, "gh", "org", "repo", "value"),
},
"IPv6": {
urlf: "https://[2001:4860:4860::8888]/org/repo//%s?ref=value",
path: simpleJoin(t, "2001:4860:4860::8888", "org", "repo", "value"),
Expand Down