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