From 5d1dea15e1981319a669c31cda3404fee8e61cda Mon Sep 17 00:00:00 2001 From: nakabonne Date: Wed, 12 Aug 2020 10:36:04 +0900 Subject: [PATCH 1/5] Enable to make git URLs --- pkg/app/api/api/BUILD.bazel | 1 + pkg/app/api/api/web_api.go | 34 ++- pkg/app/piped/trigger/deployment.go | 9 +- pkg/git/BUILD.bazel | 2 + pkg/git/url.go | 138 ++++++++++++ pkg/git/url_test.go | 322 ++++++++++++++++++++++++++++ 6 files changed, 502 insertions(+), 4 deletions(-) create mode 100644 pkg/git/url.go create mode 100644 pkg/git/url_test.go diff --git a/pkg/app/api/api/BUILD.bazel b/pkg/app/api/api/BUILD.bazel index 9eec001fb0..f5b3348b48 100644 --- a/pkg/app/api/api/BUILD.bazel +++ b/pkg/app/api/api/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//pkg/app/api/service/webservice:go_default_library", "//pkg/app/api/stagelogstore:go_default_library", "//pkg/datastore:go_default_library", + "//pkg/git:go_default_library", "//pkg/model:go_default_library", "//pkg/rpc/rpcauth:go_default_library", "@com_github_google_uuid//:go_default_library", diff --git a/pkg/app/api/api/web_api.go b/pkg/app/api/api/web_api.go index 4c9569c7e7..3756c3cfe3 100644 --- a/pkg/app/api/api/web_api.go +++ b/pkg/app/api/api/web_api.go @@ -30,6 +30,7 @@ import ( "github.com/pipe-cd/pipe/pkg/app/api/service/webservice" "github.com/pipe-cd/pipe/pkg/app/api/stagelogstore" "github.com/pipe-cd/pipe/pkg/datastore" + "github.com/pipe-cd/pipe/pkg/git" "github.com/pipe-cd/pipe/pkg/model" "github.com/pipe-cd/pipe/pkg/rpc/rpcauth" ) @@ -288,14 +289,17 @@ func (a *WebAPI) AddApplication(ctx context.Context, req *webservice.AddApplicat if err != nil { return nil, err } - + gitpath, err := a.makeGitPath(ctx, req.GitPath.Repo.Id, req.GitPath.Path, req.GitPath.ConfigFilename, req.PipedId) + if err != nil { + return nil, err + } app := model.Application{ Id: uuid.New().String(), Name: req.Name, EnvId: req.EnvId, PipedId: req.PipedId, ProjectId: claims.Role.ProjectId, - GitPath: req.GitPath, + GitPath: gitpath, Kind: req.Kind, CloudProvider: req.CloudProvider, } @@ -311,6 +315,32 @@ func (a *WebAPI) AddApplication(ctx context.Context, req *webservice.AddApplicat return &webservice.AddApplicationResponse{}, nil } +// Adds Repository info and then makes the GitPath URL. +func (a *WebAPI) makeGitPath(ctx context.Context, repoID, path, cfgFilename, pipedID string) (*model.ApplicationGitPath, error) { + + piped, err := a.getPiped(ctx, pipedID) + if err != nil { + return nil, err + } + repo := &model.ApplicationGitRepository{} + for _, r := range piped.Repositories { + if r.Id == repoID { + repo = r + break + } + } + u, err := git.MakeDirURL(repo.Remote, path, repo.Branch) + if err != nil { + return nil, err + } + return &model.ApplicationGitPath{ + Repo: repo, + Path: path, + ConfigFilename: cfgFilename, + Url: u, + }, nil +} + func (a *WebAPI) EnableApplication(ctx context.Context, req *webservice.EnableApplicationRequest) (*webservice.EnableApplicationResponse, error) { if err := a.updateApplicationEnable(ctx, req.ApplicationId, true); err != nil { return nil, err diff --git a/pkg/app/piped/trigger/deployment.go b/pkg/app/piped/trigger/deployment.go index 607f994f6f..422ef4811e 100644 --- a/pkg/app/piped/trigger/deployment.go +++ b/pkg/app/piped/trigger/deployment.go @@ -30,7 +30,7 @@ import ( ) func (t *Trigger) triggerDeployment(ctx context.Context, app *model.Application, branch string, commit git.Commit, commander string) (runErr error) { - deployment, err := buildDeploment(app, branch, commit, commander, time.Now()) + deployment, err := buildDeployment(app, branch, commit, commander, time.Now()) if err != nil { return err } @@ -109,7 +109,11 @@ func (t *Trigger) reportMostRecentlyTriggeredDeployment(ctx context.Context, d * return err } -func buildDeploment(app *model.Application, branch string, commit git.Commit, commander string, now time.Time) (*model.Deployment, error) { +func buildDeployment(app *model.Application, branch string, commit git.Commit, commander string, now time.Time) (*model.Deployment, error) { + commitURL, err := git.MakeCommitURL(app.GitPath.Repo.Remote, commit.Hash) + if err != nil { + return nil, err + } deployment := &model.Deployment{ Id: uuid.New().String(), ApplicationId: app.Id, @@ -124,6 +128,7 @@ func buildDeploment(app *model.Application, branch string, commit git.Commit, co Message: commit.Message, Author: commit.Author, Branch: branch, + Url: commitURL, CreatedAt: int64(commit.CreatedAt), }, Commander: commander, diff --git a/pkg/git/BUILD.bazel b/pkg/git/BUILD.bazel index 90c6c74c8b..bcda26d65c 100644 --- a/pkg/git/BUILD.bazel +++ b/pkg/git/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "commit.go", "repo.go", "ssh_config.go", + "url.go", ], importpath = "github.com/pipe-cd/pipe/pkg/git", visibility = ["//visibility:public"], @@ -24,6 +25,7 @@ go_test( "commit_test.go", "repo_test.go", "ssh_config_test.go", + "url_test.go", ], data = glob(["testdata/**"]), embed = [":go_default_library"], diff --git a/pkg/git/url.go b/pkg/git/url.go new file mode 100644 index 0000000000..2881d9fe9e --- /dev/null +++ b/pkg/git/url.go @@ -0,0 +1,138 @@ +// Copyright 2020 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package git + +import ( + "fmt" + "net/url" + "regexp" + "strings" +) + +// MakeCommitURL builds a link to the HTML page of the commit, using the given repoURL and hash. +func MakeCommitURL(repoURL, hash string) (string, error) { + u, err := parseGitURL(repoURL) + if err != nil { + return "", err + } + + scheme := "https" + if u.Scheme != "ssh" { + scheme = u.Scheme + } + + repoPath := strings.Trim(u.Path, "/") + repoPath = strings.TrimSuffix(repoPath, ".git") + + subPath := "" + switch u.Host { + case "github.com", "gitlab.com": + subPath = "commit" + case "bitbucket.org": + subPath = "commits" + default: + return "", fmt.Errorf("unsupported git host: %q", u.Host) + } + + return fmt.Sprintf("%s://%s/%s/%s/%s", scheme, u.Host, repoPath, subPath, hash), nil +} + +// MakeDirURL builds a link to the HTML page of the directory. +func MakeDirURL(repoURL, dir, branch string) (string, error) { + if branch == "" { + return "", fmt.Errorf("no branch given") + } + u, err := parseGitURL(repoURL) + if err != nil { + return "", err + } + + scheme := "https" + if u.Scheme != "ssh" { + scheme = u.Scheme + } + + repoPath := strings.Trim(u.Path, "/") + repoPath = strings.TrimSuffix(repoPath, ".git") + + subPath := "" + switch u.Host { + // TODO: Support more git host + case "github.com": + subPath = "tree" + default: + return "", fmt.Errorf("unsupported git host: %q", u.Host) + } + + dir = strings.Trim(dir, "/") + + return fmt.Sprintf("%s://%s/%s/%s/%s/%s", scheme, u.Host, repoPath, subPath, branch, dir), nil +} + +var ( + knownSchemes = map[string]interface{}{ + "ssh": struct{}{}, + "git": struct{}{}, + "git+ssh": struct{}{}, + "http": struct{}{}, + "https": struct{}{}, + "ftp": struct{}{}, + "ftps": struct{}{}, + "rsync": struct{}{}, + "file": struct{}{}, + } + scpRegex = regexp.MustCompile(`^([a-zA-Z0-9_]+@)?([a-zA-Z0-9._-]+):(.*)$`) +) + +// parseGitURL parses git url into a URL structure. +func parseGitURL(rawurl string) (u *url.URL, err error) { + u, err = parseTransport(rawurl) + if err == nil { + return + } + return parseScp(rawurl) +} + +// Return a structured URL only when scheme is a known Git transport. +func parseTransport(rawurl string) (*url.URL, error) { + u, err := url.Parse(rawurl) + if err != nil { + return nil, fmt.Errorf("failed to parse git url: %w", err) + } + if _, ok := knownSchemes[u.Scheme]; !ok { + return nil, fmt.Errorf("unknown scheme %q", u.Scheme) + } + return u, nil +} + +// Return a structured URL only when the rawurl is an SCP-like URL. +func parseScp(rawurl string) (*url.URL, error) { + match := scpRegex.FindAllStringSubmatch(rawurl, -1) + if len(match) == 0 { + return nil, fmt.Errorf("no scp URL found in %q", rawurl) + } + m := match[0] + user := strings.TrimRight(m[1], "@") + var userinfo *url.Userinfo + if user != "" { + userinfo = url.User(user) + } + return &url.URL{ + Scheme: "ssh", + User: userinfo, + Host: m[2], + Path: m[3], + }, nil +} diff --git a/pkg/git/url_test.go b/pkg/git/url_test.go new file mode 100644 index 0000000000..26e52f43c1 --- /dev/null +++ b/pkg/git/url_test.go @@ -0,0 +1,322 @@ +package git + +import ( + "net/url" + "reflect" + "testing" +) + +func TestMakeCommitURL(t *testing.T) { + tests := []struct { + name string + repoURL string + hash string + want string + wantErr bool + }{ + { + name: "ssh to github.com", + repoURL: "git@github.com:org/repo.git", + hash: "abc", + want: "https://github.com/org/repo/commit/abc", + wantErr: false, + }, + { + name: "ssh to gitlab.com", + repoURL: "git@gitlab.com:org/repo.git", + hash: "abc", + want: "https://gitlab.com/org/repo/commit/abc", + wantErr: false, + }, + { + name: "ssh to bitbucket.org", + repoURL: "git@bitbucket.org:org/repo.git", + hash: "abc", + want: "https://bitbucket.org/org/repo/commits/abc", + wantErr: false, + }, + { + name: "ssh to unsupported git host", + repoURL: "git@foo.com:org/repo.git", + hash: "abc", + want: "", + wantErr: true, + }, + { + name: "ssh to github.com without `.git` suffix", + repoURL: "git@github.com:org/repo", + hash: "abc", + want: "https://github.com/org/repo/commit/abc", + wantErr: false, + }, + { + name: "ssh to github.com with `/` suffix", + repoURL: "git@github.com:org/repo/", + hash: "abc", + want: "https://github.com/org/repo/commit/abc", + wantErr: false, + }, + { + name: "http to github.com", + repoURL: "http://github.com/org/repo", + hash: "abc", + want: "http://github.com/org/repo/commit/abc", + wantErr: false, + }, + { + name: "unparseable url", + repoURL: "1234abcd", + hash: "abc", + want: "", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := MakeCommitURL(tt.repoURL, tt.hash) + if (err != nil) != tt.wantErr { + t.Errorf("MakeCommitURL() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("MakeCommitURL() got = %v, want %v", got, tt.want) + } + }) + } +} + +func TestMakeDirURL(t *testing.T) { + tests := []struct { + name string + repoURL string + dir string + branch string + want string + wantErr bool + }{ + { + name: "ssh to github.com", + repoURL: "git@github.com:org/repo.git", + dir: "path/to", + branch: "abc", + want: "https://github.com/org/repo/tree/abc/path/to", + wantErr: false, + }, + { + name: "ssh to unsupported git host", + repoURL: "git@foo.com:org/repo.git", + dir: "path/to", + branch: "abc", + want: "", + wantErr: true, + }, + { + name: "ssh to github.com without `.git` suffix", + repoURL: "git@github.com:org/repo", + dir: "path/to", + branch: "abc", + want: "https://github.com/org/repo/tree/abc/path/to", + wantErr: false, + }, + { + name: "ssh to github.com with `/` suffix", + repoURL: "git@github.com:org/repo/", + dir: "path/to", + branch: "abc", + want: "https://github.com/org/repo/tree/abc/path/to", + wantErr: false, + }, + { + name: "http to github.com", + repoURL: "http://github.com/org/repo", + dir: "path/to", + branch: "abc", + want: "http://github.com/org/repo/tree/abc/path/to", + wantErr: false, + }, + { + name: "unparseable url", + repoURL: "1234abcd", + dir: "path/to", + branch: "abc", + want: "", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := MakeDirURL(tt.repoURL, tt.dir, tt.branch) + if (err != nil) != tt.wantErr { + t.Errorf("MakeCommitURL() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("MakeCommitURL() got = %v, want %v", got, tt.want) + } + }) + } +} +func Test_parseGitURL(t *testing.T) { + tests := []struct { + name string + rawurl string + wantU *url.URL + wantErr bool + }{ + { + name: "SCP-like URL with user", + rawurl: "user@host.xz:path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ssh", + User: url.User("user"), + Host: "host.xz", + Path: "path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "SCP-like URL without user", + rawurl: "host.xz:path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ssh", + User: nil, + Host: "host.xz", + Path: "path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "SCP-like URL with prefix `/`", + rawurl: "host.xz:/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ssh", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "ssh with user", + rawurl: "ssh://user@host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ssh", + User: url.User("user"), + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "ssh with user with port", + rawurl: "ssh://user@host.xz:1234/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ssh", + User: url.User("user"), + Host: "host.xz:1234", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "git+ssh", + rawurl: "git+ssh://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "git+ssh", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "file scheme", + rawurl: "file:///path/to/repo.git/", + wantU: &url.URL{ + Scheme: "file", + User: nil, + Host: "", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "rsync + ssh", + rawurl: "rsync://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "rsync", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "git scheme", + rawurl: "git://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "git", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "http scheme", + rawurl: "http://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "http", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "https scheme", + rawurl: "https://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "https", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "ftp scheme", + rawurl: "ftp://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ftp", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + { + name: "ftps scheme", + rawurl: "ftps://host.xz/path/to/repo.git/", + wantU: &url.URL{ + Scheme: "ftps", + User: nil, + Host: "host.xz", + Path: "/path/to/repo.git/", + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotU, err := parseGitURL(tt.rawurl) + if (err != nil) != tt.wantErr { + t.Errorf("parseGitURL() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(gotU, tt.wantU) { + t.Errorf("parseGitURL() got = %#v, want %#v", gotU, tt.wantU) + } + }) + } +} From d143434eaa8b20f3ea800829d5af9bbc7b41c5a3 Mon Sep 17 00:00:00 2001 From: nakabonne Date: Wed, 12 Aug 2020 11:36:10 +0900 Subject: [PATCH 2/5] Fix place where pointed out --- pkg/app/api/api/web_api.go | 18 ++++++++++++---- pkg/app/piped/trigger/deployment.go | 11 +++++++--- pkg/git/url.go | 10 +++++---- pkg/git/url_test.go | 32 +++++------------------------ 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/pkg/app/api/api/web_api.go b/pkg/app/api/api/web_api.go index 3756c3cfe3..9c6ee2f042 100644 --- a/pkg/app/api/api/web_api.go +++ b/pkg/app/api/api/web_api.go @@ -315,23 +315,33 @@ func (a *WebAPI) AddApplication(ctx context.Context, req *webservice.AddApplicat return &webservice.AddApplicationResponse{}, nil } -// Adds Repository info and then makes the GitPath URL. +// makeGitPath returns an ApplicationGitPath by adding Repository info and GitPath URL to given args. func (a *WebAPI) makeGitPath(ctx context.Context, repoID, path, cfgFilename, pipedID string) (*model.ApplicationGitPath, error) { - piped, err := a.getPiped(ctx, pipedID) if err != nil { return nil, err } - repo := &model.ApplicationGitRepository{} + + var repo *model.ApplicationGitRepository for _, r := range piped.Repositories { if r.Id == repoID { repo = r break } } + if repo == nil { + a.logger.Error("repository not found", + zap.String("repo-id", repoID), + zap.String("piped-id", pipedID), + zap.Error(err), + ) + return nil, status.Error(codes.Internal, "repository not found") + } + u, err := git.MakeDirURL(repo.Remote, path, repo.Branch) if err != nil { - return nil, err + a.logger.Error("failed to make GitPath URL", zap.Error(err)) + return nil, status.Error(codes.Internal, "failed to make GitPath URL") } return &model.ApplicationGitPath{ Repo: repo, diff --git a/pkg/app/piped/trigger/deployment.go b/pkg/app/piped/trigger/deployment.go index 422ef4811e..e46ae018b3 100644 --- a/pkg/app/piped/trigger/deployment.go +++ b/pkg/app/piped/trigger/deployment.go @@ -110,10 +110,15 @@ func (t *Trigger) reportMostRecentlyTriggeredDeployment(ctx context.Context, d * } func buildDeployment(app *model.Application, branch string, commit git.Commit, commander string, now time.Time) (*model.Deployment, error) { - commitURL, err := git.MakeCommitURL(app.GitPath.Repo.Remote, commit.Hash) - if err != nil { - return nil, err + commitURL := "" + if r := app.GitPath.Repo; r != nil { + var err error + commitURL, err = git.MakeCommitURL(r.Remote, commit.Hash) + if err != nil { + return nil, err + } } + deployment := &model.Deployment{ Id: uuid.New().String(), ApplicationId: app.Id, diff --git a/pkg/git/url.go b/pkg/git/url.go index 2881d9fe9e..49df8fe516 100644 --- a/pkg/git/url.go +++ b/pkg/git/url.go @@ -43,7 +43,10 @@ func MakeCommitURL(repoURL, hash string) (string, error) { case "bitbucket.org": subPath = "commits" default: - return "", fmt.Errorf("unsupported git host: %q", u.Host) + // TODO: Allow users to specify git host + // Currently, the same subPath as Github is applied for all of unsupported hosts, + // to support GHE where its host could be customized + subPath = "commit" } return fmt.Sprintf("%s://%s/%s/%s/%s", scheme, u.Host, repoPath, subPath, hash), nil @@ -73,7 +76,8 @@ func MakeDirURL(repoURL, dir, branch string) (string, error) { case "github.com": subPath = "tree" default: - return "", fmt.Errorf("unsupported git host: %q", u.Host) + // TODO: Allow users to specify git host + subPath = "tree" } dir = strings.Trim(dir, "/") @@ -88,8 +92,6 @@ var ( "git+ssh": struct{}{}, "http": struct{}{}, "https": struct{}{}, - "ftp": struct{}{}, - "ftps": struct{}{}, "rsync": struct{}{}, "file": struct{}{}, } diff --git a/pkg/git/url_test.go b/pkg/git/url_test.go index 26e52f43c1..19f6d94320 100644 --- a/pkg/git/url_test.go +++ b/pkg/git/url_test.go @@ -39,8 +39,8 @@ func TestMakeCommitURL(t *testing.T) { name: "ssh to unsupported git host", repoURL: "git@foo.com:org/repo.git", hash: "abc", - want: "", - wantErr: true, + want: "https://foo.com/org/repo/commit/abc", + wantErr: false, }, { name: "ssh to github.com without `.git` suffix", @@ -107,8 +107,8 @@ func TestMakeDirURL(t *testing.T) { repoURL: "git@foo.com:org/repo.git", dir: "path/to", branch: "abc", - want: "", - wantErr: true, + want: "https://foo.com/org/repo/tree/abc/path/to", + wantErr: false, }, { name: "ssh to github.com without `.git` suffix", @@ -156,7 +156,7 @@ func TestMakeDirURL(t *testing.T) { }) } } -func Test_parseGitURL(t *testing.T) { +func TestParseGitURL(t *testing.T) { tests := []struct { name string rawurl string @@ -284,28 +284,6 @@ func Test_parseGitURL(t *testing.T) { }, wantErr: false, }, - { - name: "ftp scheme", - rawurl: "ftp://host.xz/path/to/repo.git/", - wantU: &url.URL{ - Scheme: "ftp", - User: nil, - Host: "host.xz", - Path: "/path/to/repo.git/", - }, - wantErr: false, - }, - { - name: "ftps scheme", - rawurl: "ftps://host.xz/path/to/repo.git/", - wantU: &url.URL{ - Scheme: "ftps", - User: nil, - Host: "host.xz", - Path: "/path/to/repo.git/", - }, - wantErr: false, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From e7a2c7809a7169d0218cfacffd8921a5a13f8489 Mon Sep 17 00:00:00 2001 From: nakabonne Date: Wed, 12 Aug 2020 11:49:39 +0900 Subject: [PATCH 3/5] Rename rawURL --- pkg/git/url.go | 18 +++++++++--------- pkg/git/url_test.go | 26 +++++++++++++------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/pkg/git/url.go b/pkg/git/url.go index 49df8fe516..4ed0bbc91c 100644 --- a/pkg/git/url.go +++ b/pkg/git/url.go @@ -99,17 +99,17 @@ var ( ) // parseGitURL parses git url into a URL structure. -func parseGitURL(rawurl string) (u *url.URL, err error) { - u, err = parseTransport(rawurl) +func parseGitURL(rawURL string) (u *url.URL, err error) { + u, err = parseTransport(rawURL) if err == nil { return } - return parseScp(rawurl) + return parseScp(rawURL) } // Return a structured URL only when scheme is a known Git transport. -func parseTransport(rawurl string) (*url.URL, error) { - u, err := url.Parse(rawurl) +func parseTransport(rawURL string) (*url.URL, error) { + u, err := url.Parse(rawURL) if err != nil { return nil, fmt.Errorf("failed to parse git url: %w", err) } @@ -119,11 +119,11 @@ func parseTransport(rawurl string) (*url.URL, error) { return u, nil } -// Return a structured URL only when the rawurl is an SCP-like URL. -func parseScp(rawurl string) (*url.URL, error) { - match := scpRegex.FindAllStringSubmatch(rawurl, -1) +// Return a structured URL only when the rawURL is an SCP-like URL. +func parseScp(rawURL string) (*url.URL, error) { + match := scpRegex.FindAllStringSubmatch(rawURL, -1) if len(match) == 0 { - return nil, fmt.Errorf("no scp URL found in %q", rawurl) + return nil, fmt.Errorf("no scp URL found in %q", rawURL) } m := match[0] user := strings.TrimRight(m[1], "@") diff --git a/pkg/git/url_test.go b/pkg/git/url_test.go index 19f6d94320..47b34b0f89 100644 --- a/pkg/git/url_test.go +++ b/pkg/git/url_test.go @@ -159,13 +159,13 @@ func TestMakeDirURL(t *testing.T) { func TestParseGitURL(t *testing.T) { tests := []struct { name string - rawurl string + rawURL string wantU *url.URL wantErr bool }{ { name: "SCP-like URL with user", - rawurl: "user@host.xz:path/to/repo.git/", + rawURL: "user@host.xz:path/to/repo.git/", wantU: &url.URL{ Scheme: "ssh", User: url.User("user"), @@ -176,7 +176,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "SCP-like URL without user", - rawurl: "host.xz:path/to/repo.git/", + rawURL: "host.xz:path/to/repo.git/", wantU: &url.URL{ Scheme: "ssh", User: nil, @@ -187,7 +187,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "SCP-like URL with prefix `/`", - rawurl: "host.xz:/path/to/repo.git/", + rawURL: "host.xz:/path/to/repo.git/", wantU: &url.URL{ Scheme: "ssh", User: nil, @@ -198,7 +198,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "ssh with user", - rawurl: "ssh://user@host.xz/path/to/repo.git/", + rawURL: "ssh://user@host.xz/path/to/repo.git/", wantU: &url.URL{ Scheme: "ssh", User: url.User("user"), @@ -209,7 +209,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "ssh with user with port", - rawurl: "ssh://user@host.xz:1234/path/to/repo.git/", + rawURL: "ssh://user@host.xz:1234/path/to/repo.git/", wantU: &url.URL{ Scheme: "ssh", User: url.User("user"), @@ -220,7 +220,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "git+ssh", - rawurl: "git+ssh://host.xz/path/to/repo.git/", + rawURL: "git+ssh://host.xz/path/to/repo.git/", wantU: &url.URL{ Scheme: "git+ssh", User: nil, @@ -231,7 +231,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "file scheme", - rawurl: "file:///path/to/repo.git/", + rawURL: "file:///path/to/repo.git/", wantU: &url.URL{ Scheme: "file", User: nil, @@ -242,7 +242,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "rsync + ssh", - rawurl: "rsync://host.xz/path/to/repo.git/", + rawURL: "rsync://host.xz/path/to/repo.git/", wantU: &url.URL{ Scheme: "rsync", User: nil, @@ -253,7 +253,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "git scheme", - rawurl: "git://host.xz/path/to/repo.git/", + rawURL: "git://host.xz/path/to/repo.git/", wantU: &url.URL{ Scheme: "git", User: nil, @@ -264,7 +264,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "http scheme", - rawurl: "http://host.xz/path/to/repo.git/", + rawURL: "http://host.xz/path/to/repo.git/", wantU: &url.URL{ Scheme: "http", User: nil, @@ -275,7 +275,7 @@ func TestParseGitURL(t *testing.T) { }, { name: "https scheme", - rawurl: "https://host.xz/path/to/repo.git/", + rawURL: "https://host.xz/path/to/repo.git/", wantU: &url.URL{ Scheme: "https", User: nil, @@ -287,7 +287,7 @@ func TestParseGitURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotU, err := parseGitURL(tt.rawurl) + gotU, err := parseGitURL(tt.rawURL) if (err != nil) != tt.wantErr { t.Errorf("parseGitURL() error = %v, wantErr %v", err, tt.wantErr) return From 08cbdc478590b10bcd7487c4a57a387ef459eacf Mon Sep 17 00:00:00 2001 From: nakabonne Date: Wed, 12 Aug 2020 11:50:04 +0900 Subject: [PATCH 4/5] Rename watnURL --- pkg/git/url_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/git/url_test.go b/pkg/git/url_test.go index 47b34b0f89..ff5aa0b88a 100644 --- a/pkg/git/url_test.go +++ b/pkg/git/url_test.go @@ -160,13 +160,13 @@ func TestParseGitURL(t *testing.T) { tests := []struct { name string rawURL string - wantU *url.URL + wantURL *url.URL wantErr bool }{ { name: "SCP-like URL with user", rawURL: "user@host.xz:path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "ssh", User: url.User("user"), Host: "host.xz", @@ -177,7 +177,7 @@ func TestParseGitURL(t *testing.T) { { name: "SCP-like URL without user", rawURL: "host.xz:path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "ssh", User: nil, Host: "host.xz", @@ -188,7 +188,7 @@ func TestParseGitURL(t *testing.T) { { name: "SCP-like URL with prefix `/`", rawURL: "host.xz:/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "ssh", User: nil, Host: "host.xz", @@ -199,7 +199,7 @@ func TestParseGitURL(t *testing.T) { { name: "ssh with user", rawURL: "ssh://user@host.xz/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "ssh", User: url.User("user"), Host: "host.xz", @@ -210,7 +210,7 @@ func TestParseGitURL(t *testing.T) { { name: "ssh with user with port", rawURL: "ssh://user@host.xz:1234/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "ssh", User: url.User("user"), Host: "host.xz:1234", @@ -221,7 +221,7 @@ func TestParseGitURL(t *testing.T) { { name: "git+ssh", rawURL: "git+ssh://host.xz/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "git+ssh", User: nil, Host: "host.xz", @@ -232,7 +232,7 @@ func TestParseGitURL(t *testing.T) { { name: "file scheme", rawURL: "file:///path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "file", User: nil, Host: "", @@ -243,7 +243,7 @@ func TestParseGitURL(t *testing.T) { { name: "rsync + ssh", rawURL: "rsync://host.xz/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "rsync", User: nil, Host: "host.xz", @@ -254,7 +254,7 @@ func TestParseGitURL(t *testing.T) { { name: "git scheme", rawURL: "git://host.xz/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "git", User: nil, Host: "host.xz", @@ -265,7 +265,7 @@ func TestParseGitURL(t *testing.T) { { name: "http scheme", rawURL: "http://host.xz/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "http", User: nil, Host: "host.xz", @@ -276,7 +276,7 @@ func TestParseGitURL(t *testing.T) { { name: "https scheme", rawURL: "https://host.xz/path/to/repo.git/", - wantU: &url.URL{ + wantURL: &url.URL{ Scheme: "https", User: nil, Host: "host.xz", @@ -292,8 +292,8 @@ func TestParseGitURL(t *testing.T) { t.Errorf("parseGitURL() error = %v, wantErr %v", err, tt.wantErr) return } - if !reflect.DeepEqual(gotU, tt.wantU) { - t.Errorf("parseGitURL() got = %#v, want %#v", gotU, tt.wantU) + if !reflect.DeepEqual(gotU, tt.wantURL) { + t.Errorf("parseGitURL() got = %#v, want %#v", gotU, tt.wantURL) } }) } From b023edfd7d2fe7ea724001bc32f40dd0705b4d4c Mon Sep 17 00:00:00 2001 From: nakabonne Date: Wed, 12 Aug 2020 11:54:52 +0900 Subject: [PATCH 5/5] Use testify package --- pkg/git/url_test.go | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/pkg/git/url_test.go b/pkg/git/url_test.go index ff5aa0b88a..8af869a491 100644 --- a/pkg/git/url_test.go +++ b/pkg/git/url_test.go @@ -2,8 +2,9 @@ package git import ( "net/url" - "reflect" "testing" + + "github.com/stretchr/testify/assert" ) func TestMakeCommitURL(t *testing.T) { @@ -74,13 +75,8 @@ func TestMakeCommitURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := MakeCommitURL(tt.repoURL, tt.hash) - if (err != nil) != tt.wantErr { - t.Errorf("MakeCommitURL() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("MakeCommitURL() got = %v, want %v", got, tt.want) - } + assert.Equal(t, tt.wantErr, err != nil) + assert.Equal(t, got, tt.want) }) } } @@ -146,13 +142,8 @@ func TestMakeDirURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := MakeDirURL(tt.repoURL, tt.dir, tt.branch) - if (err != nil) != tt.wantErr { - t.Errorf("MakeCommitURL() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("MakeCommitURL() got = %v, want %v", got, tt.want) - } + assert.Equal(t, tt.wantErr, err != nil) + assert.Equal(t, got, tt.want) }) } } @@ -287,14 +278,9 @@ func TestParseGitURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotU, err := parseGitURL(tt.rawURL) - if (err != nil) != tt.wantErr { - t.Errorf("parseGitURL() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(gotU, tt.wantURL) { - t.Errorf("parseGitURL() got = %#v, want %#v", gotU, tt.wantURL) - } + got, err := parseGitURL(tt.rawURL) + assert.Equal(t, tt.wantErr, err != nil) + assert.Equal(t, got, tt.wantURL) }) } }