From 1dbab823d7149432ab00c2099b22b4eecb969a05 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 14 May 2024 12:58:19 +0000 Subject: [PATCH 1/5] fix: fix git url parsing --- envbuilder.go | 6 +++--- envbuilder_test.go | 35 +++++++++++++++++++++++++++++------ git.go | 8 +++++++- git_test.go | 29 +++++++++++++++++++++++++++++ go.mod | 1 + go.sum | 2 ++ 6 files changed, 71 insertions(+), 10 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 0c2bd2a0..edfa79f3 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -14,7 +14,6 @@ import ( "maps" "net" "net/http" - "net/url" "os" "os/exec" "os/user" @@ -859,12 +858,13 @@ func DefaultWorkspaceFolder(repoURL string) (string, error) { if repoURL == "" { return "/workspaces/empty", nil } - parsed, err := url.Parse(repoURL) + parsed, err := ParseGitURL(repoURL) if err != nil { return "", err } name := strings.Split(parsed.Path, "/") - return fmt.Sprintf("/workspaces/%s", name[len(name)-1]), nil + repo := strings.TrimSuffix(name[len(name)-1], ".git") + return fmt.Sprintf("/workspaces/%s", repo), nil } type userInfo struct { diff --git a/envbuilder_test.go b/envbuilder_test.go index e38f0a4d..416169f7 100644 --- a/envbuilder_test.go +++ b/envbuilder_test.go @@ -9,11 +9,34 @@ import ( func TestDefaultWorkspaceFolder(t *testing.T) { t.Parallel() - dir, err := envbuilder.DefaultWorkspaceFolder("https://github.com/coder/coder") - require.NoError(t, err) - require.Equal(t, "/workspaces/coder", dir) - dir, err = envbuilder.DefaultWorkspaceFolder("") - require.NoError(t, err) - require.Equal(t, envbuilder.EmptyWorkspaceDir, dir) + tests := []struct { + name string + gitURL string + expected string + }{ + { + name: "HTTP", + gitURL: "https://github.com/coder/envbuilder.git", + expected: "/workspaces/envbuilder", + }, + { + name: "SSH", + gitURL: "git@github.com:coder/envbuilder.git", + expected: "/workspaces/envbuilder", + }, + { + name: "empty", + gitURL: "", + expected: envbuilder.EmptyWorkspaceDir, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir, err := envbuilder.DefaultWorkspaceFolder(tt.gitURL) + require.NoError(t, err) + require.Equal(t, tt.expected, dir) + }) + } } diff --git a/git.go b/git.go index c206bf5f..71421f38 100644 --- a/git.go +++ b/git.go @@ -22,6 +22,7 @@ import ( gitssh "github.com/go-git/go-git/v5/plumbing/transport/ssh" "github.com/go-git/go-git/v5/storage/filesystem" "github.com/skeema/knownhosts" + giturls "github.com/whilp/git-urls" "golang.org/x/crypto/ssh" gossh "golang.org/x/crypto/ssh" ) @@ -46,7 +47,7 @@ type CloneRepoOptions struct { // // The bool returned states whether the repository was cloned or not. func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { - parsed, err := url.Parse(opts.RepoURL) + parsed, err := ParseGitURL(opts.RepoURL) if err != nil { return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err) } @@ -251,3 +252,8 @@ func SetupRepoAuth(options *Options) transport.AuthMethod { } return auth } + +// Parses a Git URL and returns a corresponding URL object +func ParseGitURL(URL string) (*url.URL, error) { + return giturls.Parse(URL) +} diff --git a/git_test.go b/git_test.go index 7ba0a5e3..e3928113 100644 --- a/git_test.go +++ b/git_test.go @@ -384,6 +384,35 @@ func TestSetupRepoAuth(t *testing.T) { }) } +func TestParseGitURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + gitURL string + expected string + }{ + { + name: "HTTP", + gitURL: "https://github.com/coder/envbuilder.git", + expected: "https://github.com/coder/envbuilder.git", + }, + { + name: "SSH", + gitURL: "git@github.com:coder/envbuilder.git", + expected: "ssh://git@github.com/coder/envbuilder.git", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + url, err := envbuilder.ParseGitURL(tt.gitURL) + require.NoError(t, err) + require.Equal(t, tt.expected, url.String()) + }) + } +} + func mustRead(t *testing.T, fs billy.Filesystem, path string) string { t.Helper() f, err := fs.OpenFile(path, os.O_RDONLY, 0o644) diff --git a/go.mod b/go.mod index 703ee109..90ab4252 100644 --- a/go.mod +++ b/go.mod @@ -249,6 +249,7 @@ require ( github.com/vmihailenco/msgpack v4.0.4+incompatible // indirect github.com/vmihailenco/msgpack/v4 v4.3.12 // indirect github.com/vmihailenco/tagparser v0.1.2 // indirect + github.com/whilp/git-urls v1.0.0 // indirect github.com/x448/float16 v0.8.4 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect github.com/zclconf/go-cty v1.14.1 // indirect diff --git a/go.sum b/go.sum index d957a9b4..20e24369 100644 --- a/go.sum +++ b/go.sum @@ -842,6 +842,8 @@ github.com/vmihailenco/tagparser v0.1.2 h1:gnjoVuB/kljJ5wICEEOpx98oXMWPLj22G67Vb github.com/vmihailenco/tagparser v0.1.2/go.mod h1:OeAg3pn3UbLjkWt+rN9oFYB6u/cQgqMEUPoW2WPyhdI= github.com/wagslane/go-password-validator v0.3.0 h1:vfxOPzGHkz5S146HDpavl0cw1DSVP061Ry2PX0/ON6I= github.com/wagslane/go-password-validator v0.3.0/go.mod h1:TI1XJ6T5fRdRnHqHt14pvy1tNVnrwe7m3/f1f2fDphQ= +github.com/whilp/git-urls v1.0.0 h1:95f6UMWN5FKW71ECsXRUd3FVYiXdrE7aX4NZKcPmIjU= +github.com/whilp/git-urls v1.0.0/go.mod h1:J16SAmobsqc3Qcy98brfl5f5+e0clUvg1krgwk/qCfE= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM= From c80fe81ac603b018aa2bb141972f2ede0ca90f98 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 14 May 2024 13:57:25 +0000 Subject: [PATCH 2/5] Apply review suggestions --- envbuilder.go | 6 +++++- envbuilder_test.go | 10 ++++++++++ git.go | 8 +------- git_test.go | 29 ----------------------------- go.mod | 2 +- 5 files changed, 17 insertions(+), 38 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index edfa79f3..beb6cf3b 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -49,6 +49,7 @@ import ( "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/sirupsen/logrus" "github.com/tailscale/hujson" + giturls "github.com/whilp/git-urls" "golang.org/x/xerrors" ) @@ -858,11 +859,14 @@ func DefaultWorkspaceFolder(repoURL string) (string, error) { if repoURL == "" { return "/workspaces/empty", nil } - parsed, err := ParseGitURL(repoURL) + parsed, err := giturls.Parse(repoURL) if err != nil { return "", err } name := strings.Split(parsed.Path, "/") + if len(name) == 0 { + return "", errors.New("no name found in the repository URL") + } repo := strings.TrimSuffix(name[len(name)-1], ".git") return fmt.Sprintf("/workspaces/%s", repo), nil } diff --git a/envbuilder_test.go b/envbuilder_test.go index 416169f7..43ea5f51 100644 --- a/envbuilder_test.go +++ b/envbuilder_test.go @@ -25,6 +25,16 @@ func TestDefaultWorkspaceFolder(t *testing.T) { gitURL: "git@github.com:coder/envbuilder.git", expected: "/workspaces/envbuilder", }, + { + name: "username and password", + gitURL: "https://username:password@github.com/coder/envbuilder.git", + expected: "/workspaces/envbuilder", + }, + { + name: "fragment", + gitURL: "https://github.com/coder/envbuilder.git#feature-branch", + expected: "/workspaces/envbuilder", + }, { name: "empty", gitURL: "", diff --git a/git.go b/git.go index 71421f38..4aa9c541 100644 --- a/git.go +++ b/git.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "net" - "net/url" "os" "strings" @@ -47,7 +46,7 @@ type CloneRepoOptions struct { // // The bool returned states whether the repository was cloned or not. func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { - parsed, err := ParseGitURL(opts.RepoURL) + parsed, err := giturls.Parse(opts.RepoURL) if err != nil { return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err) } @@ -252,8 +251,3 @@ func SetupRepoAuth(options *Options) transport.AuthMethod { } return auth } - -// Parses a Git URL and returns a corresponding URL object -func ParseGitURL(URL string) (*url.URL, error) { - return giturls.Parse(URL) -} diff --git a/git_test.go b/git_test.go index e3928113..7ba0a5e3 100644 --- a/git_test.go +++ b/git_test.go @@ -384,35 +384,6 @@ func TestSetupRepoAuth(t *testing.T) { }) } -func TestParseGitURL(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - gitURL string - expected string - }{ - { - name: "HTTP", - gitURL: "https://github.com/coder/envbuilder.git", - expected: "https://github.com/coder/envbuilder.git", - }, - { - name: "SSH", - gitURL: "git@github.com:coder/envbuilder.git", - expected: "ssh://git@github.com/coder/envbuilder.git", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - url, err := envbuilder.ParseGitURL(tt.gitURL) - require.NoError(t, err) - require.Equal(t, tt.expected, url.String()) - }) - } -} - func mustRead(t *testing.T, fs billy.Filesystem, path string) string { t.Helper() f, err := fs.OpenFile(path, os.O_RDONLY, 0o644) diff --git a/go.mod b/go.mod index 90ab4252..8eaa09c8 100644 --- a/go.mod +++ b/go.mod @@ -40,6 +40,7 @@ require ( github.com/skeema/knownhosts v1.2.2 github.com/stretchr/testify v1.9.0 github.com/tailscale/hujson v0.0.0-20221223112325-20486734a56a + github.com/whilp/git-urls v1.0.0 go.uber.org/mock v0.4.0 golang.org/x/crypto v0.22.0 golang.org/x/sync v0.7.0 @@ -249,7 +250,6 @@ require ( github.com/vmihailenco/msgpack v4.0.4+incompatible // indirect github.com/vmihailenco/msgpack/v4 v4.3.12 // indirect github.com/vmihailenco/tagparser v0.1.2 // indirect - github.com/whilp/git-urls v1.0.0 // indirect github.com/x448/float16 v0.8.4 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect github.com/zclconf/go-cty v1.14.1 // indirect From 84b17d78b20b6af43552a44cb5455bc95252833e Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 14 May 2024 16:24:06 +0000 Subject: [PATCH 3/5] Add non valid URLs test case --- envbuilder.go | 7 ++++--- envbuilder_test.go | 26 +++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index beb6cf3b..1ea3bbe3 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -857,15 +857,16 @@ func Run(ctx context.Context, options Options) error { // for a given repository URL. func DefaultWorkspaceFolder(repoURL string) (string, error) { if repoURL == "" { - return "/workspaces/empty", nil + return EmptyWorkspaceDir, nil } parsed, err := giturls.Parse(repoURL) if err != nil { return "", err } name := strings.Split(parsed.Path, "/") - if len(name) == 0 { - return "", errors.New("no name found in the repository URL") + hasOwnerAndRepo := len(name) >= 2 + if !hasOwnerAndRepo { + return EmptyWorkspaceDir, nil } repo := strings.TrimSuffix(name[len(name)-1], ".git") return fmt.Sprintf("/workspaces/%s", repo), nil diff --git a/envbuilder_test.go b/envbuilder_test.go index 43ea5f51..6af599c9 100644 --- a/envbuilder_test.go +++ b/envbuilder_test.go @@ -10,7 +10,7 @@ import ( func TestDefaultWorkspaceFolder(t *testing.T) { t.Parallel() - tests := []struct { + successTests := []struct { name string gitURL string expected string @@ -41,12 +41,32 @@ func TestDefaultWorkspaceFolder(t *testing.T) { expected: envbuilder.EmptyWorkspaceDir, }, } - - for _, tt := range tests { + for _, tt := range successTests { t.Run(tt.name, func(t *testing.T) { dir, err := envbuilder.DefaultWorkspaceFolder(tt.gitURL) require.NoError(t, err) require.Equal(t, tt.expected, dir) }) } + + invalidTests := []struct { + name string + invalidURL string + }{ + { + name: "simple text", + invalidURL: "not a valid URL", + }, + { + name: "website URL", + invalidURL: "www.google.com", + }, + } + for _, tt := range invalidTests { + t.Run(tt.name, func(t *testing.T) { + dir, err := envbuilder.DefaultWorkspaceFolder(tt.invalidURL) + require.NoError(t, err) + require.Equal(t, envbuilder.EmptyWorkspaceDir, dir) + }) + } } From dcf67e9d9c04b5713021db9f74ba175ec0216046 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 14 May 2024 16:51:45 +0000 Subject: [PATCH 4/5] Fix integration tests --- integration/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 52abe783..554a2a43 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -133,7 +133,7 @@ func TestSucceedsGitAuthInURL(t *testing.T) { envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"), }}) require.NoError(t, err) - gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config") + gitConfig := execContainer(t, ctr, "cat /workspaces/empty/.git/config") require.Contains(t, gitConfig, u.String()) } From 13e04f9247538d0e2f058c21f44199e0aae274d5 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 14 May 2024 16:56:31 +0000 Subject: [PATCH 5/5] Fix another test --- integration/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 554a2a43..b8927de6 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -111,7 +111,7 @@ func TestSucceedsGitAuth(t *testing.T) { envbuilderEnv("GIT_PASSWORD", "testing"), }}) require.NoError(t, err) - gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config") + gitConfig := execContainer(t, ctr, "cat /workspaces/empty/.git/config") require.Contains(t, gitConfig, srv.URL) }