v1.6.1 (#290) broke auto-merging (--pr-merge)
#293
Comments
|
Fingers crossed that this one helps instead, looking forward to it: 🤞 |
MPV
changed the title
v1.6.1 (#290) broke auto-mergingv1.6.1 (#290) broke auto-merging (--pr-merge)
|
thanks for the report. If you can provide some logs it will help us find the root cause. |
Logs using
|
Logs using
|
|
Do you have any other branch protection rules enabled? For example, |
Yes but it's a bit of a mix here with different repos having different rules (sorry, can try to look up specific examples), but one repo I know stopped working in v1.6.1 had:
...and then a bunch of repos with different kinds of repo-level branch protection, such as:
|
|
So, that might be why it's stuck waiting in I suppose my patch wasn't backwards compatible because it also checks for other branch protection rules implicitly, in addition to the status checks. But this does feel like it's the correct thing to do. IMO, if you have branch protection rules, they should be followed by octopilot. But I can also see the need for side-stepping them when you're batch updating hundreds of repos. But then the question arises what do you want to side-step? Everything or the just the approvals or commit signature checks, etc. When it's done through the UI, it's relatively easy to make that decision, but it's tricky doing it via the API because that information is not readily available. I don't know what the best solution is, but to maintain backwards compatibility maybe we add the old "status API check method" back in, make it the default, and then this new behavior is exposed with yet another flag. That would mean then there are three different ways of doing a PR merge ( |
Yes they do (but just for admins, which my app for Octopilot is): |
"Merging is blocked" is what I'm referring to. The But it sounds like you want an option to be able to bypass that. Would adding a However, that's still not the same behavior as pre |
|
I agree with you @jashandeep-sohi - by default octopilot should play by the rules. |
|
I was digging into the API a bit more to see if there's some other way of doing this, and it turns out "branch-protection rules" and "Rulesets" are completely independent from each other. So even pre Rulesets are a recent addition to the API, so the version of The GraphQL API does expose a |
|
ok thanks. moving to the latest version of the github client sounds like a good idea, so if it works we can do that |
jashandeep-sohi
added a commit
to jashandeep-sohi/octopilot
that referenced
this issue
Dec 19, 2023
This flag lets you pick how to handle branch protection rules. `statusChecks` waits until all **required** statues & check rules (from Statues & Checks API, respectively) are in a "success" state. For Checks this also means any rule that has a "neutral" or "skipped" conclusion. It looks at both required statues configured under branch protection rules **and** rulesets. This is the flag default to maintain backwards compatibility pre-`v1.6.1`. `all` waits until **all** branch protection rules are met. This also includes any matching Rules defined via the Rulesets API. `bypass` lets you bypass all branch protection rules once the authenticated user has the ability. That is, the user/app is configured to bypass rules. Also bumped Github Client to v57 to be able to use the Ruleset API. This should fix issue dailymotion-oss#293.
jashandeep-sohi
added a commit
to jashandeep-sohi/octopilot
that referenced
this issue
Dec 19, 2023
This flag lets you pick how to handle branch protection rules. `statusChecks` waits until all **required** statues & check rules (from Statues & Checks API, respectively) are in a "success" state. For Checks this also means any rule that has a "neutral" or "skipped" conclusion. It looks at both required statues configured under branch protection rules **and** rulesets. This is the flag default to maintain backwards compatibility pre-`v1.6.1`. `all` waits until **all** branch protection rules are met. This also includes any matching Rules defined via the Rulesets API. `bypass` lets you bypass all branch protection rules once the authenticated user has the ability. That is, the user/app is configured to bypass rules. Also bumped Github Client to v57 to be able to use the Ruleset API. This should fix issue dailymotion-oss#293.
vbehar
pushed a commit
that referenced
this issue
Dec 21, 2023
* feat: add flag --pr-merge-branch-protection This flag lets you pick how to handle branch protection rules. `statusChecks` waits until all **required** statues & check rules (from Statues & Checks API, respectively) are in a "success" state. For Checks this also means any rule that has a "neutral" or "skipped" conclusion. It looks at both required statues configured under branch protection rules **and** rulesets. This is the flag default to maintain backwards compatibility pre-`v1.6.1`. `all` waits until **all** branch protection rules are met. This also includes any matching Rules defined via the Rulesets API. `bypass` lets you bypass all branch protection rules once the authenticated user has the ability. That is, the user/app is configured to bypass rules. Also bumped Github Client to v57 to be able to use the Ruleset API. This should fix issue #293. * lint * formatting * lint * lint
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
I had automerge working before:
...but after upgrading, it doesn't work anymore for me.
cc @jashandeep-sohi (author of #290) for info.
I'll see if I can get back with some more details. It's hard without sharing too much internal details, as you might envision.
The text was updated successfully, but these errors were encountered: