Enable to make git URLs #594

nakabonne · 2020-08-12T01:39:22Z

What this PR does / why we need it:
This PR does:

adding utility functions to make URLs
calling one of them MakeDirURL when adding an Application
calling one of them MakeCommitURL when building a Deployment

Which issue(s) this PR fixes:

Fixes #414

Does this PR introduce a user-facing change?:

NONE

nakabonne · 2020-08-12T01:42:33Z

pkg/app/piped/trigger/deployment.go


-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)


Panic could be occurred because old Applications don't have Repo.
But proto validate ensures that Repo is populated, so I keep this implementation.
https://github.com/pipe-cd/pipe/blob/af263d98e824fd97f5b5e22934f1e188c8f32608/pkg/model/common.proto#L32

pipecd-bot · 2020-08-12T01:44:53Z

Code coverage for golang is 21.43%. This pull request increases coverage by 0.53%.

File	Function	Base	Head	Diff
pkg/app/api/api/web_api.go	WebAPI.makeGitPath	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	buildDeployment	`--`	`0.00%`	`+0.00%`
pkg/git/url.go	MakeCommitURL	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	MakeDirURL	`--`	`93.75%`	`+93.75%`
pkg/git/url.go	parseGitURL	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	parseTransport	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	parseScp	`--`	`100.00%`	`+100.00%`
pkg/app/api/api/web_api.go	WebAPI.AddApplication	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	buildDeploment	`0.00%`	`--`	`+-0.00%`

nghialv · 2020-08-12T01:52:49Z

pkg/git/url.go

+	case "bitbucket.org":
+		subPath = "commits"
+	default:
+		return "", fmt.Errorf("unsupported git host: %q", u.Host)


How about GHE where its host could be customized?

Hmm... actually I'm still debating for it.
For now, I'm wondering if "commit" is applied for all of unsupported hosts.

switch u.Host { case "github.com", "gitlab.com": subPath = "commit" case "bitbucket.org": subPath = "commits" default: subPath = "commit" }

I think it is ok to apply this way at this time. In the future, we can allow the user to specify those fields from Web UI while registering the application.

Got it, gonna do so.

nghialv · 2020-08-12T01:54:26Z

pkg/git/url_test.go

+		})
+	}
+}
+func Test_parseGitURL(t *testing.T) {


nit: TestParseGitURL

Intellij IDEA generates this name (maybe to distinguish between public ParseGitURL and private)...
But a bit gross, fix it.

nghialv · 2020-08-12T01:58:58Z

pkg/app/api/api/web_api.go

 	return &webservice.AddApplicationResponse{}, nil
 }

+// Adds Repository info and then makes the GitPath URL.


nit: "makeGitPath ..."

nghialv · 2020-08-12T01:59:06Z

pkg/app/api/api/web_api.go


+// Adds Repository info and then makes the GitPath URL.
+func (a *WebAPI) makeGitPath(ctx context.Context, repoID, path, cfgFilename, pipedID string) (*model.ApplicationGitPath, error) {
+


nit: Remove empty line.

nghialv · 2020-08-12T02:01:22Z

pkg/app/api/api/web_api.go

+	}
+	u, err := git.MakeDirURL(repo.Remote, path, repo.Branch)
+	if err != nil {
+		return nil, err


Return a gRPC error. This should error be logged and then return an internal error.

You're quite right.

nghialv · 2020-08-12T02:06:26Z

pkg/git/url.go

+		"ftp":     struct{}{},
+		"ftps":    struct{}{},
+		"rsync":   struct{}{},
+		"file":    struct{}{},


https://git-scm.com/docs/git-clone#_git_urls

Git supports ssh, git, http, and https protocols (in addition, ftp, and ftps can be used for fetching, but this is inefficient and deprecated; do not use it).

Because ftp and other protocols were deprecated, so we only need to support ssh, git, http[s].

Oh, I was not sure that. Thanks.

nghialv · 2020-08-12T02:17:19Z

pkg/app/piped/trigger/deployment.go

 }

