Merge pull request #66 from drlau/dylan/try-parse-merge-pr-number

try parsing pr number to create apply result comment on merged pr
mercari · Mar 6, 2020 · c0d0933 · c0d0933
2 parents b030441 + 2e061d5
commit c0d0933
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 9 deletions.
diff --git a/notifier/github/commits.go b/notifier/github/commits.go
@@ -3,6 +3,8 @@ package github
 import (
 	"context"
 	"errors"
+	"strconv"
+	"strings"
 
 	"github.com/google/go-github/github"
 )
@@ -45,3 +47,23 @@ func (g *CommitsService) lastOne(commits []string, revision string) (string, err
 	// 9260c54 2018/04/05 20:16:20
 	return commits[1], nil
 }
+
+func (g *CommitsService) MergedPRNumber(revision string) (int, error) {
+	commit, _, err := g.client.API.RepositoriesGetCommit(context.Background(), revision)
+	if err != nil {
+		return 0, err
+	}
+
+	message := commit.Commit.GetMessage()
+	if !strings.HasPrefix(message, "Merge pull request #") {
+		return 0, errors.New("not a merge commit")
+	}
+
+	message = strings.TrimPrefix(message, "Merge pull request #")
+	i := strings.Index(message, " from")
+	if i >= 0 {
+		return strconv.Atoi(message[0:i])
+	}
+
+	return 0, errors.New("not a merge commit")
+}
diff --git a/notifier/github/commits_test.go b/notifier/github/commits_test.go
@@ -89,3 +89,49 @@ func TestCommitsLastOne(t *testing.T) {
 		}
 	}
 }
+
+func TestMergedPRNumber(t *testing.T) {
+	testCases := []struct {
+		prNumber int
+		ok       bool
+		revision string
+	}{
+		{
+			prNumber: 1,
+			ok:       true,
+			revision: "Merge pull request #1 from mercari/tfnotify",
+		},
+		{
+			prNumber: 123,
+			ok:       true,
+			revision: "Merge pull request #123 from mercari/tfnotify",
+		},
+		{
+			prNumber: 0,
+			ok:       false,
+			revision: "destroyed the world",
+		},
+		{
+			prNumber: 0,
+			ok:       false,
+			revision: "Merge pull request #string from mercari/tfnotify",
+		},
+	}
+
+	for _, testCase := range testCases {
+		cfg := newFakeConfig()
+		client, err := NewClient(cfg)
+		if err != nil {
+			t.Fatal(err)
+		}
+		api := newFakeAPI()
+		client.API = &api
+		prNumber, err := client.Commits.MergedPRNumber(testCase.revision)
+		if (err == nil) != testCase.ok {
+			t.Errorf("got error %q", err)
+		}
+		if prNumber != testCase.prNumber {
+			t.Errorf("got %q but want %q", prNumber, testCase.prNumber)
+		}
+	}
+}
diff --git a/notifier/github/github.go b/notifier/github/github.go
@@ -13,6 +13,7 @@ type API interface {
 	IssuesListComments(ctx context.Context, number int, opt *github.IssueListCommentsOptions) ([]*github.IssueComment, *github.Response, error)
 	RepositoriesCreateComment(ctx context.Context, sha string, comment *github.RepositoryComment) (*github.RepositoryComment, *github.Response, error)
 	RepositoriesListCommits(ctx context.Context, opt *github.CommitsListOptions) ([]*github.RepositoryCommit, *github.Response, error)
+	RepositoriesGetCommit(ctx context.Context, sha string) (*github.RepositoryCommit, *github.Response, error)
 }
 
 // GitHub represents the attribute information necessary for requesting GitHub API
@@ -45,3 +46,8 @@ func (g *GitHub) RepositoriesCreateComment(ctx context.Context, sha string, comm
 func (g *GitHub) RepositoriesListCommits(ctx context.Context, opt *github.CommitsListOptions) ([]*github.RepositoryCommit, *github.Response, error) {
 	return g.Client.Repositories.ListCommits(ctx, g.owner, g.repo, opt)
 }
+
+// RepositoriesGetCommit is a wrapper of https://godoc.org/github.com/google/go-github/github#RepositoriesService.GetCommit
+func (g *GitHub) RepositoriesGetCommit(ctx context.Context, sha string) (*github.RepositoryCommit, *github.Response, error) {
+	return g.Client.Repositories.GetCommit(ctx, g.owner, g.repo, sha)
+}
diff --git a/notifier/github/github_test.go b/notifier/github/github_test.go
@@ -14,6 +14,7 @@ type fakeAPI struct {
 	FakeIssuesListComments        func(ctx context.Context, number int, opt *github.IssueListCommentsOptions) ([]*github.IssueComment, *github.Response, error)
 	FakeRepositoriesCreateComment func(ctx context.Context, sha string, comment *github.RepositoryComment) (*github.RepositoryComment, *github.Response, error)
 	FakeRepositoriesListCommits   func(ctx context.Context, opt *github.CommitsListOptions) ([]*github.RepositoryCommit, *github.Response, error)
+	FakeRepositoriesGetCommit     func(ctx context.Context, sha string) (*github.RepositoryCommit, *github.Response, error)
 }
 
 func (g *fakeAPI) IssuesCreateComment(ctx context.Context, number int, comment *github.IssueComment) (*github.IssueComment, *github.Response, error) {
@@ -36,6 +37,10 @@ func (g *fakeAPI) RepositoriesListCommits(ctx context.Context, opt *github.Commi
 	return g.FakeRepositoriesListCommits(ctx, opt)
 }
 
+func (g *fakeAPI) RepositoriesGetCommit(ctx context.Context, sha string) (*github.RepositoryCommit, *github.Response, error) {
+	return g.FakeRepositoriesGetCommit(ctx, sha)
+}
+
 func newFakeAPI() fakeAPI {
 	return fakeAPI{
 		FakeIssuesCreateComment: func(ctx context.Context, number int, comment *github.IssueComment) (*github.IssueComment, *github.Response, error) {
@@ -83,6 +88,14 @@ func newFakeAPI() fakeAPI {
 			}
 			return commits, nil, nil
 		},
+		FakeRepositoriesGetCommit: func(ctx context.Context, sha string) (*github.RepositoryCommit, *github.Response, error) {
+			return &github.RepositoryCommit{
+				SHA: github.String(sha),
+				Commit: &github.Commit{
+					Message: github.String(sha),
+				},
+			}, nil, nil
+		},
 	}
 }
 

diff --git a/notifier/github/notify.go b/notifier/github/notify.go
@@ -52,13 +52,18 @@ func (g *NotifyService) Notify(body string) (exit int, err error) {
 	}
 
 	_, isApply := parser.(*terraform.ApplyParser)
-	if !cfg.PR.IsNumber() && isApply {
-		commits, err := g.client.Commits.List(cfg.PR.Revision)
-		if err != nil {
-			return result.ExitCode, err
+	if isApply {
+		prNumber, err := g.client.Commits.MergedPRNumber(cfg.PR.Revision)
+		if err == nil {
+			cfg.PR.Number = prNumber
+		} else if !cfg.PR.IsNumber() {
+			commits, err := g.client.Commits.List(cfg.PR.Revision)
+			if err != nil {
+				return result.ExitCode, err
+			}
+			lastRevision, _ := g.client.Commits.lastOne(commits, cfg.PR.Revision)
+			cfg.PR.Revision = lastRevision
 		}
-		lastRevision, _ := g.client.Commits.lastOne(commits, cfg.PR.Revision)
-		cfg.PR.Revision = lastRevision
 	}
 
 	return result.ExitCode, g.client.Comment.Post(body, PostOptions{

diff --git a/notifier/github/notify_test.go b/notifier/github/notify_test.go
@@ -145,7 +145,7 @@ func TestNotifyNotify(t *testing.T) {
 			exitCode: 0,
 		},
 		{
-			// apply case
+			// apply case without merge commit
 			config: Config{
 				Token: "token",
 				Owner: "owner",
@@ -162,6 +162,25 @@ func TestNotifyNotify(t *testing.T) {
 			ok:       true,
 			exitCode: 0,
 		},
+		{
+			// apply case as merge commit
+			// TODO(drlau): validate cfg.PR.Number = 123
+			config: Config{
+				Token: "token",
+				Owner: "owner",
+				Repo:  "repo",
+				PR: PullRequest{
+					Revision: "Merge pull request #123 from mercari/tfnotify",
+					Number:   0, // For apply, it is always 0
+					Message:  "message",
+				},
+				Parser:   terraform.NewApplyParser(),
+				Template: terraform.NewApplyTemplate(terraform.DefaultApplyTemplate),
+			},
+			body:     "Apply complete!",
+			ok:       true,
+			exitCode: 0,
+		},
 	}
 
 	for _, testCase := range testCases {

diff --git a/notifier/slack/notify.go b/notifier/slack/notify.go
@@ -4,8 +4,8 @@ import (
 	"context"
 	"errors"
 
-	"github.com/mercari/tfnotify/terraform"
 	"github.com/lestrrat-go/slack/objects"
+	"github.com/mercari/tfnotify/terraform"
 )
 
 // NotifyService handles communication with the notification related

diff --git a/notifier/slack/notify_test.go b/notifier/slack/notify_test.go
@@ -4,8 +4,8 @@ import (
 	"context"
 	"testing"
 
-	"github.com/mercari/tfnotify/terraform"
 	"github.com/lestrrat-go/slack/objects"
+	"github.com/mercari/tfnotify/terraform"
 )
 
 func TestNotify(t *testing.T) {