From c137b462cff827b0bd67c9113d8f8c16f7e13a30 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Thu, 13 Dec 2018 11:17:46 -0800 Subject: [PATCH] Add IgnoreUpdateMerges option (#39) When this option is true, a commit on the PR branch is ignored if it meets these conditions: 1. Has two parents (i.e. is a simple merge commit) 2. Was created via the UI or the API 3. Has one parent in the last 100 commits on the target branch An ignored commit does not invalidate approvals and the author/committer do not count as contributors. Also refactor the approval test code to avoid destructively modifying the default pull.Context data. --- README.md | 26 +++ policy/approval/approve.go | 66 +++++++- policy/approval/approve_test.go | 276 ++++++++++++++++++++------------ pull/context.go | 10 +- pull/github.go | 131 +++++++++++---- pull/pulltest/context.go | 7 + 6 files changed, 379 insertions(+), 137 deletions(-) diff --git a/README.md b/README.md index 582fbe0a..29f235a1 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ UI to view the detailed approval status of any pull request. - [Disapproval is Disabled by Default](#disapproval-is-disabled-by-default) - [`or`, `and`, and `if` (Rule Predicates)](#or-and-and-if-rule-predicates) - [Cross-organization Membership Tests](#cross-organization-membership-tests) + - [Update Merges](#update-merges) * [Deployment](#deployment) * [Development](#development) * [Contributing](#contributing) @@ -163,6 +164,13 @@ options: # approvals for this rule. False by default. invalidate_on_push: false + # If true, "update merges" do not invalidate approval (if invalidate_on_push + # is enabled) and their authors/committers do not count as contributors. An + # "update merge" is a merge commit that was created in the UI or via the API + # and merges the target branch into the pull request branch. These are + # commonly created by using the "Update branch" button in the UI. + ignore_update_merges: false + # "methods" defines how users may express approval. The defaults are below. methods: comments: @@ -294,6 +302,24 @@ Effectively, skipped rules are treated as if they don't exist. not in the organization that owns the repository where the rules appear. In this case, `policy-bot` must be installed on all referenced organizations. +#### Update Merges + +For a commit on a branch to count as an "update merge" for the purpose of the +`ignore_update_merges` option, the following must be true: + +1. The commit must have exactly two parents +2. The commit must have the `committedViaWeb` property set to `true` +3. One parent must exist in the last 100 commits on the target branch of the + pull request + +These will all be true after updating a branch using the UI, but historic +merges on long-running branches or merges created with the API may not be +ignored. If this happens, you will need to reapprove the pull request. + +Note that `policy-bot` cannot detect if an update merge contains any merge +conflict resolutions. If you enable this option, users _may_ be able to merge +unapproved code by exploiting the conflict editor. + ## Deployment `policy-bot` is easy to deploy in your own environment as it has no dependencies diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 57fc3bca..25d4d04c 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -45,9 +45,10 @@ type Rule struct { } type Options struct { - AllowAuthor bool `yaml:"allow_author"` - AllowContributor bool `yaml:"allow_contributor"` - InvalidateOnPush bool `yaml:"invalidate_on_push"` + AllowAuthor bool `yaml:"allow_author"` + AllowContributor bool `yaml:"allow_contributor"` + InvalidateOnPush bool `yaml:"invalidate_on_push"` + IgnoreUpdateMerges bool `yaml:"ignore_update_merges"` Methods *common.Methods `yaml:"methods"` } @@ -123,9 +124,9 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string sort.Stable(common.CandidatesByCreationTime(candidates)) if r.Options.InvalidateOnPush { - commits, err := prctx.Commits() + commits, err := r.filteredCommits(prctx) if err != nil { - return false, "", errors.Wrap(err, "failed to get commits") + return false, "", err } lastCommitTime := commits[len(commits)-1].CreatedAt @@ -157,7 +158,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string // "contributor" is any user who added a commit to the PR if !r.Options.AllowContributor { - commits, err := prctx.Commits() + commits, err := r.filteredCommits(prctx) if err != nil { return false, "", err } @@ -211,10 +212,61 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string return false, msg, nil } +func (r *Rule) filteredCommits(prctx pull.Context) ([]*pull.Commit, error) { + commits, err := prctx.Commits() + if err != nil { + return nil, errors.Wrap(err, "failed to list commits") + } + + needsFiltering := r.Options.IgnoreUpdateMerges + if !needsFiltering { + return commits, nil + } + + var filtered []*pull.Commit + for _, c := range commits { + isUpdate, err := isUpdateMerge(prctx, c) + if err != nil { + return nil, errors.Wrap(err, "failed to detemine update merge status") + } + + switch { + case isUpdate: + default: + filtered = append(filtered, c) + } + } + return filtered, nil +} + +func isUpdateMerge(prctx pull.Context, c *pull.Commit) (bool, error) { + // must be a simple merge commit (exactly 2 parents) + if len(c.Parents) != 2 { + return false, nil + } + + // must be created via the UI or the API (no local merges) + if !c.CommittedViaWeb { + return false, nil + } + + // one parent must exist in recent history on the target branch + targets, err := prctx.TargetCommits() + if err != nil { + return false, err + } + for _, target := range targets { + if c.Parents[0] == target.SHA || c.Parents[1] == target.SHA { + return true, nil + } + } + + return false, nil +} + func numberOfApprovals(count int) string { if count == 1 { return "1 approval" } - return fmt.Sprintf("%d approvals", count) } diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index d79a77ee..9d37bc24 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -34,84 +34,100 @@ func TestIsApproved(t *testing.T) { ctx := logger.WithContext(context.Background()) now := time.Now() - prctx := &pulltest.Context{ - AuthorValue: "mhaypenny", - CommentsValue: []*pull.Comment{ - { - CreatedAt: now.Add(10 * time.Second), - Author: "other-user", - Body: "Why did you do this?", - }, - { - CreatedAt: now.Add(20 * time.Second), - Author: "comment-approver", - Body: "LGTM :+1: :shipit:", - }, - { - CreatedAt: now.Add(30 * time.Second), - Author: "disapprover", - Body: "I don't like things! :-1:", - }, - { - CreatedAt: now.Add(40 * time.Second), - Author: "mhaypenny", - Body: ":+1: my stuff is cool", - }, - { - CreatedAt: now.Add(50 * time.Second), - Author: "contributor-author", - Body: ":+1: I added to this PR", - }, - { - CreatedAt: now.Add(60 * time.Second), - Author: "contributor-committer", - Body: ":+1: I also added to this PR", - }, - }, - ReviewsValue: []*pull.Review{ - { - CreatedAt: now.Add(70 * time.Second), - Author: "disapprover", - State: pull.ReviewChangesRequested, - Body: "I _really_ don't like things!", + basePullContext := func() *pulltest.Context { + return &pulltest.Context{ + AuthorValue: "mhaypenny", + CommentsValue: []*pull.Comment{ + { + CreatedAt: now.Add(10 * time.Second), + Author: "other-user", + Body: "Why did you do this?", + }, + { + CreatedAt: now.Add(20 * time.Second), + Author: "comment-approver", + Body: "LGTM :+1: :shipit:", + }, + { + CreatedAt: now.Add(30 * time.Second), + Author: "disapprover", + Body: "I don't like things! :-1:", + }, + { + CreatedAt: now.Add(40 * time.Second), + Author: "mhaypenny", + Body: ":+1: my stuff is cool", + }, + { + CreatedAt: now.Add(50 * time.Second), + Author: "contributor-author", + Body: ":+1: I added to this PR", + }, + { + CreatedAt: now.Add(60 * time.Second), + Author: "contributor-committer", + Body: ":+1: I also added to this PR", + }, }, - { - CreatedAt: now.Add(80 * time.Second), - Author: "review-approver", - State: pull.ReviewApproved, - Body: "I LIKE THIS", + ReviewsValue: []*pull.Review{ + { + CreatedAt: now.Add(70 * time.Second), + Author: "disapprover", + State: pull.ReviewChangesRequested, + Body: "I _really_ don't like things!", + }, + { + CreatedAt: now.Add(80 * time.Second), + Author: "review-approver", + State: pull.ReviewApproved, + Body: "I LIKE THIS", + }, }, - }, - CommitsValue: []*pull.Commit{ - { - CreatedAt: now.Add(90 * time.Second), - SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb", - Author: "mhaypenny", - Committer: "mhaypenny", + CommitsValue: []*pull.Commit{ + { + CreatedAt: now.Add(5 * time.Second), + SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb", + Author: "mhaypenny", + Committer: "mhaypenny", + }, + { + CreatedAt: now.Add(15 * time.Second), + SHA: "674832587eaaf416371b30f5bc5a47e377f534ec", + Author: "contributor-author", + Committer: "mhaypenny", + }, + { + CreatedAt: now.Add(45 * time.Second), + SHA: "97d5ea26da319a987d80f6db0b7ef759f2f2e441", + Author: "mhaypenny", + Committer: "contributor-committer", + }, }, - { - CreatedAt: now.Add(100 * time.Second), - SHA: "674832587eaaf416371b30f5bc5a47e377f534ec", - Author: "contributor-author", - Committer: "mhaypenny", + TargetCommitsValue: []*pull.Commit{ + { + CreatedAt: now.Add(5 * time.Second), + SHA: "dc594ff5aca4133070a76f9006568b656a251770", + Author: "developer", + Committer: "developer", + }, + { + CreatedAt: now.Add(0 * time.Second), + SHA: "2e1b0bb6ab144bf7a1b7a1df9d3bdcb0fe85a206", + Author: "developer", + Committer: "developer", + }, }, - { - CreatedAt: now.Add(110 * time.Second), - SHA: "97d5ea26da319a987d80f6db0b7ef759f2f2e441", - Author: "mhaypenny", - Committer: "contributor-committer", + OrgMemberships: map[string][]string{ + "mhaypenny": {"everyone"}, + "contributor-author": {"everyone"}, + "contributor-committer": {"everyone"}, + "comment-approver": {"everyone", "cool-org"}, + "review-approver": {"everyone", "even-cooler-org"}, }, - }, - OrgMemberships: map[string][]string{ - "mhaypenny": {"everyone"}, - "contributor-author": {"everyone"}, - "contributor-committer": {"everyone"}, - "comment-approver": {"everyone", "cool-org"}, - "review-approver": {"everyone", "even-cooler-org"}, - }, + } } - assertApproved := func(t *testing.T, r *Rule, expected string) { + assertApproved := func(t *testing.T, prctx pull.Context, r *Rule, expected string) { approved, msg, err := r.IsApproved(ctx, prctx) require.NoError(t, err) @@ -120,7 +136,7 @@ func TestIsApproved(t *testing.T) { } } - assertPending := func(t *testing.T, r *Rule, expected string) { + assertPending := func(t *testing.T, prctx pull.Context, r *Rule, expected string) { approved, msg, err := r.IsApproved(ctx, prctx) require.NoError(t, err) @@ -130,20 +146,23 @@ func TestIsApproved(t *testing.T) { } t.Run("noApprovalRequired", func(t *testing.T) { + prctx := basePullContext() r := &Rule{} - assertApproved(t, r, "No approval required") + assertApproved(t, prctx, r, "No approval required") }) t.Run("singleApprovalRequired", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Requires: Requires{ Count: 1, }, } - assertPending(t, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") }) t.Run("authorCannotApprove", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Options: Options{ AllowAuthor: false, @@ -155,10 +174,11 @@ func TestIsApproved(t *testing.T) { }, }, } - assertApproved(t, r, "Approved by comment-approver, review-approver") + assertApproved(t, prctx, r, "Approved by comment-approver, review-approver") }) t.Run("contributorsCannotApprove", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Options: Options{ AllowContributor: false, @@ -170,10 +190,11 @@ func TestIsApproved(t *testing.T) { }, }, } - assertApproved(t, r, "Approved by comment-approver, review-approver") + assertApproved(t, prctx, r, "Approved by comment-approver, review-approver") }) t.Run("contributorsCanApprove", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Options: Options{ AllowContributor: true, @@ -185,10 +206,11 @@ func TestIsApproved(t *testing.T) { }, }, } - assertApproved(t, r, "Approved by comment-approver, mhaypenny, contributor-author, contributor-committer, review-approver") + assertApproved(t, prctx, r, "Approved by comment-approver, mhaypenny, contributor-author, contributor-committer, review-approver") }) t.Run("specificUserApproves", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Requires: Requires{ Count: 1, @@ -197,7 +219,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertApproved(t, r, "Approved by comment-approver") + assertApproved(t, prctx, r, "Approved by comment-approver") r = &Rule{ Requires: Requires{ @@ -207,10 +229,11 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") }) t.Run("specificOrgApproves", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Requires: Requires{ Count: 1, @@ -219,7 +242,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertApproved(t, r, "Approved by comment-approver") + assertApproved(t, prctx, r, "Approved by comment-approver") r = &Rule{ Requires: Requires{ @@ -229,10 +252,11 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") }) t.Run("specificOrgsOrUserApproves", func(t *testing.T) { + prctx := basePullContext() r := &Rule{ Requires: Requires{ Count: 2, @@ -242,12 +266,14 @@ func TestIsApproved(t *testing.T) { }, }, } - assertApproved(t, r, "Approved by comment-approver, review-approver") + assertApproved(t, prctx, r, "Approved by comment-approver, review-approver") }) - t.Run("invalidateOnPush_comment", func(t *testing.T) { + t.Run("invalidateCommentOnPush", func(t *testing.T) { + prctx := basePullContext() prctx.CommitsValue = []*pull.Commit{ { + CreatedAt: now.Add(25 * time.Second), SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb", Author: "mhaypenny", Committer: "mhaypenny", @@ -261,23 +287,18 @@ func TestIsApproved(t *testing.T) { Users: []string{"comment-approver"}, }, }, - Options: Options{ - InvalidateOnPush: true, - }, } + assertApproved(t, prctx, r, "Approved by comment-approver") - // set the commit after the comment - prctx.CommitsValue[0].CreatedAt = now.Add(25 * time.Second) - assertPending(t, r, "0/1 approvals required. Ignored 4 approvals from disqualified users") - - // set the commit before the comment - prctx.CommitsValue[0].CreatedAt = now.Add(15 * time.Second) - assertApproved(t, r, "Approved by comment-approver") + r.Options.InvalidateOnPush = true + assertPending(t, prctx, r, "0/1 approvals required. Ignored 4 approvals from disqualified users") }) - t.Run("invalidateOnPush_review", func(t *testing.T) { + t.Run("invalidateReviewOnPush", func(t *testing.T) { + prctx := basePullContext() prctx.CommitsValue = []*pull.Commit{ { + CreatedAt: now.Add(85 * time.Second), SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb", Author: "mhaypenny", Committer: "mhaypenny", @@ -291,17 +312,72 @@ func TestIsApproved(t *testing.T) { Users: []string{"review-approver"}, }, }, + } + assertApproved(t, prctx, r, "Approved by review-approver") + + r.Options.InvalidateOnPush = true + assertPending(t, prctx, r, "0/1 approvals required") + }) + + t.Run("ignoreUpdateMergeAfterReview", func(t *testing.T) { + prctx := basePullContext() + prctx.CommitsValue = append(prctx.CommitsValue[:1], &pull.Commit{ + CreatedAt: now.Add(25 * time.Second), + SHA: "647c5078288f0ea9de27b5c280f25edaf2089045", + CommittedViaWeb: true, + Parents: []string{ + "c6ade256ecfc755d8bc877ef22cc9e01745d46bb", + "2e1b0bb6ab144bf7a1b7a1df9d3bdcb0fe85a206", + }, + Author: "merge-committer", + }) + + r := &Rule{ + Requires: Requires{ + Count: 1, + Actors: common.Actors{ + Users: []string{"comment-approver"}, + }, + }, Options: Options{ InvalidateOnPush: true, }, } + assertPending(t, prctx, r, "0/1 approvals required. Ignored 4 approvals from disqualified users") + + r.Options.IgnoreUpdateMerges = true + assertApproved(t, prctx, r, "Approved by comment-approver") + }) + + t.Run("ignoreUpdateMergeContributor", func(t *testing.T) { + prctx := basePullContext() + prctx.CommitsValue = append(prctx.CommitsValue[:1], &pull.Commit{ + CreatedAt: now.Add(25 * time.Second), + SHA: "647c5078288f0ea9de27b5c280f25edaf2089045", + CommittedViaWeb: true, + Parents: []string{ + "c6ade256ecfc755d8bc877ef22cc9e01745d46bb", + "2e1b0bb6ab144bf7a1b7a1df9d3bdcb0fe85a206", + }, + Author: "merge-committer", + }) + prctx.CommentsValue = append(prctx.CommentsValue, &pull.Comment{ + CreatedAt: now.Add(100 * time.Second), + Author: "merge-committer", + Body: ":+1:", + }) - // set the commit after the review - prctx.CommitsValue[0].CreatedAt = now.Add(85 * time.Second) - assertPending(t, r, "0/1 approvals required") + r := &Rule{ + Requires: Requires{ + Count: 1, + Actors: common.Actors{ + Users: []string{"merge-committer"}, + }, + }, + } + assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users") - // set the commit before the review - prctx.CommitsValue[0].CreatedAt = now.Add(75 * time.Second) - assertApproved(t, r, "Approved by review-approver") + r.Options.IgnoreUpdateMerges = true + assertApproved(t, prctx, r, "Approved by merge-committer") }) } diff --git a/pull/context.go b/pull/context.go index 4dd8a9bb..0f961243 100644 --- a/pull/context.go +++ b/pull/context.go @@ -62,6 +62,10 @@ type Context interface { // branches in forks are prefixed with the owner of the fork and a colon. // The base branch will always be unprefixed. Branches() (base string, head string, err error) + + // TargetCommits returns recent commits on the target branch of the pull + // request. The exact number of commits is an implementation detail. + TargetCommits() ([]*Commit, error) } type FileStatus int @@ -80,8 +84,10 @@ type File struct { } type Commit struct { - CreatedAt time.Time - SHA string + CreatedAt time.Time + SHA string + Parents []string + CommittedViaWeb bool // Author is the login name of the author. It is empty if the author is not // a real user. diff --git a/pull/github.go b/pull/github.go index 6ff88866..94c1da78 100644 --- a/pull/github.go +++ b/pull/github.go @@ -35,6 +35,11 @@ const ( // MaxPullRequestCommits is the max number of commits returned by GitHub // https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request MaxPullRequestCommits = 250 + + // TargetCommitLimit is the max number of commits to retrieve from the + // target branch of a pull request. 100 items is the maximum that can be + // retrieved from the GitHub API without paging. + TargetCommitLimit = 100 ) // GitHubContext is a Context implementation that gets information from GitHub. @@ -51,12 +56,13 @@ type GitHubContext struct { pr *github.PullRequest // cached fields - files []*File - commits []*Commit - comments []*Comment - reviews []*Review - teamIDs map[string]int64 - membership map[string]bool + files []*File + commits []*Commit + targetCommits []*Commit + comments []*Comment + reviews []*Review + teamIDs map[string]int64 + membership map[string]bool } func NewGitHubContext(ctx context.Context, mbrCtx MembershipContext, client *github.Client, v4client *githubv4.Client, pr *github.PullRequest) Context { @@ -174,6 +180,51 @@ func (ghc *GitHubContext) Branches() (base string, head string, err error) { return } +func (ghc *GitHubContext) TargetCommits() ([]*Commit, error) { + if ghc.targetCommits == nil { + var q struct { + Repository struct { + Ref struct { + Target struct { + Commit struct { + History struct { + Nodes []*v4Commit + } `graphql:"history(first: $limit)"` + } `graphql:"... on Commit"` + } + } `graphql:"ref(qualifiedName: $ref)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + qvars := map[string]interface{}{ + "owner": githubv4.String(ghc.owner), + "name": githubv4.String(ghc.repo), + "ref": githubv4.String(ghc.pr.GetBase().GetRef()), + "limit": githubv4.Int(TargetCommitLimit), + } + + if err := ghc.v4client.Query(ghc.ctx, &q, qvars); err != nil { + return nil, errors.Wrap(err, "failed to list target commits") + } + + commits := q.Repository.Ref.Target.Commit.History.Nodes + ghc.targetCommits = make([]*Commit, len(commits)) + + // GitHub only records pushedDate for the HEAD commit in a batch push + // Backfill this to all other commits for the purpose of sorting + var lastPushed *time.Time + for i := len(commits) - 1; i >= 0; i-- { + if commits[i].PushedDate != nil { + lastPushed = commits[i].PushedDate + } else { + commits[i].PushedDate = lastPushed + } + + ghc.targetCommits[i] = commits[i].ToCommit() + } + } + return ghc.targetCommits, nil +} + func (ghc *GitHubContext) loadTimeline() error { var q struct { Repository struct { @@ -183,7 +234,7 @@ func (ghc *GitHubContext) loadTimeline() error { EndCursor githubv4.String HasNextPage bool } - Nodes []*timelineEvent + Nodes []*v4TimelineEvent } `graphql:"timeline(first: 100, after: $cursor)"` } `graphql:"pullRequest(number: $number)"` } `graphql:"repository(owner: $owner, name: $name)"` @@ -195,7 +246,7 @@ func (ghc *GitHubContext) loadTimeline() error { "cursor": (*githubv4.String)(nil), } - var allEvents []*timelineEvent + var allEvents []*v4TimelineEvent for { if err := ghc.v4client.Query(ghc.ctx, &q, qvars); err != nil { return errors.Wrap(err, "failed to list pull request timeline") @@ -234,12 +285,7 @@ func (ghc *GitHubContext) loadTimeline() error { for _, event := range allEvents { switch event.Type { case "Commit": - ghc.commits = append(ghc.commits, &Commit{ - CreatedAt: event.CreatedAt(), - SHA: event.Commit.OID, - Author: event.Commit.Author.GetLogin(), - Committer: event.Commit.Committer.GetLogin(), - }) + ghc.commits = append(ghc.commits, event.Commit.ToCommit()) case "PullRequestReview": state := ReviewState(strings.ToLower(event.PullRequestReview.State)) ghc.reviews = append(ghc.reviews, &Review{ @@ -267,19 +313,14 @@ func (ghc *GitHubContext) loadTimeline() error { return nil } -type timelineEvent struct { +type v4TimelineEvent struct { Type string `graphql:"__typename"` - Commit struct { - OID string - PushedDate *time.Time - Author gitActor - Committer gitActor - } `graphql:"... on Commit"` + Commit v4Commit `graphql:"... on Commit"` PullRequestReview struct { ID string - Author actor + Author v4Actor State string Body string SubmittedAt time.Time @@ -293,13 +334,13 @@ type timelineEvent struct { } `graphql:"... on ReviewDismissedEvent"` IssueComment struct { - Author actor + Author v4Actor Body string CreatedAt time.Time } `graphql:"... on IssueComment"` } -func (event *timelineEvent) CreatedAt() (t time.Time) { +func (event *v4TimelineEvent) CreatedAt() (t time.Time) { switch event.Type { case "Commit": if event.Commit.PushedDate != nil { @@ -315,15 +356,49 @@ func (event *timelineEvent) CreatedAt() (t time.Time) { return } -type actor struct { +type v4Commit struct { + OID string + PushedDate *time.Time + Author v4GitActor + Committer v4GitActor + CommittedViaWeb bool + Parents struct { + Nodes []struct { + OID string + } + } `graphql:"parents(first: 10)"` +} + +func (c *v4Commit) ToCommit() *Commit { + var parents []string + for _, p := range c.Parents.Nodes { + parents = append(parents, p.OID) + } + + var createdAt time.Time + if c.PushedDate != nil { + createdAt = *c.PushedDate + } + + return &Commit{ + CreatedAt: createdAt, + SHA: c.OID, + Parents: parents, + CommittedViaWeb: c.CommittedViaWeb, + Author: c.Author.GetLogin(), + Committer: c.Committer.GetLogin(), + } +} + +type v4Actor struct { Login string } -type gitActor struct { - User *actor +type v4GitActor struct { + User *v4Actor } -func (ga gitActor) GetLogin() string { +func (ga v4GitActor) GetLogin() string { if ga.User != nil { return ga.User.Login } diff --git a/pull/pulltest/context.go b/pull/pulltest/context.go index 188c632a..949f1a5c 100644 --- a/pull/pulltest/context.go +++ b/pull/pulltest/context.go @@ -45,6 +45,9 @@ type Context struct { BranchBaseName string BranchHeadName string BranchesError error + + TargetCommitsValue []*pull.Commit + TargetCommitsError error } func (c *Context) Locator() string { @@ -104,5 +107,9 @@ func (c *Context) Branches() (base string, head string, err error) { return c.BranchBaseName, c.BranchHeadName, c.BranchesError } +func (c *Context) TargetCommits() ([]*pull.Commit, error) { + return c.TargetCommitsValue, c.TargetCommitsError +} + // assert that the test object implements the full interface var _ pull.Context = &Context{}