diff --git a/contrib/submit-queue/github/github.go b/contrib/submit-queue/github/github.go index 73e089d6795..f1a4d88f18f 100644 --- a/contrib/submit-queue/github/github.go +++ b/contrib/submit-queue/github/github.go @@ -86,7 +86,22 @@ type FilterConfig struct { RequiredStatusContexts []string } -func validateLGTMAfterPush(client *github.Client, user, project string, pr *github.PullRequest) (bool, error) { +func lastModifiedTime(client *github.Client, user, project string, pr *github.PullRequest) (*time.Time, error) { + list, _, err := client.PullRequests.ListCommits(user, project, *pr.Number, &github.ListOptions{}) + if err != nil { + return nil, err + } + var lastModified *time.Time + for ix := range list { + item := list[ix] + if lastModified == nil || item.Commit.Committer.Date.After(*lastModified) { + lastModified = item.Commit.Committer.Date + } + } + return lastModified, nil +} + +func validateLGTMAfterPush(client *github.Client, user, project string, pr *github.PullRequest, lastModifiedTime *time.Time) (bool, error) { var lgtmTime *time.Time events, _, err := client.Issues.ListIssueEvents(user, project, *pr.Number, &github.ListOptions{}) if err != nil { @@ -102,10 +117,7 @@ func validateLGTMAfterPush(client *github.Client, user, project string, pr *gith if lgtmTime == nil { return false, fmt.Errorf("Couldn't find time for LGTM label, this shouldn't happen, skipping PR: %d", *pr.Number) } - if pr.Head == nil || pr.Head.Repo == nil || pr.Head.Repo.PushedAt == nil { - return false, fmt.Errorf("Couldn't find push time for PR, this shouldn't happen, skipping PR: %d", *pr.Number) - } - return pr.Head.Repo.PushedAt.Before(*lgtmTime), nil + return lastModifiedTime.Before(*lgtmTime), nil } // For each PR in the project that matches: @@ -156,7 +168,12 @@ func ForEachCandidatePRDo(client *github.Client, user, project string, fn PRFunc continue } - if ok, err := validateLGTMAfterPush(client, user, project, pr); err != nil { + lastModifiedTime, err := lastModifiedTime(client, user, project, pr) + if err != nil { + glog.Errorf("Failed to get last modified time, skipping PR: %d", *pr.Number) + continue + } + if ok, err := validateLGTMAfterPush(client, user, project, pr, lastModifiedTime); err != nil { glog.Errorf("Error validating LGTM: %v, Skipping: %d", err, *pr.Number) continue } else if !ok { diff --git a/contrib/submit-queue/github/github_test.go b/contrib/submit-queue/github/github_test.go index 382a4da9b45..f0c8b759992 100644 --- a/contrib/submit-queue/github/github_test.go +++ b/contrib/submit-queue/github/github_test.go @@ -394,9 +394,9 @@ func TestComputeStatus(t *testing.T) { func TestValidateLGTMAfterPush(t *testing.T) { tests := []struct { - issueEvents []github.IssueEvent - shouldPass bool - pull github.PullRequest + issueEvents []github.IssueEvent + shouldPass bool + lastModified time.Time }{ { issueEvents: []github.IssueEvent{ @@ -408,15 +408,8 @@ func TestValidateLGTMAfterPush(t *testing.T) { CreatedAt: timePtr(time.Unix(10, 0)), }, }, - pull: github.PullRequest{ - Number: intPtr(1), - Head: &github.PullRequestBranch{ - Repo: &github.Repository{ - PushedAt: &github.Timestamp{time.Unix(9, 0)}, - }, - }, - }, - shouldPass: true, + lastModified: time.Unix(9, 0), + shouldPass: true, }, { issueEvents: []github.IssueEvent{ @@ -428,20 +421,13 @@ func TestValidateLGTMAfterPush(t *testing.T) { CreatedAt: timePtr(time.Unix(10, 0)), }, }, - pull: github.PullRequest{ - Number: intPtr(1), - Head: &github.PullRequestBranch{ - Repo: &github.Repository{ - PushedAt: &github.Timestamp{time.Unix(11, 0)}, - }, - }, - }, - shouldPass: false, + lastModified: time.Unix(11, 0), + shouldPass: false, }, } for _, test := range tests { client, server, mux := initTest() - mux.HandleFunc(fmt.Sprintf("/repos/o/r/issues/%d/events", test.pull.Number), func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc(fmt.Sprintf("/repos/o/r/issues/1/events"), func(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { t.Errorf("Unexpected method: %s", r.Method) } @@ -451,7 +437,7 @@ func TestValidateLGTMAfterPush(t *testing.T) { t.Errorf("Unexpected error: %v", err) } w.Write(data) - ok, err := validateLGTMAfterPush(client, "o", "r", &test.pull) + ok, err := validateLGTMAfterPush(client, "o", "r", &github.PullRequest{Number: intPtr(1)}, &test.lastModified) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -462,3 +448,123 @@ func TestValidateLGTMAfterPush(t *testing.T) { server.Close() } } + +func TestGetLastModified(t *testing.T) { + tests := []struct { + commits []github.RepositoryCommit + expectedTime *time.Time + }{ + { + commits: []github.RepositoryCommit{ + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(10, 0)), + }, + }, + }, + }, + expectedTime: timePtr(time.Unix(10, 0)), + }, + { + commits: []github.RepositoryCommit{ + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(10, 0)), + }, + }, + }, + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(11, 0)), + }, + }, + }, + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(12, 0)), + }, + }, + }, + }, + expectedTime: timePtr(time.Unix(12, 0)), + }, + { + commits: []github.RepositoryCommit{ + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(10, 0)), + }, + }, + }, + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(9, 0)), + }, + }, + }, + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(8, 0)), + }, + }, + }, + }, + expectedTime: timePtr(time.Unix(10, 0)), + }, + { + commits: []github.RepositoryCommit{ + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(9, 0)), + }, + }, + }, + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(10, 0)), + }, + }, + }, + { + Commit: &github.Commit{ + Committer: &github.CommitAuthor{ + Date: timePtr(time.Unix(9, 0)), + }, + }, + }, + }, + expectedTime: timePtr(time.Unix(10, 0)), + }, + } + for _, test := range tests { + client, server, mux := initTest() + mux.HandleFunc(fmt.Sprintf("/repos/o/r/pulls/1/commits"), func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Errorf("Unexpected method: %s", r.Method) + } + w.WriteHeader(http.StatusOK) + data, err := json.Marshal(test.commits) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + w.Write(data) + ts, err := lastModifiedTime(client, "o", "r", &github.PullRequest{Number: intPtr(1)}) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if !ts.Equal(*test.expectedTime) { + t.Errorf("expected: %v, saw: %v", test.expectedTime, ts) + } + }) + server.Close() + } +}