Merging pull requests before 72 hours

[Christopher Jones]

Hi,

> On 17 Oct 2018, at 1:17 am, Rainer Müller <raimue at macports.org> wrote:
> 
> On 2018-10-16 10:06, Chris Jones wrote:
>> 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.
> 
> What you describe would be the process without openmaintainer. With
> openmaintainer, "minor updates" can be pushed directly by other project
> members.
> 
> I would always assume that a project member was looking for explicit
> maintainer approval when submitting a change to an openmaintainer port
> as a pull request. Otherwise they could just have pushed it directly.
> Therefore I would recommend not to merge them unless approved by the
> port's maintainer.

What then about non-member git PRs, should they wait the 72 hours before merging or not ? Non-members have no othert option other than submit a PR (or a ticket).
If a member reviews one of these with the 72 hours and deems it ‘minor’, surely to be consistent with the
fact they could have just committed that change directly themselves, they should be free to merge it immediately ?

Allowing members to directly commit ‘minor’ changes directly but then complaining when ‘minor’ PRs get merged quickly to me is inconsistent.

> 
>>> 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."
> 
> For the record, this is from the maintainer update policy in the guide:
> https://guide.macports.org/#project.update-policies.nonmaintainer
> 
>> which does not mention this point. It leaves the interpretation of minor
>> and major to the reader.
> 
> I always assumed that "minor update" means increasing version/revision
> of a port. If you did not want that allowed, what else would
> openmaintainer cover?

This is the problem. What is written is not explicit and open to interpretation. I, for instance, have always felt version updates generally are not minor updates. 
> 
> The problem has always been that the definition of what is considered a
> minor or a major change largely depends on the software we are
> packaging. It is hard to find a more specific rule that still applies
> universally to all ports.

That is understood. Either we allow ‘minor’ version updates directly, and accept that there will always be disagreements on what this means exactly, and sometimes a ‘minor’ update will be made that turns out not to be so minor, or we don’t allow any version updates under the ‘minor’ tag, and accept the additional work as members that *every* update has to then go via a PR. The guide also could perhaps be improved to be a bit more explicitly on what minor means. For instance, if we agree that includes minor version updates, then that should be stated.

Chris

> 
> In general, if you as a maintainer do not want others to make updates,
> you should not add the openmaintainer policy.
> 
> Rainer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1930 bytes
Desc: not available
URL: <http://lists.macports.org/pipermail/macports-dev/attachments/20181017/a7a5ad9f/attachment.bin>