chdsbd / kodiak

🔮 A bot to automatically update and merge GitHub PRs
https://kodiakhq.com
GNU Affero General Public License v3.0
1.03k stars 65 forks source link

considers skipped as success #780

Open alita-moore opened 2 years ago

alita-moore commented 2 years ago

if the check is skipped instead of failed; see https://github.com/alita-moore/EIPs/pull/388

Screen Shot 2022-01-17 at 8 55 26 PM

In this case the "greetings" was the only required check. But the bot kodiak still merged. It would be nice to have an option to consider skips as fails, just to be safe.

sbdchd commented 2 years ago

Yeah seems like it could fit behind a config flag, something like, require_skipped_checks = true

We need to update the evaluation logic:

https://github.com/chdsbd/kodiak/blob/008d21849650667fda2f23401d988896d264f6e9/bot/kodiak/evaluation.py#L868-L914

Currently we check the failing checks and compare them against the branch protection's required checks.

chdsbd commented 2 years ago

I think this current behavior is correct, but we could add this as an enhancement.

GitHub considers "skipped" checks as passing. If you want to block merging if a check has been skipped, I think you could use a GitHub Action to fail the build.

alita-moore commented 2 years ago

I think this current behavior is correct, but we could add this as an enhancement.

GitHub considers "skipped" checks as passing. If you want to block merging if a check has been skipped, I think you could use a GitHub Action to fail the build.

do you mean to use a separate github action to fail the builds that skip so that kodiak would not merge skipped checks? Do you have any in mind?

alita-moore commented 2 years ago

In any case, you could simply fail the skipped check instead of skipping it if a criteria is not met, but then you will be showing the user that the check failed when in reality it was skipped. Which (imo) is confusing.

chdsbd commented 2 years ago

@alita-moore Maybe this will help for your use case? https://github.com/chdsbd/kodiak/issues/719#issuecomment-912681682

I think the https://github.com/technote-space/workflow-conclusion-action action can be used to fail the build if the conclusion of some jobs is failure

alita-moore commented 2 years ago

@chdsbd we can't use kodiak until this feature is implemented because it allows for circumventing required checks when you go from draft -> PR: https://github.com/ethereum/EIPs/pull/4749

chdsbd commented 2 years ago

@alita-moore I've deployed #785 to production, so you can enable the merge.block_on_neutral_required_check_runs setting (currently undocumented) to stop Kodiak from merging a PR if a required check run has a neutral status.

Screen Shot 2022-02-03 at 9 17 07 PM
alita-moore commented 2 years ago

Thank you 🙏🙇‍♀️