Skip to content

Commit

Permalink
githubclt: check mergeable_state field when evaluating if a PR is upt…
Browse files Browse the repository at this point in the history
…odate

The githubclt.PRIsUptodate() can return the wrong result
(#28).

It evaluates if a PR is uptodate with the base branch via information from the
github REST pulls API. It compares the base.sha field of the PR message with the
HEAD SHA that is retrieved for the base branch.

The base.sha returned from the pulls API is the SHA of the base-branch from when
the PR was created.
It is not related to the commits in the PR branch, it is not the most recent
ancestor of both branches.

That means if e.g. a PR is created with a branch based on commit main-HEAD^, the
base.sha in the pulls message will be main-HEAD instead.
The Check will return that the PR is uptodate despite it is not.

To improve the behavior PRIsUptodate() now also checks if the mergeable_state
has the value "behind", which means that the branch is not update.

This does not fix the issue, it only makes it a bit more unlikely to occur.
mergeable_state can have another value like UNSTABLE and the PR might be
outofdate.
  • Loading branch information
fho committed Aug 17, 2021
1 parent 6e3221a commit 2d9ff1a
Showing 1 changed file with 11 additions and 6 deletions.
17 changes: 11 additions & 6 deletions internal/githubclt/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ func (clt *Client) CombinedStatus(ctx context.Context, owner, repo, ref string)
// Additionally it returns the SHA of the head commit for which the status was
// checked.
// If the PR is closed PullRequestIsClosedError is returned.
// Known Bugs: https://github.com/simplesurance/goordinator/issues/28
func (clt *Client) PRIsUptodate(ctx context.Context, owner, repo string, pullRequestNumber int) (isUptodate bool, headSHA string, err error) {
pr, _, err := clt.clt.PullRequests.Get(ctx, owner, repo, pullRequestNumber)
if err != nil {
Expand All @@ -120,6 +121,15 @@ func (clt *Client) PRIsUptodate(ctx context.Context, owner, repo string, pullReq
return false, "", ErrPullRequestIsClosed
}

prHead := pr.GetHead()
if prHead == nil {
return false, "", errors.New("got pull request object with empty head")
}

if pr.GetMergeableState() == "behind" {
return false, prHead.GetSHA(), nil
}

base := pr.GetBase()
if base == nil {
return false, "", errors.New("got pull request object with empty base")
Expand All @@ -133,12 +143,7 @@ func (clt *Client) PRIsUptodate(ctx context.Context, owner, repo string, pullReq
return false, "", fmt.Errorf("could not retrieve head SHA of base branch %q: %w", baseBranch, err)
}

head := pr.GetHead()
if head == nil {
return false, "", errors.New("got pull request object with empty head")
}

return PRbaseSHA == baseBranchHEADSHA, head.GetSHA(), nil
return PRbaseSHA == baseBranchHEADSHA, prHead.GetSHA(), nil
}

// CreateIssueComment creates a comment in a issue or pull request
Expand Down

0 comments on commit 2d9ff1a

Please sign in to comment.