-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) {


Better than panic, let's add a check for the existence of Repo field and return an error if it was nil.

Instead of returning an error, I decided to ignore if it's nil

nghialv · 2020-08-12T02:20:01Z

pkg/app/api/api/web_api.go

+	if err != nil {
+		return nil, err
+	}
+	repo := &model.ApplicationGitRepository{}


var repo *model.ApplicationGitRepository

and return an error when the repository was not found.

pipecd-bot · 2020-08-12T02:41:08Z

Code coverage for golang is 21.41%. This pull request increases coverage by 0.51%.

File	Function	Base	Head	Diff
pkg/app/api/api/web_api.go	WebAPI.makeGitPath	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	buildDeployment	`--`	`0.00%`	`+0.00%`
pkg/git/url.go	MakeCommitURL	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	MakeDirURL	`--`	`93.75%`	`+93.75%`
pkg/git/url.go	parseGitURL	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	parseTransport	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	parseScp	`--`	`100.00%`	`+100.00%`
pkg/app/api/api/web_api.go	WebAPI.AddApplication	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	buildDeploment	`0.00%`	`--`	`+-0.00%`

nghialv · 2020-08-12T02:45:07Z

pkg/git/url_test.go

+func TestParseGitURL(t *testing.T) {
+	tests := []struct {
+		name    string
+		rawurl  string


nit: rawURL

nghialv · 2020-08-12T02:45:16Z

pkg/git/url_test.go

+	tests := []struct {
+		name    string
+		rawurl  string
+		wantU   *url.URL


nit: wantURL

nghialv · 2020-08-12T02:47:27Z

pkg/git/url_test.go

+			if !reflect.DeepEqual(gotU, tt.wantU) {
+				t.Errorf("parseGitURL() got = %#v, want %#v", gotU, tt.wantU)
+			}
+		})


We are using testify for testing so let's use its functions to unify the code.

assert.Equal(t, tc.expectedURL, gotURL) assert.Equal(t, tc.exepcetedErr, err)

nghialv · 2020-08-12T02:54:25Z

LGTM

pipecd-bot · 2020-08-12T02:55:03Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Allow users to specify git host

https://github.com/pipe-cd/pipe/blob/b023edfd7d2fe7ea724001bc32f40dd0705b4d4c/pkg/git/url.go#L46-L49

This was created by todo plugin since "TODO:" was found in `b023edf` when #594 was merged. cc: @nakabonne.

2. Support more git host

https://github.com/pipe-cd/pipe/blob/b023edfd7d2fe7ea724001bc32f40dd0705b4d4c/pkg/git/url.go#L75-L78

This was created by todo plugin since "TODO:" was found in `b023edf` when #594 was merged. cc: @nakabonne.

3. Allow users to specify git host

https://github.com/pipe-cd/pipe/blob/b023edfd7d2fe7ea724001bc32f40dd0705b4d4c/pkg/git/url.go#L79-L82

This was created by todo plugin since "TODO:" was found in `b023edf` when #594 was merged. cc: @nakabonne.

pipecd-bot · 2020-08-12T03:00:07Z

Code coverage for golang is 21.41%. This pull request increases coverage by 0.51%.

File	Function	Base	Head	Diff
pkg/app/api/api/web_api.go	WebAPI.makeGitPath	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	buildDeployment	`--`	`0.00%`	`+0.00%`
pkg/git/url.go	MakeCommitURL	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	MakeDirURL	`--`	`93.75%`	`+93.75%`
pkg/git/url.go	parseGitURL	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	parseTransport	`--`	`100.00%`	`+100.00%`
pkg/git/url.go	parseScp	`--`	`100.00%`	`+100.00%`
pkg/app/api/api/web_api.go	WebAPI.AddApplication	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	buildDeploment	`0.00%`	`--`	`+-0.00%`

nakabonne · 2020-08-12T03:00:42Z

Fixed

nghialv · 2020-08-12T03:03:13Z

Thank you.
/approve

pipecd-bot · 2020-08-12T03:03:17Z

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

pipecd-bot · 2020-08-12T03:14:33Z

failed to handle template rule examples

Details

Error: unabled to push rendered files to destination

Enable to make git URLs

5d1dea1

pipecd-bot added release-note-none size/XL area/api area/piped labels Aug 12, 2020

pipecd-bot requested review from cakecatz and gkuga August 12, 2020 01:39

nakabonne commented Aug 12, 2020

View reviewed changes

nghialv reviewed Aug 12, 2020

View reviewed changes

Fix place where pointed out

d143434

nghialv reviewed Aug 12, 2020

View reviewed changes

pkg/git/url_test.go Outdated

func TestParseGitURL(t *testing.T) {

tests := []struct {

name string

rawurl string

Copy link

Member

nghialv Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rawURL

nghialv reviewed Aug 12, 2020

View reviewed changes

pkg/git/url_test.go Outdated

tests := []struct {

name string

rawurl string

wantU *url.URL

Copy link

Member

nghialv Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: wantURL

nghialv reviewed Aug 12, 2020

View reviewed changes

nakabonne added 2 commits August 12, 2020 11:49

Rename rawURL

e7a2c78

Rename watnURL

08cbdc4

Use testify package

b023edf

pipecd-bot added size/L and removed size/XL labels Aug 12, 2020

pipecd-bot added the approved label Aug 12, 2020

pipecd-bot merged commit 25cf0ea into master Aug 12, 2020

pipecd-bot deleted the git-urls branch August 12, 2020 03:09

This was referenced Aug 12, 2020

Allow users to specify git host #596

Closed

Support more git host #597

Closed

Allow users to specify git host #598

Closed

pipecd-bot mentioned this pull request Aug 12, 2020

Implement application list pager #600

Merged


		// Adds Repository info and then makes the GitPath URL.
		func (a WebAPI) makeGitPath(ctx context.Context, repoID, path, cfgFilename, pipedID string) (model.ApplicationGitPath, error) {

Enable to make git URLs #594

Enable to make git URLs #594

Conversation

nakabonne commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv commented Aug 12, 2020

Uh oh!

pipecd-bot commented Aug 12, 2020

1. Allow users to specify git host

This was created by todo plugin since "TODO:" was found in b023edf when #594 was merged. cc: @nakabonne.

2. Support more git host

This was created by todo plugin since "TODO:" was found in b023edf when #594 was merged. cc: @nakabonne.

3. Allow users to specify git host

This was created by todo plugin since "TODO:" was found in b023edf when #594 was merged. cc: @nakabonne.

Uh oh!

pipecd-bot commented Aug 12, 2020

Uh oh!

nakabonne commented Aug 12, 2020

Uh oh!

nghialv commented Aug 12, 2020

Uh oh!

pipecd-bot commented Aug 12, 2020

Uh oh!

pipecd-bot commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

This was created by todo plugin since "TODO:" was found in `b023edf` when #594 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `b023edf` when #594 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `b023edf` when #594 was merged. cc: @nakabonne.