Renovate: Implement requiredStatusChecks

Created on 20 Apr 2018  Â·  21Comments  Â·  Source: renovatebot/renovate

This is a:

  • [ ] Bug report (non-security related)
  • [x] Feature request
  • [ ] I'm not sure which of those it is

The requiredStatusChecks option is not fully implemented - it only works if you set it to null (i.e. NO required status checks).

We should implement the feature fully so that users can specify a list of status checks that are required for automerge to be "green" and we ignore the rest.

github priority-4-low feature

Most helpful comment

I’ve added extra logic to the branch automerge try/catch so that it shouldn’t force a PR if the error is just because some requires status checks aren’t met. But beware you shouldn’t set checks that are incompatible with a branch automerge, such as required reviews or checks that only fire when there’s a PR.

All 21 comments

Is it possible to see if the base branch has required status checks and go off of those? Our configuration looks like the below, but it always opens a PR since the required status checks we have on master prevent branch automerge.

  "devDependencies": {
    "automerge": true,
    "automergeType": "branch",
    "major": {
      "automerge": false
    }
  }
}

@stevenzeck can you describe more about what branch protection configuration you have? e.g. does your repo have 5 status checks but only 3 of them are marked as required?

For one of the repos I count 12, where 4 are required. The Renovate dashboard error says 2 of 4 required status checks are expected.

@rarkins do you know of any ways for Renovate to work with GitHub required status checks while continuing to have automergeType of branch?

I think it’s possible to look up via the commit hash like we did with status checks.

Can you share how to configure our renovate files to do that? Right now we're disabling required status checks against master so that Renovate doesn't run into errors when merging from branches.

Sorry, I misunderstood your previous question and thought it was about the new check runs.

Right now Renovate just waits for all test to be passed (then it merges the branch), or for any to be failed (it then raises a PR instead), or for for a timeout (default: 25 hours) in which case it also raises a PR.

I guess what you'd prefer is that Renovate can read which status checks you have configured in branch protection and wait only on those before deciding to merge or raise PR?

The alternative if that is too hard is to make you configure the same status checks in the Renovate config's requiredStatusChecks field so we don't need to look them up, although then you would be making "duplicate" settings in your repo, which is never good.

@rarkins thanks for the information. Ultimately when Renovate is configured with automergeType: branch, I want it to wait until the required status checks are completed or until at least one required one fails before attempting to merge or creating a PR. It looks like https://github.com/renovatebot/renovate/blob/8f8cffbd6f26783e9decdc8a5b927ab7010135d4/lib/platform/github/index.js#L498 doesn't return pending statuses perhaps?

From memory, that query returns all statuses including pending. So Renovate will wait until they are all "passed", or one is "failed", or for the 25 hour pending timeout.

@rarkins I looked into this a bit more and not all checks are present in the combined status response, which is what renovate looks at. For this repo, here are the PR "checks" and which API they show in:

  • codeclimate/total-coverage (shows in status API only after it's not in the dark brown status)
  • codeclimate/diff-coverage (shows in status API only after it's not in the dark brown status)
  • Travis CI - Branch (shows in check-runs API)
  • Travis CI - Pull Request (shows in check-runs API)
  • Unibeautify - Branch (shows in check-runs API)
  • Unibeautify - Pull Request (shows in check-runs API)
  • WIP (shows in status API)
  • codeclimate (shows in status API)
  • continuous-integration/unibeautify/pr (shows in status API)
  • continuous-integration/unibeautify/push (shows in status API)

I'm not sure where codeclimate/total-coverage and codeclimate/diff-coverage are. I know Travis CI recently stopped using statuses in favor of checks. But this is definitely part of our problem with automerging.

Edit: #2659 looks to have implemented check-runs into renovate

Here's a good example. The first run of renovate found that @types/jest had an update, created the branch, and checked the status of the branch and it to be pending.

The second run only returned one status when it should have returned six. If you curl it now curl -H "Accept: application/vnd.github.antiope-preview+json" 'https://api.github.com/repos/unibeautify/beautifier-prettier/commits/renovate/jest-23.x/status' you'll see all six.

Edit: @rarkins I think the checks that start with a dark brown circle (like codeclimate/coverage) are not in the status response until they have been reported. When I open a sample PR and run the code immediately after I open it, it does not include the two codeclimate/coverage statuses. When I wait until the statuses are not in the dark brown color and run, they are returned.

Thanks, that is very helpful. I think we can look for that message like “2 of 4 required status checks are expected” and if so then not raise a PR yet because it’s hopefully a temporary failure and not a permanent one

Might want to consider retrieving the required status checks under the branch protection rules for the repo and branch, then compare that with the status and checks API response and see if they are included in that yet.

We're also experiencing that setting e.g. 3 out of 6 checks to required makes Renovate "give up" trying to automerge, so to speak, even before the status checks have finished running.

However, today it looks like it actually came back later and merged it. That's a first 🙂

screen shot 2018-10-30 at 07 31 02

I’ve added extra logic to the branch automerge try/catch so that it shouldn’t force a PR if the error is just because some requires status checks aren’t met. But beware you shouldn’t set checks that are incompatible with a branch automerge, such as required reviews or checks that only fire when there’s a PR.

@rarkins on second look, it seems we disabled our status checks which is why it started working consistently 11 days ago. However for a couple of new repos where I re-implemented required status checks, it still fails. Here is a gist from the logs: https://gist.github.com/stevenzeck/f1185da90a8f9c0a49f5e6ae77926f87.

This catch block, is the err.message always "Branch automerged failed", as seen here? Whereas the error saying "required status checks are expected" is thrown here?

@stevenzeck thank you for the detailed logs. You were right - we were rewriting the err object and losing the context. Renovate should now detect automerge failures due to status checks pending and not raise PRs so eagerly. Leaving this issue open though because it's not technically the same thing.

If the current status is that the only option is to ignore all checks it should also be documented more clearly. Te current documentation is a bit hard to understand from my perspective.

If anything else is possible, it's also not well documented what the strings needs to contain to match certain checks.
E.g. on github they are displayed differently debending on the page, there is a workflow name (from yaml or file name), that contains one or more jobs, that can be run against a matrix, and in a PR even the event that triggered the check is part of the "label":
image

@karfau confirmed: only null is supported today. A docs-only PR would be welcome - it's in this repo

BTW I have a suspicion that some people want this to mean "only require certain status checks and ignore if others are incomplete or failing" while others want this to mean "don't merge until at least these status checks are met, and of course only if all others are passing too"

@karfau confirmed: only null is supported today. A docs-only PR would be welcome - it's in this repo

7926

Was this page helpful?
0 / 5 - 0 ratings