From 1dbab823d7149432ab00c2099b22b4eecb969a05 Mon Sep 17 00:00:00 2001
From: BrunoQuaresma <bruno_nonato_quaresma@hotmail.com>
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 <bruno_nonato_quaresma@hotmail.com>
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 <bruno_nonato_quaresma@hotmail.com>
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 <bruno_nonato_quaresma@hotmail.com>
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 <bruno_nonato_quaresma@hotmail.com>
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)
}