[RFC] CONTRIBUTING.md: add four eyes requirement for committers

[poranje]

Maintainer: @thess

Compile tested: n.a.
Run tested: n.a.
Description:
The happy addition of five new committers makes application of four eyes more relevant.
Add a soft requirement to that end.
Please feel free to improve the wording.
Regards,
Paul

[poranje]

Check buildpr fails with "Commit subject line MUST start with '"
CONTRIBUTING.md has been used for latest commits relating to that file.
What should the package name be to pass the check ?

[thess]

I think it's the '.' in the package name - just checked the RE used to match and it doesn't allow for periods in the name. I'll fix that in a bit. (My last remark about the RFC did not apply here - deleted)

[hnyman]

Make no sense for e.g. simple version bumps without any other changes.

It may also be hard to find reviewers who actually have compiled/used/tested the package for more exotic packages.

Yes, having somebody else to review the changes would be good practice for more complex changes, e.g. like in the complex context of python package packaging changes, but is not necessary for many usual simple upgrades.

Note that there is no such requirement on main OpenWrt.

If this kind of advice is put into the file, it should be even softer than the current proposal.

[stintel]

I'm inclined to NAK this.
I'm not going to put extra effort in creating PRs and waiting for review for changes to packages for which I am the maintainer. I will just push them directly as I have always been doing.

For changes to packages that are maintained by someone else, you preferably wait for maintainer approval. For when the maintainer doesn't respond, we could introduce a policy like this:
If there is no response from the maintainer(s) within 4 weeks, a PR can be merged after approval by 2 committers.

[thess]

I think the current items plus the CI checks are enough guidelines for how we would like to operate. Experience has shown us that we get more backlog and/or non-participation if too many obstacles 😉

So, NAK from me also.

[diizzyy]

I'm not against having all commits to be reviewed by someone (ie PR) before pushed but it seems like most are not in favour of that proposal.

What I would however like to implement is a requirement of use case(s) when doing a PR for a new package and some kind of voting system whether to accept based on 2-3(?) members with commit access that would apply for everyone. Mainly because of two reasons, avoid limbo status of PRs and avoid getting unmaintained (niche/drive-by submitted and merged) packages.

Edit: FWIW, I did propose a similar timeout policy a while back (more or less adopt FreeBSD's package repo policy) however nothing came out of the discussion in the end.

[neheb]

NAK here as well. Not that it matters. OpenWrt is not a big project like Debian or Arch. Something like this can't be afforded.

[poranje]

To wrap up: proposal currently lacks support because the anticipated extra effort, e.g. for trivial changes.

Still, this policy proposal states "preferably", which would not hinder merging trivial commits without ado. Plane version updates of packages that have already been merged elsewhere in renowned distribution like Debian fall into the same category.

Since the number of committers has grown, some space for extra review is likely. And please keep in mind that a little bit of extra quality assurance yields in terms of less work in the end.

As an ardent follower of this project I have noticed that the extra impetus that the project lately shows also seemingly has come with some more rash changes. Forgive me for not providing a list of such cases as it would be unwanted somewhat targeted (and, of course I am to(o) lazy).

Without further suggestions for better wording or support otherwise I will close this PR within a fortnight.

[BKPepe]

I hope that I am not a little bit late to the party.

First, I appreciate every feedback, but yeah five people get commit access and one of them was me and since December I have been able to see, how maintainers and people with commit access are doing it. I don't have anything against you, so don't take it wrong, but I didn't see you are active and you would see that not many people are left here. However, I see what you mean. So I might have a different view on this issue.

My issues, what I observed so far:

• Sometimes, I have been able to see that PR was merged too fast.
  No chance to review it or even propose any change.
• There are some situations, when did someone want to push the changes directly, but within a few minutes, he realized that he had bad checksum and do another commit.
• Why some new packages were added in a hurry and within 3 days appeared 3 pull requests as fixups and others are waiting to be accepted for more than a month? Was it really necessary?
• Some PRs were merged even that one of the checks failed, also some PRs were merged even there were not run tested. Even I created a comment in that PR, but nobody responded to me and it was merged. Why someone is filling GitHub pull request template and doing what they can (at least do some testing) and others are not?
• Some maintainers are not open to any suggestions. Honestly, I am not surprised, but sometimes it could be useful if they could listen and do some change.

And my last point, I found that in the repository there were 2 packages, which couldn't be tested at all. So in the end, it doesn't matter if you will run and compile tested as it is about trust. That's why you can see that in some my pull requests I added screenshots that it was indeed tested.

I will appreciate if something what I have mention could be improved. While I agree that it would be good if PRs would be reviewed, however, there are just two people, who are really proactive it in and it's @diizzyy and @neheb.

For Python packages would be cool if we can use CODEOWNERS as proposed in #7125 as they are two people (but they need to have commit access), who know a lot about Python in OpenWrt and in all my PRs for Python, I'm including them in CC.

As it was said in some old issue, there are many packages, which are not maintained and what we should do about them? I can do a pull request to update it, but how long I should wait to merge it? Will there be someone new, who can take ownership from the previous maintainer? What about new packages? We shouldn't stop adding new packages as already we have more than 150 pull requests. I think it's really bad, when the PR was reviewed by 2-3 or people and it was merged after 2-3 weeks when it was reviewed.

In the perfect world, it would be good if we can not apply rules just for someone, but for all.

TL,DR: All is about people and maintainers. We should do some change. However, as it was said, sometimes it doesn't make any sense to do it for simple version bump. I agree that there should be some proposal what we should do, when maintainer is inactive and sometimes when the package has security issue even 4 weeks could be a really long period. It's really easy to say that all PRs should be reviewed, but we need really see all situations and think about it.

[diizzyy]

This is why almost all prefers runtime tests and preferably have clean checks before merging. I'm fine with people committing to their own packages but at least to a PR just to avoid unnecessary breakage as we all do mistakes. Regarding pull requests we can continue the discussion here #8528

[poranje]

I hope that I am not a little bit late to the party.

First, I appreciate every feedback, but yeah five people get commit access and one of them was me and since December I have been able to see, how maintainers and people with commit access are doing it. I don't have anything against you, so don't take it wrong, but I didn't see you are active and you would see that not many people are left here. However, I see what you mean. So I might have a different view on this issue.

No sweat, no offence taken.

My issues, what I observed so far:

* Sometimes, I have been able to see that PR was merged too fast.
  No chance to review it or even propose any change.

* There are some situations, when did someone want to push the changes directly, but within a few minutes, he realized that he had bad checksum and do another commit.

* Why some new packages were added in a hurry and within 3 days appeared 3 pull requests as fixups and others are waiting to be accepted for more than a month? Was it really necessary?

* Some PRs were merged even that one of the checks failed, also some PRs were merged even there were not run tested.  Even I created a comment in that PR, but nobody responded to me and it was merged. **Why someone is filling GitHub pull request template and doing what they can (at least do some testing) and others are not?**

* Some maintainers are not open to any suggestions. Honestly, I am not surprised, but sometimes it could be useful if they could listen and do some change.

And my last point, I found that in the repository there were 2 packages, which couldn't be tested at all. So in the end, it doesn't matter if you will run and compile tested as it is about trust. That's why you can see that in some my pull requests I added screenshots that it was indeed tested.

Indeed and that is where the proposed extra safeguard may help.
IMHO often there are to many rules which also discourage grass root participation and organisation, but for security purposes it is difficult to do well without some annoyance.

I will appreciate if something what I have mention could be improved. While I agree that it would be good if PRs would be reviewed, however, there are just two people, who are really proactive it in and it's @diizzyy and @neheb.

You are to pessimistic, there are more than two, but indeed, these guys do an incredible lot of work, almost to much, which is a reason for being cautious and alert.

For Python packages would be cool if we can use CODEOWNERS as proposed in #7125 as they are two people (but they need to have commit access), who know a lot about Python in OpenWrt and in all my PRs for Python, I'm including them in CC.

As it was said in some old issue, there are many packages, which are not maintained and what we should do about them? I can do a pull request to update it, but how long I should wait to merge it? Will there be someone new, who can take ownership from the previous maintainer? What about new packages? We shouldn't stop adding new packages as already we have more than 150 pull requests. I think it's really bad, when the PR was reviewed by 2-3 or people and it was merged after 2-3 weeks when it was reviewed.

Very good questions which have popped up in earlier discussions also, but annoying as these issues are, they are a bit OT in respect to this PR. PR #6584 ran a rather longish discussion about that.

In the perfect world, it would be good if we can not apply rules just for someone, but for all.

The idea of rules is that these do apply for all, like the rule of law. A perfect world is not a requirement.

TL,DR: All is about people and maintainers. We should do some change. However, as it was said, sometimes it doesn't make any sense to do it for simple version bump. I agree that there should be some proposal what we should do, when maintainer is inactive and sometimes when the package has security issue even 4 weeks could be a really long period. It's really easy to say that all PRs should be reviewed, but we need really see all situations and think about it.

[poranje]

This is why almost all prefers runtime tests and preferably have clean checks before merging. I'm fine with people committing to their own packages but at least to a PR just to avoid unnecessary breakage as we all do mistakes.

So that is covered by the rule "Do NOT use git push --force." ?

Regarding pull requests we can continue the discussion here #8528

Okay, I'll close this PR.