Uh oh!
There was an error while loading. Please reload this page.
fix: improve git URL parsing - #486
Conversation
| require.NoError(t, err) | ||
| dockerFilePath := execContainer(t, ctr, "find /workspaces -name Dockerfile") | ||
| require.NotEmpty(t, dockerFilePath) | ||
| dockerFile := execContainer(t, ctr, "cat "+dockerFilePath) | ||
| require.Contains(t, dockerFile, testImageAlpine) |
| require.NoError(t, err) | ||
| require.True(t, cloned) | ||
| require.Equal(t, "Hello, world!", mustRead(t, clientFS, "/workspace/README.md")) |
| addr, ok := l.Addr().(*net.TCPAddr) | ||
| require.True(t, ok) | ||
| tr, err := transport.NewEndpoint(fmt.Sprintf("ssh://git@%s:%d/", addr.IP, addr.Port)) | ||
| tr, err := transport.NewEndpoint(fmt.Sprintf("ssh://git@%s:%d%s", addr.IP, addr.Port, fs.Root())) |
There was a problem hiding this comment.
I think previous me was to blame for this, for which I humbly apologise.
There was a problem hiding this comment.
I didn't add new tests for this function as I figured the existing tests should suffice.
mafredri
left a comment
There was a problem hiding this comment.
Nice improvement! I'm however a bit concerned about the potentially brittle parsing.
| _, err = git.CloneContext(ctx, gitStorage, fs, &git.CloneOptions{ | ||
| URL: parsed.String(), | ||
| URL: opts.RepoURL, // Use the EXACT URL provided. |
There was a problem hiding this comment.
Do we have tests to verify that whitespace won't cause issues here for go-git?
There was a problem hiding this comment.
Leading and trailing whitespace can cause issues, but whitespace in the URL itself appears to be handled 'correctly'. I've added some tests + added logic to return the "cleaned" URL (with whitespace and #ref trimmed) in 3618988.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mafredri
left a comment
There was a problem hiding this comment.
Looks good, only one remaining question about expanding the sample formats in docs but otherwise this new approach is much better!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Relates to #485
@bjornrobertsson notified me of some issues with parsing
ssh://URLs.The root of the issue is that we were using
github.com/chainguard-dev/git-urlsto do our URL validation, and notgo-git's own validation library.This PR removes the dependency on
git-urlsand usesgo-git'stransport.NewEndpointinstead, with some additional massaging to keep our current behaviour in line.Other changes:
gittestthat was causing us to be unable to properly test SSH clones.log.