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
mafredri
left a comment
There was a problem hiding this comment.
Nice improvement! I'm however a bit concerned about the potentially brittle parsing.
git/git.go
Outdated
|
|
||
| _, 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.
3 participants
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.