Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If two workflows are triggered on a branch and one is successful and one is skipped, mp doesn't allow merge #107

Open
brettjenkins opened this issue Aug 12, 2021 · 2 comments · May be fixed by #160
Labels
bug GitHub :octocat: Github-specific issue help wanted

Comments

@brettjenkins
Copy link

Hey,

Just trying MP, and noticed a bug, if two workflows are configured on a branch, and one is successful and one is skipped (because it doesn't apply in this case), when trying to merge you get this error:

merge error: Build status was not 'success', instead was 'pending'. Use --ignore-build-status to override this check.

Presumably it's treating the skipped check as pending and so I have to ignore the build status to continue the merge.

Thanks

@nathanleiby
Copy link
Contributor

@brettjenkins thanks for the reporting! Agreed, seems like unexpected UX.

Did this happen for a build in a private or public repo? If public please link! Else can try to reproduce.


For reference, the relevant code is here:

// (2) Check commit status
<-repoLimiter.C
status, _, err := client.Repositories.GetCombinedStatus(ctx, input.Repo.Owner, input.Repo.Name, input.CommitSHA, &github.ListOptions{})
if err != nil {
return Output{Success: false}, err
}
if input.RequireBuildSuccess {
state := status.GetState()
if state != "success" {
return Output{Success: false}, fmt.Errorf("Build status was not 'success', instead was '%s'. Use --ignore-build-status to override this check.", state)
}
}

We are calling GetCombinedStatus. Docs:

The most recent status for each context is returned, up to 100. This field paginates if there are over 100 contexts.
Additionally, a combined state is returned. The state is one of:

  • failure if any of the contexts report as error or failure
  • pending if there are no statuses or a context is pending
  • success if the latest status for all contexts is success

We may need to dig into the individual statuses to handle this, rather than just accept the combined status.

@nathanleiby nathanleiby added help wanted GitHub :octocat: Github-specific issue labels Aug 12, 2021
@robherley
Copy link

robherley commented Jun 7, 2022

👋 @nathanleiby The /repos/{owner}/{repo}/commits/{ref}/status route used by GetCombinedStatus unfortunately only accounts for states from the Commit Status API.

If you want to account for all of the states from the Commit Status API, Checks API and Actions, you'll need to use the GraphQL to get the StatusCheckRollup.

For example, a PR from this repository: #159

That PR has both Checks (from the dependabot Actions) and a Commit Status (from Circle CI).

If you query with just the REST API for Statuses, you'll see it only has a single status:

Status API REST:

$ curl -H "Accept: application/vnd.github.v3+json" \
  https://api.github.com/repos/Clever/microplane/commits/dd872cc3820a726e242f9f20680f2e35aad9950b/status
📬 Status API REST Result
{
  "state": "success",
  "statuses": [
    {
      "url": "https://api.github.com/repos/Clever/microplane/statuses/dd872cc3820a726e242f9f20680f2e35aad9950b",
      "avatar_url": "https://avatars.githubusercontent.com/oa/4808?v=4",
      "id": 17692110259,
      "node_id": "SC_kwDOBmm9eM8AAAAEHogtsw",
      "state": "success",
      "description": "Your tests passed on CircleCI!",
      "target_url": "https://circleci.com/gh/Clever/microplane/374?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link",
      "context": "ci/circleci: build",
      "created_at": "2022-06-06T17:16:42Z",
      "updated_at": "2022-06-06T17:16:42Z"
    }
  ],
  "sha": "dd872cc3820a726e242f9f20680f2e35aad9950b",
  "total_count": 1,
  // ...
}

⚠️ Note the total_count of 1, it's only looking at the single Status.

Instead, you should be using the StatusCheckRollup present in the GraphQL API:

query {
  repository(owner: "Clever", name: "microplane") {
    object(oid: "dd872cc3820a726e242f9f20680f2e35aad9950b") {
      ... on Commit {
        statusCheckRollup {
          contexts {
            totalCount
          }
          state
        }
      }
    }
  }
}
📬 StatusCheckRollup GraphQL Result
{
  "data": {
    "repository": {
      "object": {
        "statusCheckRollup": {
          "contexts": {
            "totalCount": 2
          },
          "state": "SUCCESS"
        }
      }
    }
  }
}

This'll factor in all the runs (checks + actions) plus the commit statuses. I apologize for the confusing nature of these endpoints 😅

I'd also suggest you look into handling cases for branch protection rules as well: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

/cc @benwaffle

benwaffle pushed a commit to benwaffle/microplane that referenced this issue Jun 7, 2022
Fetch everything that makes a PR green, including the Status API and the Checks API.

Fixes Clever#107
@benwaffle benwaffle linked a pull request Jun 7, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug GitHub :octocat: Github-specific issue help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants