Move "0001-change-pr-merging-rules" to "Accepted" state

[fballiano]

This issue is opened to allow people to vote for the approval of https://github.com/OpenMage/rfcs/blob/main/proposal/0001-change-pr-merging-rules.md

Please comment with a "+1" down below to allow us to know you agree with the proposal.

Thank you!

[kiatng]

+1

[addison74]

+1

[Flyingmana]

+1

[f1-outsourcing]

+1

[elidrissidev]

+1

[f1-outsourcing]

This issue is opened to allow people to vote for the approval of https://github.com

@fballiano

Fabrazio, is it possible to add some page or so with a listing of developers who are willing to do some coding for donations/payments. Maybe even some procedure to allow such paid pr's to go at a higher priority?

[luigifab]

+1

[tmotyl]

I disagree with tge 4non maintainer rule.
Maintainer who does a review and votes takes responsibility to fix regressions if they occur.
so we need at last one maintainer to vote and merge the patch. Otherwise the quality will go down.

[simbus82]

+1

PS: @tmotyl many big open source projects are based on a number of "non-manteiners" users approval.
But only maintainers are allowed to merge the PR.
The utility of "4 users" for me is also to dissolve any mainteners's different idea from the masses idea. If no maintainer approve a good PR for the mass, this PR will never be merged.
The risk of a regression is low if there are rules to let users approve a patch, like writing a report or use a strong test procedure.

[fballiano]

I would agree with @tmotyl in principle, if we had more active maintainers this whole thing probably would have never arisen but... if no maintainers are taking the responsability to approve or close a PR, users should have the right to do that.

Also, now that I'm reading tens of issues/PRs I can say that our members are really thoughtful and I don't see the problem of low quality code approved by 4 or more "non maintainers".

[tmotyl]

As long as the maintainer who merges stuff is responsible for taking action if sth go south, im ok with it.
the problem with community reviews are that most of the time they mean „ i like the idea” rather than „ive analyzed the code, thought about alternatives and I approve it” and very rarely they mean „ive tested the code on my projects and run manual and automated tests against it”.

Because of this other review systems like gerrit have separate votes for „look ok by reading” and „tested it and worked”.

Sure i feel the pain of not getting reviews. There are several my patches waiting for a review. And im really happy for any approach which increases the merge rate without hurting quality

[colinmollenhour]

I agree with @tmotyl that non-maintainer reviews cannot always be taken too seriously.. If the expectation can be conveyed properly I'd like to see such approvals accompanied by comments like "I've used this in production" or similar to give some weight to the approval. Perhaps non-maintainer approvals should only be counted if they are not without comment?

For example, I've seen very few (zero?) non-maintainer reviews that requested changes.. so I think the ratio of approvals to non-approval reviews for non-maintainers is significantly higher than for maintainers which would be evidence to back @tmotyl's concern about lower quality as a whole. I don't say this to minimize every non-maintainer approval, I'm sure some of them are very solid - we just need to be able to classify them and take that into account.

As an aside, thank you so much for your recent contributions, Fabrizio!

[fballiano]

In theory I would agree with both @tmotyl and @colinmollenhour but if the maintainers don't do their work (which is totally fine, everything is voluntary) they're (we're) sending OM to its death so I believe that, in this case, users should have the power to move the project forward by themselves.

We don't have a lot of active users here on github, it's not easy for a PR to get many user reviews and I think that, if a PR gets a lot of user reviews anyway without any maintainers taking care of anything, whom fault is this?

A maintainer will have to merge the PR anyway, if he/she sees all of the users reviews he/she should check if he/she sees something wrong before merging. Maintainers keep having the power to close/veto a PR. But if they don't care, users should have rights.

On a closing one I'd say that if we had more active maintainers (again, I'm not pointing fingers to anybody, we all have limited time, maybe now I can do more work but nobody know if that's gonna last forever) this whole thing wouldn't have been arise.

Last thing, although this PR is a bit disruptive and comes from a lot or "arguing" I want to say that I've extreme trust in OM's maintainers (the technical skills and experience is through the roof) and all of this topic it's because we all love OM and everybody's goal is to keep it alive :-)

[colinmollenhour]

I just saw the "4 similar comments" which includes 4 more +1s including from @Flyingmana so looks like it is almost unanimous. Since the PRs with 4 non-maintainer approvals still require a maintainer to click merge (effectively signing off) I would agree with this RFC with the understanding that there will never (without another RFC) be a process to merge a PR with zero involvement from a maintainer. Do you still object @tmotyl ?

[fballiano]

I think it's not even possible (to merge without a maintainer) :-)

[colinmollenhour]

You're right, perhaps being a bit to explicit but just putting it to rest that there wouldn't be any automated process or anything like that. 😄

[tmotyl]

No objection from my side, agree with @colinmollenhour

[Flyingmana]

Will give it 1 more week. Seems like everything speaks for acceptance 🙂

[fballiano]

Hi guys, what happens when we move this forward?

[fballiano]

any update on this?

[Flyingmana]

moved to accepted with 9d5c830