considers skipped as success

[alita-moore]

if the check is skipped instead of failed; see alita-moore/EIPs#388

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]

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

We need to update the evaluation logic:

kodiak/bot/kodiak/evaluation.py

Lines 868 to 914 in 008d218

	required = set(branch_protection.requiredStatusCheckContexts)
	for status_context in contexts:
	# handle dont_wait_on_status_checks. We want to consider a
	# status_check failed if it is incomplete and in the
	# configuration.
	if (
	status_context.context in config.merge.dont_wait_on_status_checks
	and status_context.state
	in (StatusState.EXPECTED, StatusState.PENDING)
	):
	skippable_contexts.append(status_context.context)
	continue
	if status_context.state in (StatusState.ERROR, StatusState.FAILURE):
	failing_contexts.append(status_context.context)
	elif status_context.state in (
	StatusState.EXPECTED,
	StatusState.PENDING,
	):
	pending_contexts.append(status_context.context)
	else:
	assert status_context.state == StatusState.SUCCESS
	passing_contexts.append(status_context.context)
	for check_run in deduplicate_check_runs(check_runs):
	if (
	check_run.name in config.merge.dont_wait_on_status_checks
	and check_run.conclusion in (None, CheckConclusionState.NEUTRAL)
	):
	skippable_contexts.append(check_run.name)
	continue
	if check_run.conclusion is None:
	continue
	if check_run.conclusion == CheckConclusionState.SUCCESS:
	passing_contexts.append(check_run.name)
	if check_run.conclusion in (
	CheckConclusionState.ACTION_REQUIRED,
	CheckConclusionState.FAILURE,
	CheckConclusionState.TIMED_OUT,
	CheckConclusionState.CANCELLED,
	CheckConclusionState.SKIPPED,
	CheckConclusionState.STALE,
	):
	failing_contexts.append(check_run.name)
	passing = set(passing_contexts)
	failing = set(failing_contexts)
	# we have failing statuses that are required
	failing_required_status_checks = failing & required

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

[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.

[alita-moore]

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]

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]

@alita-moore Maybe this will help for your use case? #719 (comment)

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]

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

[chdsbd]

@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.

[alita-moore]

Thank you 🙏🙇‍♀️