Re: Find some consensus


danielb987
 

Paul

I think you and I are on the same page, but we express it differently.

I agree that there is PRs that never should be merged, and I may have been responsible for some of these.

There are also PRs there a single reviewer strongly disagrees with the PR and doesn't accept it, while the group as a whole thinks that it should be merged.

It's for the later case, there a single reviewer disagree with the PR, but others thinks that the PR should be merged anyway, that I think should be documented on how to solve that.

The following sentense is an _example_ : If Bob Jacobsen as the owner of JMRI is the one that has veto, we could make an agreement with Bob that if at least four of us believes that a PR should be merged, despite Bobs wish, he agrees to do that.

My point is that all of us should know how to handle a dispyte. It doesn't need to be a technical solution, and it should not be a lot of text, but some sentences describing how to handle it. Just to ensure that we don't get into the situation you describe there a single developer can rule out the work of another developer.

Daniel

2020-07-22 17:44 skrev Paul Bender:

All,
I want to address a couple of the comments made by Randal and Matt.
On 7/22/20 8:12 AM, Randall Wood via groups.io wrote:
Today, Bob Jacobsen reviews every single PR and may or may not choose
to include it, effectively wielding veto power irrespective of any
other reviews. This has led to situations where Bob has (rightly or
wrongly is not relevant here) felt the need to remove the rights of
some individuals to contribute as completely as they may otherwise
have because they did not withdraw their PRs on their own, but argued
that Bob was wrong to question their choices in their PR
Yes, and I have a problem with this. in particular.
The problem is not, in and of itself, that Bob is (effectively) the only
one who reviews PRs.
The problem is that Bob effectively can decide who contributes to the
project and how.  None of us should live in fear of loosing the ability
to contribute to the project in the way we wish to because we have a
disagreement with Bob.
Yes, maintainers can merge pull requests, but the reality is that most
maintainers only merge their own code.

The repo owners (Bob Jacobsen and maybe others) can override a veto
and merge anyway, but outside of that, no-one else should be able to.
I deliberately provided examples where not having a veto could have
expensive consequences (members of this project have been sued for
actions taken in this project), and not grayer examples of a veto in
action, but they do exist in the GitHub history.
Randall is absolutely right that there ARE valid reasons for a PR to be
vetoed.  Technical and legal reasons being the major ones.
On 7/22/20 7:04 AM, Matthew Harris wrote:
Taking on Bob M and Daniel's points, there is a mechanism available
within GitHub that can get us close to what's being asked.
It's the `CODEOWNERS` file -
see https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners
Until now, it's not something we've considered but is something that
could help guide us.
In essence, it allows us to setup certain areas of the tree to require
a code review by the nominated persons or groups.
I think it would certainly be a good idea for this to get put into place.

This file is only writable by Owners or Administrators of the
repository so cannot easily get accidentally modified.
In addition, there is the whole protected branches configuration we
have right now. In addition to the various required status checks, the
`release-*` and `dev-*` branches are only available to be written to
by the `release-pumpkins`.
Actually, the master branch is a protected branch.
The way things are currently configured, owners, administrators, and
maintainers are able to merge to the master branch.

As far as the notion of reviews goes, I am personally in favour of the
'four-eyes principle' - in other words, ensuring that at least two
sets of eyes get the chance to look at a contribution, namely the
original contributor plus at least one other person. This can also
help us to ensure that each area of the codebase has more than one
person who is familiar with what goes on.
This is my opinion as well.
This is the reason that I state that there should be no exceptions to
the peer review rules.
There is a checkbox in the github configuration for protected branches
which indicates that the rules also apply to administrators.  I think
that needs to be implemented.
In other words, my recommendation for how to configure github for reviews:
1) Require reviews for PRs to the master branch. (or, if we implement
the semantic versioning as Bob outlined, to all branches that could lead
to a release).
2) Apply the review requirement to all users (administrators included)
3) Use the code owners file to pick the right reviewers for a given PR.
4) Reset approvals if any new commits are added to the PR.
We might also consider a minimum time period a PR must be open to allow
other reviewers time to look over the code before it is merged.  This
isn't behavior built into github, but there are github actions available
that claim to be able to enforce this ( see
https://github.com/marketplace/actions/hold-your-horses ).  This also
makes it harder for someone to sneak a controversial change in, because
it forces us to allow time for anyone to review.
Paul

Join jmri@jmri-developers.groups.io to automatically receive all group messages.