Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Less constrained merge method access #1036

Open
xmo-odoo opened this issue Jan 14, 2025 · 0 comments
Open

Less constrained merge method access #1036

xmo-odoo opened this issue Jan 14, 2025 · 0 comments
Labels
mergebot

Comments

@xmo-odoo
Copy link
Collaborator

Apparently some teams have decided nobody should have self r+, which then becomes an issue with merge methods as those are tied to r+ rights, and reviewers setting merge methods is ???

  • missing merge method notifications is delayed (part to the linked PR status check which is currently hourly)
  • merge method requirement is unreliable due to github's inconsistent behaviour around some events (and issue_comment doesn't provide it as far as I can tell)
  • an author can not set the merge method on their PRs if they don't have self-r+ rights

Possibilities:

  • put the merge method check on a shorter cron and batch query github to validate that the PR is not squash
  • add a new ACL / capability (based on what?) to allow "trusted" authors to set the merge method, maybe other commands would be moved to that (up from author or down from reviewer)
  • just allow authors to set the merge method, this does allow some amount of unreviewed changes (set PR message and set merge method to merge or squash) but there is no deeply functional impact...
@xmo-odoo xmo-odoo moved this to accepted in Mergebot Jan 14, 2025
@xmo-odoo xmo-odoo moved this from accepted to done in Mergebot Jan 30, 2025
xmo-odoo added a commit that referenced this issue Jan 30, 2025
@xmo-odoo
[IMP] runbot_merge: check squash on every PR event
1099fc5 
All `pull_request` events seem to provide the `commits` count
property. As such we can use them all to check the `squash` state even
if we don't otherwise care for the event.

Also split out notifying an approved pull request about its missing
merge method into a separate cron from the one notifying a PR that its
siblings are ready.

Fixes #1036
xmo-odoo added a commit that referenced this issue Jan 30, 2025
@xmo-odoo
[IMP] runbot_merge: relax requirements for setting the merge method
d7da328 
Not entirely sure about just allowing any PR author to set the merge
method as it gives them a lot more control over commits (via the PR
message), and I'm uncertain about the ramifications of doing that.

However if the author of the PR is classified as an
employee (via a provisioned user linked to the github account) then
allow it. Should not be prone to abuse at least.

Fixes #1036
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
mergebot
Projects
Mergebot
Status: done
Milestone
No milestone
Development

No branches or pull requests
1 participant
@xmo-odoo