diff --git a/prow/github/client.go b/prow/github/client.go
index 33f588e37b77..0f98c09841dd 100644
--- a/prow/github/client.go
+++ b/prow/github/client.go
@@ -1014,11 +1014,11 @@ func (c *Client) ListReviews(org, repo string, number int) ([]Review, error) {
// CreateStatus creates or updates the status of a commit.
//
// See https://developer.github.com/v3/repos/statuses/#create-a-status
-func (c *Client) CreateStatus(org, repo, sha string, s Status) error {
- c.log("CreateStatus", org, repo, sha, s)
+func (c *Client) CreateStatus(org, repo, SHA string, s Status) error {
+ c.log("CreateStatus", org, repo, SHA, s)
_, err := c.request(&request{
method: http.MethodPost,
- path: fmt.Sprintf("/repos/%s/%s/statuses/%s", org, repo, sha),
+ path: fmt.Sprintf("/repos/%s/%s/statuses/%s", org, repo, SHA),
requestBody: &s,
exitCodes: []int{201},
}, nil)
@@ -1089,6 +1089,20 @@ func (c *Client) GetRepos(org string, isUser bool) ([]Repo, error) {
return repos, nil
}
+// GetSingleCommit returns a single commit.
+//
+// See https://developer.github.com/v3/repos/#get
+func (c *Client) GetSingleCommit(org, repo, SHA string) (SingleCommit, error) {
+ c.log("GetSingleCommit", org, repo, SHA)
+ var commit SingleCommit
+ _, err := c.request(&request{
+ method: http.MethodGet,
+ path: fmt.Sprintf("/repos/%s/%s/commits/%s", org, repo, SHA),
+ exitCodes: []int{200},
+ }, &commit)
+ return commit, err
+}
+
// GetBranches returns all branches in the repo.
//
// If onlyProtected is true it will only return repos with protection enabled,
@@ -1681,7 +1695,7 @@ func (e *FileNotFound) Error() string {
return fmt.Sprintf("%s/%s/%s @ %s not found", e.org, e.repo, e.path, e.commit)
}
-// GetFile uses GitHub repo contents API to retrieve the content of a file with commit sha.
+// GetFile uses GitHub repo contents API to retrieve the content of a file with commit SHA.
// If commit is empty, it will grab content from repo's default branch, usually master.
// TODO(krzyzacy): Support retrieve a directory
//
@@ -2088,7 +2102,7 @@ func (c *Client) ListIssueEvents(org, repo string, num int) ([]ListedIssueEvent,
// IsMergeable determines if a PR can be merged.
// Mergeability is calculated by a background job on GitHub and is not immediately available when
// new commits are added so the PR must be polled until the background job completes.
-func (c *Client) IsMergeable(org, repo string, number int, sha string) (bool, error) {
+func (c *Client) IsMergeable(org, repo string, number int, SHA string) (bool, error) {
backoff := time.Second * 3
maxTries := 3
for try := 0; try < maxTries; try++ {
@@ -2096,8 +2110,8 @@ func (c *Client) IsMergeable(org, repo string, number int, sha string) (bool, er
if err != nil {
return false, err
}
- if pr.Head.SHA != sha {
- return false, fmt.Errorf("pull request head changed while checking mergeability (%s -> %s)", sha, pr.Head.SHA)
+ if pr.Head.SHA != SHA {
+ return false, fmt.Errorf("pull request head changed while checking mergeability (%s -> %s)", SHA, pr.Head.SHA)
}
if pr.Merged {
return false, errors.New("pull request was merged while checking mergeability")
diff --git a/prow/github/client_test.go b/prow/github/client_test.go
index 2b08ffe44baa..d102568f0d6a 100644
--- a/prow/github/client_test.go
+++ b/prow/github/client_test.go
@@ -312,11 +312,37 @@ func TestGetRef(t *testing.T) {
}))
defer ts.Close()
c := getClient(ts.URL)
- sha, err := c.GetRef("k8s", "kuber", "heads/mastah")
+ SHA, err := c.GetRef("k8s", "kuber", "heads/mastah")
if err != nil {
t.Errorf("Didn't expect error: %v", err)
- } else if sha != "abcde" {
- t.Errorf("Wrong sha: %s", sha)
+ } else if SHA != "abcde" {
+ t.Errorf("Wrong SHA: %s", SHA)
+ }
+}
+
+func TestGetSingleCommit(t *testing.T) {
+ ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ if r.Method != http.MethodGet {
+ t.Errorf("Bad method: %s", r.Method)
+ }
+ if r.URL.Path != "/repos/octocat/Hello-World/commits/6dcb09b5b57875f334f61aebed695e2e4193db5e" {
+ t.Errorf("Bad request path: %s", r.URL.Path)
+ }
+ fmt.Fprint(w, `{
+ "commit": {
+ "tree": {
+ "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e"
+ }
+ }
+ }`)
+ }))
+ defer ts.Close()
+ c := getClient(ts.URL)
+ commit, err := c.GetSingleCommit("octocat", "Hello-World", "6dcb09b5b57875f334f61aebed695e2e4193db5e")
+ if err != nil {
+ t.Errorf("Didn't expect error: %v", err)
+ } else if commit.Commit.Tree.SHA != "6dcb09b5b57875f334f61aebed695e2e4193db5e" {
+ t.Errorf("Wrong tree-hash: %s", commit.Commit.Tree.SHA)
}
}
diff --git a/prow/github/fakegithub/fakegithub.go b/prow/github/fakegithub/fakegithub.go
index 85750525f083..8548e3930a8a 100644
--- a/prow/github/fakegithub/fakegithub.go
+++ b/prow/github/fakegithub/fakegithub.go
@@ -24,6 +24,8 @@ import (
)
const botName = "k8s-ci-robot"
+
+// Bot is the exported botName
const Bot = botName
// FakeClient is like client, but fake.
@@ -41,6 +43,7 @@ type FakeClient struct {
CombinedStatuses map[string]*github.CombinedStatus
CreatedStatuses map[string][]github.Status
IssueEvents map[int][]github.ListedIssueEvent
+ Commits map[string]github.SingleCommit
//All Labels That Exist In The Repo
ExistingLabels []string
@@ -183,12 +186,17 @@ func (f *FakeClient) GetRef(owner, repo, ref string) (string, error) {
return "abcde", nil
}
+// GetSingleCommit returns a single commit.
+func (f *FakeClient) GetSingleCommit(org, repo, SHA string) (github.SingleCommit, error) {
+ return f.Commits[SHA], nil
+}
+
// CreateStatus adds a status context to a commit.
-func (f *FakeClient) CreateStatus(owner, repo, sha string, s github.Status) error {
+func (f *FakeClient) CreateStatus(owner, repo, SHA string, s github.Status) error {
if f.CreatedStatuses == nil {
f.CreatedStatuses = make(map[string][]github.Status)
}
- statuses := f.CreatedStatuses[sha]
+ statuses := f.CreatedStatuses[SHA]
var updated bool
for i := range statuses {
if statuses[i].Context == s.Context {
@@ -199,7 +207,7 @@ func (f *FakeClient) CreateStatus(owner, repo, sha string, s github.Status) erro
if !updated {
statuses = append(statuses, s)
}
- f.CreatedStatuses[sha] = statuses
+ f.CreatedStatuses[SHA] = statuses
return nil
}
diff --git a/prow/github/types.go b/prow/github/types.go
index d45078ed5136..c7d915666e71 100644
--- a/prow/github/types.go
+++ b/prow/github/types.go
@@ -510,6 +510,16 @@ type Commit struct {
Modified []string `json:"modified"`
}
+// SingleCommit is the commit part received when requesting a single commit
+// https://developer.github.com/v3/repos/commits/#get-a-single-commit
+type SingleCommit struct {
+ Commit struct {
+ Tree struct {
+ SHA string `json:"sha"`
+ } `json:"tree"`
+ } `json:"commit"`
+}
+
// ReviewEventAction enumerates the triggers for this
// webhook payload type. See also:
// https://developer.github.com/v3/activity/events/types/#pullrequestreviewevent
diff --git a/prow/plugins.yaml b/prow/plugins.yaml
index 0a53801f956e..d3a411912dbd 100644
--- a/prow/plugins.yaml
+++ b/prow/plugins.yaml
@@ -97,6 +97,9 @@ lgtm:
- repos:
- bazelbuild
review_acts_as_lgtm: true
+- repos:
+ - kubernetes/test-infra
+ store_tree_hash: true
blockades:
- repos:
diff --git a/prow/plugins/lgtm/lgtm.go b/prow/plugins/lgtm/lgtm.go
index 56ee864c0334..3b4ade77c00f 100644
--- a/prow/plugins/lgtm/lgtm.go
+++ b/prow/plugins/lgtm/lgtm.go
@@ -35,6 +35,8 @@ import (
const PluginName = "lgtm"
var (
+ addLGTMLabelNotification = "LGTM label has been added. Git tree hash: %s "
+ addLGTMLabelNotificationRe = regexp.MustCompile(fmt.Sprintf(addLGTMLabelNotification, "(.*)"))
// LGTMLabel is the name of the lgtm label applied by the lgtm plugin
LGTMLabel = "lgtm"
lgtmRe = regexp.MustCompile(`(?mi)^/lgtm(?: no-issue)?\s*$`)
@@ -42,10 +44,14 @@ var (
removeLGTMLabelNoti = "New changes are detected. LGTM label has been removed."
)
+type commentPruner interface {
+ PruneComments(shouldPrune func(github.IssueComment) bool)
+}
+
func init() {
plugins.RegisterGenericCommentHandler(PluginName, handleGenericCommentEvent, helpProvider)
plugins.RegisterPullRequestHandler(PluginName, func(pc plugins.PluginClient, pe github.PullRequestEvent) error {
- return handlePullRequest(pc.GitHubClient, pe, pc.Logger)
+ return handlePullRequestEvent(pc, pe)
}, helpProvider)
plugins.RegisterReviewEventHandler(PluginName, handlePullRequestReviewEvent, helpProvider)
}
@@ -99,6 +105,7 @@ type githubClient interface {
ListIssueComments(org, repo string, number int) ([]github.IssueComment, error)
DeleteComment(org, repo string, ID int) error
BotName() (string, error)
+ GetSingleCommit(org, repo, SHA string) (github.SingleCommit, error)
}
// reviewCtx contains information about each review event
@@ -110,7 +117,16 @@ type reviewCtx struct {
}
func handleGenericCommentEvent(pc plugins.PluginClient, e github.GenericCommentEvent) error {
- return handleGenericComment(pc.GitHubClient, pc.PluginConfig, pc.OwnersClient, pc.Logger, e)
+ return handleGenericComment(pc.GitHubClient, pc.PluginConfig, pc.OwnersClient, pc.Logger, pc.CommentPruner, e)
+}
+
+func handlePullRequestEvent(pc plugins.PluginClient, pre github.PullRequestEvent) error {
+ return handlePullRequest(
+ pc.Logger,
+ pc.GitHubClient,
+ pc.PluginConfig,
+ &pre,
+ )
}
func handlePullRequestReviewEvent(pc plugins.PluginClient, e github.ReviewEvent) error {
@@ -119,10 +135,10 @@ func handlePullRequestReviewEvent(pc plugins.PluginClient, e github.ReviewEvent)
if !opts.ReviewActsAsLgtm {
return nil
}
- return handlePullRequestReview(pc.GitHubClient, pc.PluginConfig, pc.OwnersClient, pc.Logger, e)
+ return handlePullRequestReview(pc.GitHubClient, pc.PluginConfig, pc.OwnersClient, pc.Logger, pc.CommentPruner, e)
}
-func handleGenericComment(gc githubClient, config *plugins.Configuration, ownersClient repoowners.Interface, log *logrus.Entry, e github.GenericCommentEvent) error {
+func handleGenericComment(gc githubClient, config *plugins.Configuration, ownersClient repoowners.Interface, log *logrus.Entry, cp commentPruner, e github.GenericCommentEvent) error {
rc := reviewCtx{
author: e.User.Login,
issueAuthor: e.IssueAuthor.Login,
@@ -150,10 +166,10 @@ func handleGenericComment(gc githubClient, config *plugins.Configuration, owners
}
// use common handler to do the rest
- return handle(wantLGTM, config, ownersClient, rc, gc, log)
+ return handle(wantLGTM, config, ownersClient, rc, gc, log, cp)
}
-func handlePullRequestReview(gc githubClient, config *plugins.Configuration, ownersClient repoowners.Interface, log *logrus.Entry, e github.ReviewEvent) error {
+func handlePullRequestReview(gc githubClient, config *plugins.Configuration, ownersClient repoowners.Interface, log *logrus.Entry, cp commentPruner, e github.ReviewEvent) error {
rc := reviewCtx{
author: e.Review.User.Login,
issueAuthor: e.PullRequest.User.Login,
@@ -187,10 +203,10 @@ func handlePullRequestReview(gc githubClient, config *plugins.Configuration, own
}
// use common handler to do the rest
- return handle(wantLGTM, config, ownersClient, rc, gc, log)
+ return handle(wantLGTM, config, ownersClient, rc, gc, log, cp)
}
-func handle(wantLGTM bool, config *plugins.Configuration, ownersClient repoowners.Interface, rc reviewCtx, gc githubClient, log *logrus.Entry) error {
+func handle(wantLGTM bool, config *plugins.Configuration, ownersClient repoowners.Interface, rc reviewCtx, gc githubClient, log *logrus.Entry, cp commentPruner) error {
author := rc.author
issueAuthor := rc.issueAuthor
assignees := rc.assignees
@@ -230,9 +246,9 @@ func handle(wantLGTM bool, config *plugins.Configuration, ownersClient repoowner
if ok, merr := gc.IsCollaborator(org, repoName, author); merr == nil && !ok {
msg = fmt.Sprintf("only %s/%s repo collaborators may be assigned issues", org, repoName)
} else if merr != nil {
- log.WithError(merr).Errorf("Failed IsCollaborator(%s, %s, %s)", org, repoName, author)
+ log.WithError(merr).Errorf("Failed to check if author is a collaborator.")
} else {
- log.WithError(err).Errorf("Failed AssignIssue(%s, %s, %d, %s)", org, repoName, number, author)
+ log.WithError(err).Errorf("Failed to assign issue to author.")
}
resp := "changing LGTM is restricted to assignees, and " + msg + "."
log.Infof("Reply to assign via /lgtm request with comment: \"%s\"", resp)
@@ -263,49 +279,54 @@ func handle(wantLGTM bool, config *plugins.Configuration, ownersClient repoowner
// Only add the label if it doesn't have it, and vice versa.
labels, err := gc.GetIssueLabels(org, repoName, number)
if err != nil {
- log.WithError(err).Errorf("Failed to get the labels on %s/%s#%d.", org, repoName, number)
+ log.WithError(err).Errorf("Failed to get issue labels.")
}
hasLGTM := github.HasLabel(LGTMLabel, labels)
// remove the label if necessary, we're done after this
+ opts := optionsForRepo(config, rc.repo.Owner.Login, rc.repo.Name)
if hasLGTM && !wantLGTM {
log.Info("Removing LGTM label.")
- return gc.RemoveLabel(org, repoName, number, LGTMLabel)
- }
-
- // add the label if necessary, we're done after this
- if !hasLGTM && wantLGTM {
- log.Info("Adding LGTM label.")
- if err := gc.AddLabel(org, repoName, number, LGTMLabel); err != nil {
+ if err := gc.RemoveLabel(org, repoName, number, LGTMLabel); err != nil {
return err
}
- // Delete the LGTM removed notification after the LGTM label is added.
- botname, err := gc.BotName()
- if err != nil {
- log.WithError(err).Errorf("Failed to get bot name.")
+ if opts.StoreTreeHash {
+ cp.PruneComments(func(comment github.IssueComment) bool {
+ return addLGTMLabelNotificationRe.MatchString(comment.Body)
+ })
}
- comments, err := gc.ListIssueComments(org, repoName, number)
- if err != nil {
- log.WithError(err).Errorf("Failed to get the list of issue comments on %s/%s#%d.", org, repoName, number)
+ } else if !hasLGTM && wantLGTM {
+ log.Info("Adding LGTM label.")
+ if err := gc.AddLabel(org, repoName, number, LGTMLabel); err != nil {
+ return err
}
- for _, comment := range comments {
- if comment.User.Login == botname && comment.Body == removeLGTMLabelNoti {
- if err := gc.DeleteComment(org, repoName, comment.ID); err != nil {
- log.WithError(err).Errorf("Failed to delete comment from %s/%s#%d, ID:%d.", org, repoName, number, comment.ID)
+ if opts.StoreTreeHash {
+ pr, err := gc.GetPullRequest(org, repoName, number)
+ if err != nil {
+ log.WithError(err).Errorf("Failed to get pull request.")
+ }
+ if pr.MergeSHA != nil {
+ commit, err := gc.GetSingleCommit(org, repoName, *pr.MergeSHA)
+ if err != nil {
+ log.WithError(err).Errorf("Failed to get commit.")
+ }
+ treeHash := commit.Commit.Tree.SHA
+ log.Info("Adding comment to store tree-hash: " + treeHash)
+ if err := gc.CreateComment(org, repoName, number, fmt.Sprintf(addLGTMLabelNotification, treeHash)); err != nil {
+ log.WithError(err).Errorf("Failed to add comment.")
}
}
}
+ // Delete the LGTM removed noti after the LGTM label is added.
+ cp.PruneComments(func(comment github.IssueComment) bool {
+ return strings.Contains(comment.Body, removeLGTMLabelNoti)
+ })
}
return nil
}
-type ghLabelClient interface {
- RemoveLabel(owner, repo string, number int, label string) error
- CreateComment(owner, repo string, number int, comment string) error
-}
-
-func handlePullRequest(gc ghLabelClient, pe github.PullRequestEvent, log *logrus.Entry) error {
+func handlePullRequest(log *logrus.Entry, gc githubClient, config *plugins.Configuration, pe *github.PullRequestEvent) error {
if pe.PullRequest.Merged {
return nil
}
@@ -314,12 +335,51 @@ func handlePullRequest(gc ghLabelClient, pe github.PullRequestEvent, log *logrus
return nil
}
- // Don't bother checking if it has the label... it's a race, and we'll have
- // to handle failure due to not being labeled anyway.
org := pe.PullRequest.Base.Repo.Owner.Login
repo := pe.PullRequest.Base.Repo.Name
number := pe.PullRequest.Number
+ // rc.repo.Owner.Login, rc.repo.Name)
+ opts := optionsForRepo(config, org, repo)
+ if opts.StoreTreeHash {
+ // Check if we have LGTM label
+ labels, err := gc.GetIssueLabels(org, repo, number)
+ if err != nil {
+ log.WithError(err).Errorf("Failed to get labels.")
+ }
+
+ if github.HasLabel(LGTMLabel, labels) && pe.PullRequest.MergeSHA != nil {
+ // Check if we have a tree-hash comment
+ var lastLgtmTreeHash string
+ botname, err := gc.BotName()
+ if err != nil {
+ return err
+ }
+ comments, err := gc.ListIssueComments(org, repo, number)
+ if err != nil {
+ log.WithError(err).Errorf("Failed to get issue comments.")
+ }
+ for _, comment := range comments {
+ m := addLGTMLabelNotificationRe.FindStringSubmatch(comment.Body)
+ if comment.User.Login == botname && m != nil && comment.UpdatedAt.Equal(comment.CreatedAt) {
+ lastLgtmTreeHash = m[1]
+ break
+ }
+ }
+ // Get the current tree-hash
+ commit, err := gc.GetSingleCommit(org, repo, *pe.PullRequest.MergeSHA)
+ if err != nil {
+ log.WithError(err).Errorf("Failed to get commit.")
+ }
+ treeHash := commit.Commit.Tree.SHA
+ if treeHash == lastLgtmTreeHash {
+ // Don't remove the label, PR code hasn't changed
+ log.Infof("Keeping LGTM label as the tree-hash remained the same: %s", treeHash)
+ return nil
+ }
+ }
+ }
+
var labelNotFound bool
if err := gc.RemoveLabel(org, repo, number, LGTMLabel); err != nil {
if _, labelNotFound = err.(*github.LabelNotFound); !labelNotFound {
diff --git a/prow/plugins/lgtm/lgtm_test.go b/prow/plugins/lgtm/lgtm_test.go
index 76688d6800cc..e0d2b07e83c9 100644
--- a/prow/plugins/lgtm/lgtm_test.go
+++ b/prow/plugins/lgtm/lgtm_test.go
@@ -19,6 +19,7 @@ package lgtm
import (
"fmt"
"testing"
+ "time"
"github.com/sirupsen/logrus"
"k8s.io/apimachinery/pkg/api/equality"
@@ -50,6 +51,19 @@ type fakeRepoOwners struct {
reviewers map[string]sets.String
}
+type fakePruner struct {
+ GithubClient *fakegithub.FakeClient
+ IssueComments []github.IssueComment
+}
+
+func (fp *fakePruner) PruneComments(shouldPrune func(github.IssueComment) bool) {
+ for _, comment := range fp.IssueComments {
+ if shouldPrune(comment) {
+ fp.GithubClient.IssueCommentsDeleted = append(fp.GithubClient.IssueCommentsDeleted, comment.Body)
+ }
+ }
+}
+
var _ repoowners.RepoOwnerInterface = &fakeRepoOwners{}
func (f *fakeRepoOwners) FindApproverOwnersForFile(path string) string { return "" }
@@ -88,8 +102,7 @@ func TestLGTMComment(t *testing.T) {
shouldComment bool
shouldAssign bool
skipCollab bool
- prs map[int]*github.PullRequest
- changes map[int][]github.PullRequestChange
+ storeTreeHash bool
}{
{
name: "non-lgtm comment",
@@ -99,18 +112,20 @@ func TestLGTMComment(t *testing.T) {
shouldToggle: false,
},
{
- name: "lgtm comment by reviewer, no lgtm on pr",
- body: "/lgtm",
- commenter: "reviewer1",
- hasLGTM: false,
- shouldToggle: true,
+ name: "lgtm comment by reviewer, no lgtm on pr",
+ body: "/lgtm",
+ commenter: "reviewer1",
+ hasLGTM: false,
+ shouldToggle: true,
+ shouldComment: true,
},
{
- name: "LGTM comment by reviewer, no lgtm on pr",
- body: "/LGTM",
- commenter: "reviewer1",
- hasLGTM: false,
- shouldToggle: true,
+ name: "LGTM comment by reviewer, no lgtm on pr",
+ body: "/LGTM",
+ commenter: "reviewer1",
+ hasLGTM: false,
+ shouldToggle: true,
+ shouldComment: true,
},
{
name: "lgtm comment by reviewer, lgtm on pr",
@@ -142,7 +157,7 @@ func TestLGTMComment(t *testing.T) {
commenter: "o",
hasLGTM: false,
shouldToggle: true,
- shouldComment: false,
+ shouldComment: true,
shouldAssign: true,
},
{
@@ -151,7 +166,7 @@ func TestLGTMComment(t *testing.T) {
commenter: "o",
hasLGTM: false,
shouldToggle: true,
- shouldComment: false,
+ shouldComment: true,
shouldAssign: true,
},
{
@@ -160,7 +175,7 @@ func TestLGTMComment(t *testing.T) {
commenter: "o",
hasLGTM: false,
shouldToggle: true,
- shouldComment: false,
+ shouldComment: true,
shouldAssign: true,
},
{
@@ -169,7 +184,7 @@ func TestLGTMComment(t *testing.T) {
commenter: "o",
hasLGTM: false,
shouldToggle: true,
- shouldComment: false,
+ shouldComment: true,
shouldAssign: true,
},
{
@@ -221,32 +236,33 @@ func TestLGTMComment(t *testing.T) {
shouldToggle: false,
},
{
- name: "lgtm comment, based off OWNERS only",
- body: "/lgtm",
- commenter: "sam",
- hasLGTM: false,
- shouldToggle: true,
- skipCollab: true,
- prs: map[int]*github.PullRequest{
+ name: "lgtm comment, based off OWNERS only",
+ body: "/lgtm",
+ commenter: "sam",
+ hasLGTM: false,
+ shouldToggle: true,
+ shouldComment: true,
+ skipCollab: true,
+ },
+ }
+ SHA := "0bd3ed50c88cd53a09316bf7a298f900e9371652"
+ for _, tc := range testcases {
+ t.Logf("Running scenario %q", tc.name)
+ fc := &fakegithub.FakeClient{
+ IssueComments: make(map[int][]github.IssueComment),
+ PullRequests: map[int]*github.PullRequest{
5: {
Base: github.PullRequestBranch{
Ref: "master",
},
+ MergeSHA: &SHA,
},
},
- changes: map[int][]github.PullRequestChange{
+ PullRequestChanges: map[int][]github.PullRequestChange{
5: {
{Filename: "doc/README.md"},
},
},
- },
- }
- for _, tc := range testcases {
- t.Logf("Running scenario %q", tc.name)
- fc := &fakegithub.FakeClient{
- IssueComments: make(map[int][]github.IssueComment),
- PullRequests: tc.prs,
- PullRequestChanges: tc.changes,
}
e := &github.GenericCommentEvent{
Action: github.GenericCommentActionCreated,
@@ -268,7 +284,15 @@ func TestLGTMComment(t *testing.T) {
if tc.skipCollab {
pc.Owners.SkipCollaborators = []string{"org/repo"}
}
- if err := handleGenericComment(fc, pc, oc, logrus.WithField("plugin", PluginName), *e); err != nil {
+ pc.Lgtm = append(pc.Lgtm, plugins.Lgtm{
+ Repos: []string{"org/repo"},
+ StoreTreeHash: true,
+ })
+ fp := &fakePruner{
+ GithubClient: fc,
+ IssueComments: fc.IssueComments[5],
+ }
+ if err := handleGenericComment(fc, pc, oc, logrus.WithField("plugin", PluginName), fp, *e); err != nil {
t.Errorf("didn't expect error from lgtmComment: %v", err)
continue
}
@@ -381,9 +405,15 @@ func TestLGTMCommentWithLGTMNoti(t *testing.T) {
shouldDelete: false,
},
}
+ SHA := "0bd3ed50c88cd53a09316bf7a298f900e9371652"
for _, tc := range testcases {
fc := &fakegithub.FakeClient{
IssueComments: make(map[int][]github.IssueComment),
+ PullRequests: map[int]*github.PullRequest{
+ 5: {
+ MergeSHA: &SHA,
+ },
+ },
}
e := &github.GenericCommentEvent{
Action: github.GenericCommentActionCreated,
@@ -410,23 +440,27 @@ func TestLGTMCommentWithLGTMNoti(t *testing.T) {
fc.IssueComments[5] = append(fc.IssueComments[5], ic)
oc := &fakeOwnersClient{approvers: approvers, reviewers: reviewers}
pc := &plugins.Configuration{}
- if err := handleGenericComment(fc, pc, oc, logrus.WithField("plugin", PluginName), *e); err != nil {
+ fp := &fakePruner{
+ GithubClient: fc,
+ IssueComments: fc.IssueComments[5],
+ }
+ if err := handleGenericComment(fc, pc, oc, logrus.WithField("plugin", PluginName), fp, *e); err != nil {
t.Errorf("For case %s, didn't expect error from lgtmComment: %v", tc.name, err)
continue
}
- found := false
- for _, v := range fc.IssueComments[5] {
- if v.User.Login == botName && v.Body == removeLGTMLabelNoti {
- found = true
+ deleted := false
+ for _, body := range fc.IssueCommentsDeleted {
+ if body == removeLGTMLabelNoti {
+ deleted = true
break
}
}
if tc.shouldDelete {
- if found {
+ if !deleted {
t.Errorf("For case %s, LGTM removed notification should have been deleted", tc.name)
}
} else {
- if !found {
+ if deleted {
t.Errorf("For case %s, LGTM removed notification should not have been deleted", tc.name)
}
}
@@ -443,6 +477,7 @@ func TestLGTMFromApproveReview(t *testing.T) {
shouldToggle bool
shouldComment bool
shouldAssign bool
+ storeTreeHash bool
}{
{
name: "Request changes review by reviewer, no lgtm on pr",
@@ -462,11 +497,21 @@ func TestLGTMFromApproveReview(t *testing.T) {
shouldAssign: false,
},
{
- name: "Approve review by reviewer, no lgtm on pr",
- state: github.ReviewStateApproved,
- reviewer: "reviewer1",
- hasLGTM: false,
- shouldToggle: true,
+ name: "Approve review by reviewer, no lgtm on pr",
+ state: github.ReviewStateApproved,
+ reviewer: "reviewer1",
+ hasLGTM: false,
+ shouldToggle: true,
+ shouldComment: true,
+ storeTreeHash: true,
+ },
+ {
+ name: "Approve review by reviewer, no lgtm on pr, do not store tree_hash",
+ state: github.ReviewStateApproved,
+ reviewer: "reviewer1",
+ hasLGTM: false,
+ shouldToggle: true,
+ shouldComment: false,
},
{
name: "Approve review by reviewer, lgtm on pr",
@@ -482,8 +527,9 @@ func TestLGTMFromApproveReview(t *testing.T) {
reviewer: "o",
hasLGTM: false,
shouldToggle: true,
- shouldComment: false,
+ shouldComment: true,
shouldAssign: true,
+ storeTreeHash: true,
},
{
name: "Request changes review by non-reviewer, no lgtm on pr",
@@ -533,9 +579,16 @@ func TestLGTMFromApproveReview(t *testing.T) {
shouldAssign: false,
},
}
+ SHA := "0bd3ed50c88cd53a09316bf7a298f900e9371652"
for _, tc := range testcases {
fc := &fakegithub.FakeClient{
IssueComments: make(map[int][]github.IssueComment),
+ LabelsAdded: []string{},
+ PullRequests: map[int]*github.PullRequest{
+ 5: {
+ MergeSHA: &SHA,
+ },
+ },
}
e := &github.ReviewEvent{
Review: github.Review{Body: tc.body, State: tc.state, HTMLURL: "", User: github.User{Login: tc.reviewer}},
@@ -543,11 +596,19 @@ func TestLGTMFromApproveReview(t *testing.T) {
Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"},
}
if tc.hasLGTM {
- fc.LabelsAdded = []string{"org/repo#5:" + LGTMLabel}
+ fc.LabelsAdded = append(fc.LabelsAdded, "org/repo#5:"+LGTMLabel)
}
oc := &fakeOwnersClient{approvers: approvers, reviewers: reviewers}
pc := &plugins.Configuration{}
- if err := handlePullRequestReview(fc, pc, oc, logrus.WithField("plugin", PluginName), *e); err != nil {
+ pc.Lgtm = append(pc.Lgtm, plugins.Lgtm{
+ Repos: []string{"org/repo"},
+ StoreTreeHash: tc.storeTreeHash,
+ })
+ fp := &fakePruner{
+ GithubClient: fc,
+ IssueComments: fc.IssueComments[5],
+ }
+ if err := handlePullRequestReview(fc, pc, oc, logrus.WithField("plugin", PluginName), fp, *e); err != nil {
t.Errorf("For case %s, didn't expect error from pull request review: %v", tc.name, err)
continue
}
@@ -592,37 +653,9 @@ func TestLGTMFromApproveReview(t *testing.T) {
}
}
-type fakeIssueComment struct {
- Owner string
- Repo string
- Number int
- Comment string
-}
-
-type githubUnlabeler struct {
- labelsRemoved []string
- issueComments []fakeIssueComment
- removeLabelErr error
- createCommentErr error
-}
-
-func (c *githubUnlabeler) RemoveLabel(owner, repo string, pr int, label string) error {
- c.labelsRemoved = append(c.labelsRemoved, label)
- return c.removeLabelErr
-}
-
-func (c *githubUnlabeler) CreateComment(owner, repo string, number int, comment string) error {
- ic := fakeIssueComment{
- Owner: owner,
- Repo: repo,
- Number: number,
- Comment: comment,
- }
- c.issueComments = append(c.issueComments, ic)
- return c.createCommentErr
-}
-
func TestHandlePullRequest(t *testing.T) {
+ SHA := "0bd3ed50c88cd53a09316bf7a298f900e9371652"
+ treeSHA := "6dcb09b5b57875f334f61aebed695e2e4193db5e"
cases := []struct {
name string
event github.PullRequestEvent
@@ -630,8 +663,9 @@ func TestHandlePullRequest(t *testing.T) {
createCommentErr error
err error
+ labelsAdded []string
labelsRemoved []string
- issueComments []fakeIssueComment
+ issueComments map[int][]github.IssueComment
expectNoComments bool
}{
@@ -649,15 +683,16 @@ func TestHandlePullRequest(t *testing.T) {
Name: "kubernetes",
},
},
+ MergeSHA: &SHA,
},
},
labelsRemoved: []string{LGTMLabel},
- issueComments: []fakeIssueComment{
- {
- Owner: "kubernetes",
- Repo: "kubernetes",
- Number: 101,
- Comment: removeLGTMLabelNoti,
+ issueComments: map[int][]github.IssueComment{
+ 101: {
+ {
+ Body: removeLGTMLabelNoti,
+ User: github.User{Login: fakegithub.Bot},
+ },
},
},
expectNoComments: false,
@@ -670,7 +705,7 @@ func TestHandlePullRequest(t *testing.T) {
expectNoComments: true,
},
{
- name: "pr_synchronize, with RemoveLabel github.LabelNotFound error",
+ name: "pr_synchronize, same tree-hash, keep label",
event: github.PullRequestEvent{
Action: github.PullRequestActionSynchronize,
PullRequest: github.PullRequest{
@@ -683,26 +718,80 @@ func TestHandlePullRequest(t *testing.T) {
Name: "kubernetes",
},
},
+ MergeSHA: &SHA,
},
},
- removeLabelErr: &github.LabelNotFound{
- Owner: "kubernetes",
- Repo: "kubernetes",
- Number: 101,
- Label: LGTMLabel,
+ issueComments: map[int][]github.IssueComment{
+ 101: {
+ {
+ Body: fmt.Sprintf(addLGTMLabelNotification, treeSHA),
+ User: github.User{Login: fakegithub.Bot},
+ },
+ },
},
- labelsRemoved: []string{LGTMLabel},
expectNoComments: true,
},
+ {
+ name: "pr_synchronize, same tree-hash, keep label, edited comment",
+ event: github.PullRequestEvent{
+ Action: github.PullRequestActionSynchronize,
+ PullRequest: github.PullRequest{
+ Number: 101,
+ Base: github.PullRequestBranch{
+ Repo: github.Repo{
+ Owner: github.User{
+ Login: "kubernetes",
+ },
+ Name: "kubernetes",
+ },
+ },
+ MergeSHA: &SHA,
+ },
+ },
+ labelsRemoved: []string{LGTMLabel},
+ issueComments: map[int][]github.IssueComment{
+ 101: {
+ {
+ Body: fmt.Sprintf(addLGTMLabelNotification, treeSHA),
+ User: github.User{Login: fakegithub.Bot},
+ CreatedAt: time.Date(1981, 2, 21, 12, 30, 0, 0, time.UTC),
+ UpdatedAt: time.Date(1981, 2, 21, 12, 31, 0, 0, time.UTC),
+ },
+ },
+ },
+ expectNoComments: false,
+ },
}
-
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
- fakeGitHub := &githubUnlabeler{
- removeLabelErr: c.removeLabelErr,
- createCommentErr: c.createCommentErr,
+ fakeGitHub := &fakegithub.FakeClient{
+ IssueComments: c.issueComments,
+ PullRequests: map[int]*github.PullRequest{
+ 101: {
+ Base: github.PullRequestBranch{
+ Ref: "master",
+ },
+ MergeSHA: &SHA,
+ },
+ },
+ Commits: make(map[string]github.SingleCommit),
+ LabelsAdded: c.labelsAdded,
}
- err := handlePullRequest(fakeGitHub, c.event, logrus.WithField("plugin", PluginName))
+ fakeGitHub.LabelsAdded = append(fakeGitHub.LabelsAdded, "kubernetes/kubernetes#101:lgtm")
+ commit := github.SingleCommit{}
+ commit.Commit.Tree.SHA = treeSHA
+ fakeGitHub.Commits[SHA] = commit
+ pc := &plugins.Configuration{}
+ pc.Lgtm = append(pc.Lgtm, plugins.Lgtm{
+ Repos: []string{"kubernetes/kubernetes"},
+ StoreTreeHash: true,
+ })
+ err := handlePullRequest(
+ logrus.WithField("plugin", "approve"),
+ fakeGitHub,
+ pc,
+ &c.event,
+ )
if err != nil && c.err == nil {
t.Fatalf("handlePullRequest error: %v", err)
@@ -716,20 +805,116 @@ func TestHandlePullRequest(t *testing.T) {
t.Fatalf("handlePullRequest error mismatch: got %v, want %v", got, want)
}
- if got, want := len(fakeGitHub.labelsRemoved), len(c.labelsRemoved); got != want {
- t.Logf("labelsRemoved: got %v, want: %v", fakeGitHub.labelsRemoved, c.labelsRemoved)
+ if got, want := len(fakeGitHub.LabelsRemoved), len(c.labelsRemoved); got != want {
+ t.Logf("labelsRemoved: got %v, want: %v", fakeGitHub.LabelsRemoved, c.labelsRemoved)
t.Fatalf("labelsRemoved length mismatch: got %d, want %d", got, want)
}
- if got, want := fakeGitHub.issueComments, c.issueComments; !equality.Semantic.DeepEqual(got, want) {
+ if got, want := fakeGitHub.IssueComments, c.issueComments; !equality.Semantic.DeepEqual(got, want) {
t.Fatalf("LGTM revmoved notifications mismatch: got %v, want %v", got, want)
}
- if c.expectNoComments && len(fakeGitHub.issueComments) > 0 {
- t.Fatalf("expected no comments but got %v", fakeGitHub.issueComments)
+ if c.expectNoComments && len(fakeGitHub.IssueCommentsAdded) > 0 {
+ t.Fatalf("expected no comments but got %v", fakeGitHub.IssueCommentsAdded)
}
- if !c.expectNoComments && len(fakeGitHub.issueComments) == 0 {
+ if !c.expectNoComments && len(fakeGitHub.IssueCommentsAdded) == 0 {
t.Fatalf("expected comments but got none")
}
})
}
}
+
+func TestAddTreeHashComment(t *testing.T) {
+ SHA := "0bd3ed50c88cd53a09316bf7a298f900e9371652"
+ treeSHA := "6dcb09b5b57875f334f61aebed695e2e4193db5e"
+ pc := &plugins.Configuration{}
+ pc.Lgtm = append(pc.Lgtm, plugins.Lgtm{
+ Repos: []string{"kubernetes/kubernetes"},
+ StoreTreeHash: true,
+ })
+ rc := reviewCtx{
+ author: "alice",
+ issueAuthor: "bob",
+ repo: github.Repo{
+ Owner: github.User{
+ Login: "kubernetes",
+ },
+ Name: "kubernetes",
+ },
+ number: 101,
+ body: "/lgtm",
+ }
+ fc := &fakegithub.FakeClient{
+ Commits: make(map[string]github.SingleCommit),
+ IssueComments: map[int][]github.IssueComment{},
+ PullRequests: map[int]*github.PullRequest{
+ 101: {
+ Base: github.PullRequestBranch{
+ Ref: "master",
+ },
+ MergeSHA: &SHA,
+ },
+ },
+ }
+ commit := github.SingleCommit{}
+ commit.Commit.Tree.SHA = treeSHA
+ fc.Commits[SHA] = commit
+ handle(true, pc, &fakeOwnersClient{}, rc, fc, logrus.WithField("plugin", PluginName), &fakePruner{})
+ found := false
+ for _, body := range fc.IssueCommentsAdded {
+ if addLGTMLabelNotificationRe.MatchString(body) {
+ found = true
+ break
+ }
+ }
+ if !found {
+ t.Fatalf("expected tree_hash comment but got none")
+ }
+}
+
+func TestRemoveTreeHashComment(t *testing.T) {
+ treeSHA := "6dcb09b5b57875f334f61aebed695e2e4193db5e"
+ pc := &plugins.Configuration{}
+ pc.Lgtm = append(pc.Lgtm, plugins.Lgtm{
+ Repos: []string{"kubernetes/kubernetes"},
+ StoreTreeHash: true,
+ })
+ rc := reviewCtx{
+ author: "alice",
+ issueAuthor: "bob",
+ repo: github.Repo{
+ Owner: github.User{
+ Login: "kubernetes",
+ },
+ Name: "kubernetes",
+ },
+ assignees: []github.User{{Login: "alice"}},
+ number: 101,
+ body: "/lgtm cancel",
+ }
+ fc := &fakegithub.FakeClient{
+ IssueComments: map[int][]github.IssueComment{
+ 101: {
+ {
+ Body: fmt.Sprintf(addLGTMLabelNotification, treeSHA),
+ User: github.User{Login: fakegithub.Bot},
+ },
+ },
+ },
+ }
+ fc.LabelsAdded = []string{"kubernetes/kubernetes#101:" + LGTMLabel}
+ fp := &fakePruner{
+ GithubClient: fc,
+ IssueComments: fc.IssueComments[101],
+ }
+ handle(false, pc, &fakeOwnersClient{}, rc, fc, logrus.WithField("plugin", PluginName), fp)
+ found := false
+ for _, body := range fc.IssueCommentsDeleted {
+ if addLGTMLabelNotificationRe.MatchString(body) {
+ found = true
+ break
+ }
+ }
+ if !found {
+ t.Fatalf("expected deleted tree_hash comment but got none")
+ }
+}
diff --git a/prow/plugins/plugins.go b/prow/plugins/plugins.go
index e8dcc82b7a18..73b0f6f86120 100644
--- a/prow/plugins/plugins.go
+++ b/prow/plugins/plugins.go
@@ -366,6 +366,9 @@ type Lgtm struct {
// ReviewActsAsLgtm indicates that a Github review of "approve" or "request changes"
// acts as adding or removing the lgtm label
ReviewActsAsLgtm bool `json:"review_acts_as_lgtm,omitempty"`
+ // StoreTreeHash indicates if tree_hash should be stored inside a comment to detect
+ // squashed commits before removing lgtm labels
+ StoreTreeHash bool `json:"store_tree_hash,omitempty"`
}
type Cat struct {