Merging pull requests before 72 hours

[Chris Jones]

Hi,

On 16/10/18 07:37, Leonardo Brondani Schenkel wrote:
>> On 15 Oct 2018, at 11:18 pm, Chris Jones <jonesc at hep.phy.cam.ac.uk> 
>> wrote:
>>
>>> On 15 Oct 2018, at 10:34 pm, Leonardo Brondani Schenkel <lbschenkel 
>>> at macports.org> wrote:
>>> I'm a committer, and if I'm doing a trivial bump of an openmaintainer 
>>> port I'll push it directly.
>>
>> Depends entirely on what you consider trivial. If you consider a 
>> version update a trivial bump then I disagree. This in my opinion is 
>> something that should also be submitted as a PR so the port maintainer 
>> always gets a chance to review it. If the port is open maintainer and 
>> they do not respond within the 72 hour timeout, it can then be merged 
>> without their comment, but not before. For a committer to directly 
>> commit an update to a port that isn’t trivial and they do not maintain 
>> is in my humble opinion an abuse of the member role. Members should 
>> not bypass the PR review stage just because they can.
> 
> Like it or not, that's the definition of 'openmaintainer': for simple 
> version bumps, no maintainer approval is necessary.

Please point me to where this is documented ? i.e. where is it stated 
that openmaintainer allows revision changes. This appears to be one of 
the issues here since not everyone, myself included, agrees with you. 
The only statement I have found is

"If a port's maintainer contains the address 
<openmaintainer at macports.org>, this means that the author allows minor 
updates to the port without contacting him first. But permission should 
still be sought for major changes."

which does not mention this point. It leaves the interpretation of minor 
and major to the reader.

Chris

  Since the maintainer
> is the one explicitly opting in to such policy, I don't see any issue 
> with respecting their wish. I deliberately added 'openmaintainer' to all 
> the ports I maintain so others are free to do version bumps without 
> going through me first, reducing friction (but not other changes, see 
> below) >
> Ports that tend to break on any minor version bump, or are problematic 
> in other ways, should probably not be marked as 'openmaintainer' -- and 
> I believe they aren't.
> 
>>
>>> If I'm opening a GitHub PR for an openmaintainer port this means that 
>>> for some reason I want the maintainer's opinion/review before it gets 
>>> merged.
>>
>> In my opinion the ‘rules’ for when this happens should not be 
>> subjective, as they currently seem to be. There needs to be a clear 
>> policy on what is and is not trivial and thus what does and does not 
>> require PR review.
> 
> I agree that it looks like different people have different 
> interpretations for the scope of 'openmaintainer' (apart from simple 
> version bumps being allowed, which is the definition that is explicitly 
> written in the documentation).
> 
> My own interpretation is that any change that is not a simple version 
> bump, a simple revision bump due to a dependency being upgraded, or 
> fixing a livecheck or a typo, do not fall within the scope of 
> 'openmaintainer' and should be cleared with the maintainer first.
> 
>>> My belief is that in general nobody else should merge the PRs besides 
>>> the submitter of the PR and/or the maintainer.
>>
>> Disagree. Any member with commit rights should be able merge PRs. As 
>> long as the procedures have been followed, so either the maintainer 
>> has oked the changes, or the 72 hour timeout has expired and another 
>> reviewer has agreed with the changes, it does not really matter who 
>> finally clicks merge.
> 
> Naturally any member can merge PRs by virtue of having the rights to do 
> so, but my point was that when a PR is opened against a maintained port, 
> the recipient for that PR is the maintainer although it can be 
> additionally reviewed/commented by anybody else (I suspect most PRs 
> don't). Barring maintainer timeouts and other special circumstances, 
> it's the maintainer that ultimately decides if that PR should be merged. 
> At that point, they can signal it by approving the PR -- and depending 
> on who has commit rights they can merge it themselves, or ask the 
> submitter to merge, or ask any non-participant member to merge.
> 
>> ( Its actually quite common practise in many circles for submitters of 
>> a PR to require third party approval, and for someone else to merge 
>> it, and for the submitter to simply do it themselves to be discouraged. )
> 
> In the scenarios I have in mind and I participated in, the submitter 
> merges if they're asked to by the maintainer or when the PR is approved 
> without requests for changes or other concerns. Sometimes I submit PRs 
> to other ports maintained by non-members, and I'm the one who has commit 
> rights.
> 
>>
>>> They can merge at any time of their choosing in case they want to 
>>> proceed, and there is no need to second guess their intentions.
>>
>> Only the maintainer of a port should be free to merge before the 72 
>> hour timeout, if they have the rights to do so. If they do not have 
>> commit rights, then can signify they have agreed to the changes at 
>> which point any member could merge. 
> 
> I think I wasn't clear on that point. What I meant is that a PR is 
> either on a 'waiting for review/reviewing' phase, or an 'approved/ready 
> to be merged' phase, and once the PR is 'approved' then anybody can 
> merge. If it's on the 'review' phase, it's up to the participants to 
> transition that PR into 'approved' and merge (or ask for it to be 
> merged). I see very little room here for a non-participant third-party 
> to second guess a participant's intention and prematurely merge a PR -- 
> and if this seems to be warranted, simply asking first will eliminate 
> the vast majority of misunderstandings.
> 
> // Leonardo.