chdsbd / kodiak

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

Kodiak shows skipped status check as failure and doesn't update/merge the PR #885

Closed erezrokah closed 7 months ago

erezrokah commented 7 months ago

Example PR in https://github.com/cloudquery/cloudquery/pull/17769

Validate PR title is shown as skipped but Kodiak thinks it failed: image image

This is the workflow https://github.com/cloudquery/cloudquery/blob/222aa899288cdea653d485a155b82aabd0c8f4e0/.github/workflows/validate_pr_title.yml#L15

Please notice we have a condition on the job so a skipped job should be counted as success. Kodiak config https://github.com/cloudquery/cloudquery/blob/222aa899288cdea653d485a155b82aabd0c8f4e0/.github/.kodiak.toml


version = 1

[approve]
auto_approve_usernames = ["cq-bot"]

[merge.message]
body = "pull_request_body"
strip_html_comments = true
title = "pull_request_title"

[merge.automerge_dependencies]
usernames = ["cq-bot"]
versions = ["patch"]

[merge]
blocking_labels = ["wip", "no automerge"]
notify_on_conflict = false
priority_merge_label = "priority merge"
erezrokah commented 7 months ago

This seems like a regression in GitHub maybe? It was working for us a week ago

mabadir commented 7 months ago

We are facing the same issue

chdsbd commented 7 months ago

Sorry for the delayed response. I was away.

This is weird. Kodiak is using the requirements from the GitHub API to know that Check is required. I will take a look at this later tonight.

Definitely possible that GitHub changed the API behavior

mabadir commented 7 months ago

@chdsbd just sharing what I am experiencing in case it helps. Kodiak does not auto-update when the Check is skipped, and hence does not auto-merge. If I manually updated the branch, it automatically merges.

chdsbd commented 7 months ago

@mabadir That's helpful! Sounds like a web hook isn't being sent for a skipped check run

chdsbd commented 7 months ago

It seems like we consider "skipped" as a reason for failure. Maybe this has always been wrong or it's been changed at some point, but I think we need to change this so "SKIPPED" counts as success.

https://github.com/chdsbd/kodiak/blob/4e8e18007795b916437cf41372e1f51120914423/bot/kodiak/evaluation.py#L949-L957

mabadir commented 7 months ago

@chdsbd thanks for looking into it. I believe this has been the case for quite some time. I am not a Python developer myself, but happy to open a PR if it is as easy as removing line 954 😅

chdsbd commented 7 months ago

I'll make the change and test it. Hopefully I can have this deployed in the next couple days.

mabadir commented 7 months ago

Thank you!

erezrokah commented 7 months ago

Not sure how it's reported in the web book but not all skipped checks should be considered as successful https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks For example if a required check was skipped due to path filtering it blocks merging

chdsbd commented 7 months ago

I've deployed #890 which should fix the issue

erezrokah commented 7 months ago

I can confirm this solved it for us thanks @chdsbd 🚀