Update CONTRIBUTING.md with guidelines for commits, PRs, reviews and merges

[gregw]

The CONTRIBUTING.md has the following:

Before your contribution can be accepted by the project team contributors must
electronically sign the Eclipse Contributor Agreement (ECA).

* http://www.eclipse.org/legal/ECA.php

Commits that are provided by non-committers must have a Signed-off-by field in
the footer indicating that the author is aware of the terms by which the
contribution has been provided to the project. The non-committer must
additionally have an Eclipse Foundation account and must have a signed Eclipse
Contributor Agreement (ECA) on file.

I think we need to consider having a lot more in there:

Code

• code should use https://github.com/eclipse-ee4j/ee4j code style
• Do we want to run check style also?

Pull Requests

• Should PRs from committers use branches within the repo or come from personal forks?

Merging

• CI must pass
• Must be reviewed by at least 1 project leader?
• Should we have a minimum age so that committers in all timezones PRs have a chance to review?
• Should any committer be able to call for a formal -1, 0, +1 style vote on a PRs (as is described in the EDP)?
• With votes, who is it that may adjudicate if a -1 is an Invalid Veto as per the process?

I think we need to discuss what guidelines we want, formulate them into a PR on CONTRIBUTING.md and then have an EDP style vote on it to make it official.

[gregw]

My thoughts are as follows:

• checkstyle must pass
• CI must pass
• PRs can come from branches in any repo. Committers must cleanup branches in the main repo.
• Any committer can call for a formal vote in a PR comment, which will be take at 1 week and be adjudicated by the project lead(s).
• Non voted on PRs can be merged after being non-draft for 1 week and having 2 reviews from committers from a different organisation(s) to the author(s).
• Project leads may bypass any of these criteria, but must document their reasons for doing so.

[markt-asf]

I agree with your views on CheckStyle, CI and PR branches. I'd say that any committer can override those criteria as long as they document the reason. Implicit in that is that if the consensus is that the reason isn't valid, then the commit may need to be reverted. (Other options include an additional correcting commit, leave as-is but agree to use a PR for future similar changes etc.) I'll note I don't particularly mind which style we enforce as long as there is tooling to support formatting to that style and checking that the style is followed.

I think we need less process around PRs, not more. I'd advocate for:

• remove requirement for review before a PR can be applied
• trust committers to know what can be 'just applied' and what needs to go via a PR and how long to give folks to review it
• we should be aiming for community consensus rather than requiring votes or explicit reviews (on the basis silence indicates consent)
• I'd expect a minimum of 72 hours review where it was considered required with longer being allowed if there isn't consensus on the way forward

This morning we have had the import of the spec document with only a few hours passing between the PR being opened and the commit to master. And I am fine with that. It was completely appropriate for those circumstances. I'd hate to have seen that delayed by a week or more.

As the spec document is cleaned up I'm expecting some changes to be 'obvious', others to require more discussion. I would struggle to define rules that provided the right amount of oversight without adding unnecessary delay.

I would prefer to see the community moving relatively quickly and making the odd mistake that had to reverted rather than adopting rules that required us to move slowly.

[gregw]

This morning we have had the import of the spec document with only a few hours passing between the PR being opened and the commit to master. And I am fine with that. It was completely appropriate for those circumstances. I'd hate to have seen that delayed by a week or more.

That was me and it felt wrong the moment I clicked the button. I've screwed up enough commits/merges that have seamed to me to be obvious/trivial/simple only to later discovered something that a simple review would have revealed... if only I'd gone through a PR and waited a bit. Hence I lean towards a bit more process by default, but happy to allow overrides if reasons are documented, and I agree that could extend to all committers.

[stuartwdouglas]

Once we have the 5.0 release out I am fine with enforcing time limits, but I don't think it is practical if we want to get the spec document out in a timely manner. We are likely going to have lots of formatting/clean up changes that can potentially conflict, IMHO we want to get these in as quickly as possible to reduce the risk of conflict, if there is a problem we can always fix it later.

Once we are working on 5.1 and actually adding stuff to the spec I think a longer review process is appropriate.

[gregw]

So how about "New features or changed behaviour must be submitted via a Pull Requests that is open for comment in a non draft state for at least 1 week before merging.". I think all 5.0 work will fall outside of that as it will not be new feature or changed behaviour.

[gregw]

How about the addition below? They are just guidelines, mostly at "should" strength and with explicit bypass allowed. I have added a point that merging a PR doesn't give it any special status - ie we must always seek consensus to include rather than revert, else there is an incentive to rush to merge contentious PRs.

Should I make this as a PR so we can comment on individual lines... or are we not there yet?

Project Contribution Guidelines

• Code should adhere to the style defined at https://github.com/eclipse-ee4j/ee4j and should pass checkstyle.
• Contributions should be made via github pull requests at https://github.com/eclipse-ee4j/servlet-api/pulls
• Pull Requests should have reviews requested from several project committers from different organisations than the author(s).
• Pull Request by committers that receive no review responses after a reasonable time, considering time zones and working days, can be interpreted as approved.
• Pull Requests should pass continuous integration tests prior to merging.
• Pull Requests for significant contributions, new features or changed behaviour should be open for comment and review in a non draft state for at least 1 week prior to merging.
• For disputed Pull Requests, any committer may request a formal consensus vote (ie. +1, 0, -1). Any such vote will be scheduled, conducted and adjudicated by the Project Leader(s).
• If a disputed Pull Request is merged prior to a consensus being established, then it shall be considered reverted and consensus must be sought for it to remain rather than for it's reversion.
• In trivial or exceptional circumstances, these guidelines may be bypassed if the reasons for doing so are documented.

[markt-asf]

I think we are close enough for a PR now. I have some comments but they are minor "per line" comments.