> python merge_pr.py --check-approved --pr 41110
2022-09-08 20:36:48,885 Going to process PR #41110 in repo ClickHouse/ClickHouse
2022-09-08 20:36:48,892 Found credentials in shared credentials file: ~/.aws/credentials
2022-09-08 20:36:52,084 Checking the PR for approvals
2022-09-08 20:36:52,321 There aren't reviews for PR #41110
2022-09-08 20:36:52,321 We don't merge the PR

executing it locally. Would somebody approve it so I can test the script?

Oh dear, the logic of taking the latest review per user isn't a good idea

In [109]: repo.get_pull(41110).head.repo.pushed_at
Out[109]: datetime.datetime(2022, 9, 12, 13, 35, 11)

In [110]: repo.get_pull(41110).head.repo.pushed_at
Out[110]: datetime.datetime(2022, 9, 12, 13, 35, 19)

In [111]: repo.get_pull(41110).head.repo.pushed_at
Out[111]: datetime.datetime(2022, 9, 12, 13, 35, 19)

In [112]: repo.get_pull(41110).head.repo.pushed_at
Out[112]: datetime.datetime(2022, 9, 12, 13, 35, 52)

Hmm, doesn't look like pushed_at works

I think it's OK to have approval from collaborators as a marker that the PR is generally ok.

update: we discussed internally, and the script will consider only members' reviews

Oh no, the PR would be merged on Finish in any case, even if there are failed statuses. It needs another check

Maybe it also worth to check:

• that the PR is not in a draft state? (just in case)
• that there are no unresolved comments

Actually right now github provide everything similar:

• Require approvals
• Dismiss stale pull request approvals when new commits are pushed
• Require review from Code Owners
• Require status checks to pass before merging
• Require conversation resolution before merging

The only problem is that you have to have write access to merge PR, so this is great, but will not work.

Actually I found auto-merge. So maybe all of this can be simply replaced with configuring proper policies on github?

And AFAIU anybody can select it, and this looks better then simply auto merge, maybe an author will find more problems with the patch.

P.S. llvm uses different policy

We can limit the recognized approvals to the core team.

@azat the GH's approach is pretty dumb (not in a bad way, just simple) and straightforward.

• First, we don't want to force approvals, and either they are strictly required, or aren't at all
• The status checks are as well very, very unflexible. Either it completely blocks the merge, or it is just ignored completely.

These two don't allow us to consider the solution, it just won't work for us