Skip to content
New issue

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

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

Already on GitHub? # to your account

feature request: add validator for other Github checks in a PR #395

Open
abid-mujtaba opened this issue Oct 6, 2020 · 6 comments · Fixed by #418
Open

feature request: add validator for other Github checks in a PR #395

abid-mujtaba opened this issue Oct 6, 2020 · 6 comments · Fixed by #418

Comments

@abid-mujtaba
Copy link
Contributor

My PRs often have long-running CI and linter checks. These don't work with the merge action in this bot since the bot has no way to validate and block based on other Github checks that are pending or have failed. This causes the bot to attempt to merge the PR and fail.

It would be really useful to have a validator that ensures that other Github checks have all passed which we can then use to automatically merge.

@sambhav
Copy link
Contributor

sambhav commented Oct 8, 2020

AFAICT this doesn't really need a validator, but simply for the merge action to support the status and check_suite events. Given that #386 is now fixed - all we need to do is make all of the above required checks via branch protection and the above ability combined with the ability to have empty validators as per #402 we should be able to achieve this.

Then a recipe like

version: 2
mergeable:
  - when: pull_request.*, status.*, check_suite.*
    name: 'All checks pass'
    validate: []
    pass:
      - do: merge
        merge_method: "squash"

is all we would need.

@abid-mujtaba
Copy link
Contributor Author

Agreed. This covers the use case I described. Thanks for the relevant changes.

@abid-mujtaba
Copy link
Contributor Author

This feature has been working really well where the base/target is the default (main/master) branch which is protected. However it is causing PRs between feature branches (where the base/target is not protected) to auto-merge even before the check suite has finished.

Since I am primarily interested in auto-merging on the default (protected) branch it should be possible to address this by extending the baseRef validator to support check_suite.* (and perhaps even status.*) events.

@abid-mujtaba
Copy link
Contributor Author

For PRs that require long-running CI builds to pass we need to support status.* events in the baseRef validator because the last event that comes in is a success status from the CI.

@munkherdeneen
Copy link

This feature has been working really well where the base/target is the default (main/master) branch which is protected. However it is causing PRs between feature branches (where the base/target is not protected) to auto-merge even before the check suite has finished.

Since I am primarily interested in auto-merging on the default (protected) branch it should be possible to address this by extending the baseRef validator to support check_suite.* (and perhaps even status.*) events.

Hi @abid-mujtaba, Could you share your mergeable.yml and branch protection rule here? Because I have very similar environment as you using GitHub Enterprise and merge bot is on-prem. But currently I am facing with following issue.

  1. The PR opens.
  2. Get all required approvals while the long-standing pipeline runs.
  3. While the pipeline runs we have a change on the main branch and it makes conflicts with the PR branch.
  4. Then the pipeline finishes and is ready to merge. I mean all set.
  5. But there is a conflict and it blocks our merge.
  6. Then fix the conflict between the main and PR branch with git push.
  7. Then it merges the PR immediately without checking other conditions. After the fix commit it dismisses all the approvals
    with the branch protection rule.

image

Have you had this issue with protected branch? Can you share you mergeable.yml and branch protection rule?

@abid-mujtaba
Copy link
Contributor Author

@munkherdeneen, I have semi-recently updated my config to:

 # Auto-merge PR when all required (in branch protection) checks have passed
  - when: pull_request_review.submitted, check_suite.completed
    name: 'All checks pass'
    validate:
      - do: baseRef
        must_include:
          regex: 'main'
          message: 'Auto-merge is only supported for the default branch'
        mediaType:
          previews:
            - groot
    pass:
      - do: merge
        merge_method: squash

I realized that the issue with baseRef (and a number of other validators) is that they do not support status.* events (which have a non-standard payload compared to nearly every other type of event). I fixed the problem on my team's workflow by switching to a Github App based Jenkins workflow which allows Jenkins to report updates via the newer Checks API (rather than the Status API). With that change I can now depend on a much smaller set of trigger events (pull_request_review.submitted, check_suite.completed) and merge in a reliable manner.

Feel free to ask for clarification. Cheers.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
3 participants