Automatic enforcement of the 5 day rule #147
Comments
chrisgorgo
added
enhancement
|
Killing two birds with one stone -- I think a bot will clear up any ambiguity about when that 5-day clock starts ! 👍 |
|
I wonder if we could write a rule for Policy Bot. |
|
Investigating if we can do this with mergify.io Mergifyio/mergify#349 |
|
Based on discussion in #150, it sounds like we should write rules for disapproval that will only be lifted after 5 days from oldest commit. That way the merging reviewer will still be able to ensure that the PR title is correct for the changelog. Unless anyone else has another suggestion for how to approach this issue ! |
+1, this would be more robust instead of automatically merging after 5 days if 2 approving reviews, |
|
Hello! I'm just passing through while exploring OSS x Neuroscience, but I did a bit of research last night and found Mergeable, a Probot plugin. I'm not sure if this will serve your needs completely, since it doesn't seem compatible with PushEvents (which, I'm pretty sure, is the particular event you need to be watching for/timing) but perhaps it could be modified slightly to handle those! If folks are interested, I can do some more exploring this weekend. |
|
Thank you @gallexi for raising Mergeable! I was just preparing a comment to propose that too! I found the pro's of Mergeable is the days old feature and that it can detect the 2 approvals. The one thing it is missing (as you mentioned) is the merge aspect. This would more than likely have to be used with another tool (like mergify). A potential workflow could be: Policy bot would provide the disapproval until a cue (like a comment) is given by Mergeable to change that to an approval and then the other tool (like mergify) can merge it. A concern is over enginnering is. What do we think about this or other suggestions? This is what I saw as a potential solution and gallexi if you would like to explore further, that sounds good, thank you! Side note: The tool we have implemented for generating the changelog (github changelog generator) allows for PR titles to be changed after a merge and it will be represented correctly in the changelog on the next merged PR (this is the action that triggers a changelog build). |
|
I've looked more into Mergeable, as well as probot-scheduled-merge, and ended up coming upon semantic pull requests, which, despite not having anything to do with time on its own, I think could actually be pretty easily modified to fit the goal here! I'll see if I can get a proof of concept up, but the general idea is that it would return a "pending" checks status until the latest commit (and, I assume we also want the PR itself) is older than 5 days. Then, it will return "success"! Does that sound reasonable? |
|
Thank you @gallexi for looking into this and I am also excited to see the proof concept! |
|
GitHub now has "branch protection rules" where one can define CI jobs that must pass before a PR can be merged. (Note that admin overwrites are always possible). Implementing the 5-day rule could work by writing a GitHub workflow that for a given PR checks the date of the most recent commit on that PR and compares it to the current date, passing only if the delta is >= 5 days. Open points:
Perhaps there are better ideas, but I wanted to drop this here. |
|
Probably only worth running for the PR that are not excluced from the change log. Probably doable with the right sequence of "if". |
|
Was playing around and at least this a script that checks all PRs and can sort those that match those criterias:
I am not so sure we need this automation but if anyone wants to push that ball a little bit further. from pathlib import Path
from warnings import warn
from datetime import datetime, timedelta, timezone
import requests
from rich import print
USERNAME = "Remi-Gau"
# may require a token if run often
TOKEN_FILE = Path("/home/remi/Documents/tokens/gh_read_pr_bids_spec.txt")
# repo to check
GH_USERNAME = "bids-standard"
GH_REPO = "bids-specification"
DEBUG = False
def authorize():
TOKEN = None
if TOKEN_FILE.exists():
with open(TOKEN_FILE) as f:
TOKEN = f.read().strip()
return None if USERNAME is None or TOKEN is None else (USERNAME, TOKEN)
def repo_url(gh_username, gh_repo):
"""Return the URL of the repo."""
base_url = "https://api.github.com/repos/"
return f"{base_url}{gh_username}/{gh_repo}"
def get_list_of_prs(gh_username: str, gh_repo: str, auth=None):
"""List open PRs for a given repo.
Parameters
----------
gh_username : str
_description_
gh_repo : str
_description_
auth : None | tuple[str, str], optional
_description_, by default None
Returns
-------
_type_
_description_
"""
url = f"{repo_url(gh_username, gh_repo)}/pulls?per_page=100"
response = requests.get(url, auth=auth)
if response.status_code != 200:
warn(f"Error {response.status_code}: {response.text}")
return None
return response.json()
def main():
""""""
pulls = get_list_of_prs(
gh_username=GH_USERNAME, gh_repo=GH_REPO, auth=authorize()
)
for pr in pulls:
if pr["state"] != "open" or pr["draft"]:
continue
print(f"PR {pr['number']}: {pr['title']}")
if has_failure(pr):
continue
if has_positive_reviews(pr, threshold=2) and more_than_x_days_ago(pr, nb_days=5):
print(pr.keys())
print(f" {pr['updated_at']}")
def has_positive_reviews(pr, threshold=2):
url = f"{repo_url(GH_USERNAME, GH_REPO)}/pulls/{pr['number']}/reviews"
response = requests.get(url, auth=authorize())
if response.status_code != 200:
warn(f"Error {response.status_code}: {response.text}")
return False
if len(response.json()) == 0:
return False
nb_approvals = 0
changes_requested = False
for review in response.json():
if review['state'] == 'CHANGES_REQUESTED':
print(f" {review['user']['login']} - {review['state']}")
changes_requested = True
if review['state'] == 'APPROVED':
print(f" {review['user']['login']} - {review['state']}")
nb_approvals += 1
if nb_approvals > 0:
print(f" {pr['html_url']}")
return nb_approvals >= threshold and not changes_requested
def has_failure(pr):
"""Check if a PR has a failure."""
response = requests.get(pr['statuses_url'], auth=authorize())
if response.status_code != 200:
warn(f"Error {response.status_code}: {response.text}")
return True
value = False
for status in response.json():
if status['state'] == 'failure':
print(f" {status['state']}: {status['context']}")
value = True
return value
def more_than_x_days_ago(pr, nb_days=5):
"""Check if a PR has been updated more than 5 days ago."""
updated_at = pr['updated_at']
# Convert the string to a datetime object
updated_datetime = datetime.fromisoformat(updated_at.replace("Z", "+00:00")).replace(tzinfo=timezone.utc)
# Check if more than 5 days have elapsed
elapsed_time = datetime.now(timezone.utc) - updated_datetime
return elapsed_time > timedelta(days=nb_days)
if __name__ == "__main__":
main() |
|
closing, as we have managed quite well >3 years without automating this rule. Feel free to protest / re-open if you disagree. |
According to https://github.com/bids-standard/bids-specification/blob/master/DECISION-MAKING.md#rules PRs should not be merged sooner than 5 days from the last commit. Right now this rule needs to be manually observed by members with write permissions.
We could automate this by creating a bot that will a) give a request changes review to PRs younger than 5 days and remove it when the 5 day period passes b) merge mergeable PRs.
The text was updated successfully, but these errors were encountered: