kodiak icon indicating copy to clipboard operation
kodiak copied to clipboard

considers skipped as success

Open alita-moore opened this issue 3 years ago • 8 comments

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.

alita-moore avatar Jan 18 '22 03:01 alita-moore

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.

sbdchd avatar Jan 18 '22 03:01 sbdchd

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.

chdsbd avatar Jan 18 '22 03:01 chdsbd

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 avatar Jan 18 '22 20:01 alita-moore

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.

alita-moore avatar Jan 18 '22 20:01 alita-moore

@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

chdsbd avatar Jan 18 '22 22:01 chdsbd

@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

alita-moore avatar Feb 03 '22 16:02 alita-moore

@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

chdsbd avatar Feb 04 '22 02:02 chdsbd

Thank you 🙏🙇‍♀️

alita-moore avatar Feb 04 '22 09:02 alita-moore