Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disambiguate and document the closure process #7516

Merged
merged 23 commits into from
Mar 15, 2023
Merged
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9dc450a
Disambiguate and document the closure process
basil Dec 12, 2022
d3c0946
Merge branch 'master' into closure
basil Dec 14, 2022
cfbf1a5
Fixups
basil Dec 14, 2022
f0c61cb
Clean up table
basil Dec 14, 2022
746559b
Percentages did not make a difference in GitHub rendering
basil Dec 14, 2022
d983ea1
Merge remote-tracking branch 'origin/master' into closure
basil Dec 17, 2022
54dfb5a
Merge remote-tracking branch 'origin/master' into closure
basil Dec 18, 2022
31b54a8
Merge branch 'master' into closure
basil Jan 16, 2023
22173dc
https://github.com/jenkinsci/jenkins/pull/7516#pullrequestreview-1227…
basil Jan 16, 2023
f568fee
https://github.com/jenkinsci/jenkins/pull/7516#issuecomment-1384369235
basil Jan 16, 2023
a48d5cd
Merge branch 'master' into closure
basil Jan 19, 2023
6f48f90
Update docs/MAINTAINERS.adoc
basil Jan 19, 2023
be39519
https://github.com/jenkinsci/jenkins/pull/7516#discussion_r1049385637
basil Jan 19, 2023
f866bfc
https://github.com/jenkinsci/jenkins/pull/7516#discussion_r1051681935
basil Jan 19, 2023
0f18a46
https://github.com/jenkinsci/jenkins/pull/7516#discussion_r1051687276
basil Jan 19, 2023
8a35089
Merge branch 'master' into closure
basil Jan 28, 2023
b7bf24d
https://github.com/jenkinsci/jenkins/pull/7516#discussion_r1089739236
basil Jan 28, 2023
a45419c
https://github.com/jenkinsci/jenkins/pull/7516#discussion_r1089739525
basil Jan 28, 2023
b24bd7b
Update docs/MAINTAINERS.adoc
basil Feb 10, 2023
45d9750
Merge branch 'master' into closure
basil Feb 10, 2023
3a1bfd3
copyedit
basil Feb 10, 2023
a8994a2
https://github.com/jenkinsci/jenkins/pull/7516#discussion_r1102683288
basil Feb 10, 2023
283f7b9
Merge branch 'master' into closure
basil Mar 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 146 additions & 12 deletions docs/MAINTAINERS.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Remoting updates in the core are subject to the process though.
* Issue Triage Team Member
* Core Pull Request Reviewer
* Core Maintainer
** Pull Request Assignee
* Release Team Member

**Contributors** submit pull requests to the Jenkins core and review changes submitted by others.
Expand All @@ -53,7 +54,10 @@ and to guide newcomer contributors who are not familiar with the project's proce
GitHub team: link:https://github.com/orgs/jenkinsci/teams/core-pr-reviewers[@jenkinsci/core-pr-reviewers].

**Core Maintainers** get `Write` permissions in the repository, and hence they are able to merge pull requests.
Their responsibility is to perform pull request reviews on a regular basis and to merge ready pull requests towards the weekly releases (`master` branch).
Their responsibility is to perform pull request reviews on a regular basis and to bring pull requests to closure,
either by merging ready pull requests towards weekly releases (`master` branch)
or by closing pull requests that are not ready for merge after an extended period of time.
basil marked this conversation as resolved.
Show resolved Hide resolved
Core maintainers make a commitment to bringing a pull request to closure by becoming an **Assignee.**
They are also responsible to monitor the weekly release status and to perform triage of critical issues.
GitHub team: link:https://github.com/orgs/jenkinsci/teams/core[@jenkinsci/core].

Expand Down Expand Up @@ -84,7 +88,10 @@ At the same time, we are interested to make the review process as simple as poss
Pull requests review in Jenkins is not just about reviewing code and accepting them if the code is OK.
Core maintainers are expected to ensure feasibility and compatibility of changes,
to maintain a good quality of the codebase and documentation,
and to ensure there is a consensus between contributors.
to ensure there is a consensus between contributors,
and to bring pull requests to closure in a timely fashion,
either by merging ready pull requests towards weekly releases
or by closing pull requests that are not ready for merge after an extended period of time.

==== Verifying Feasibility

Expand Down Expand Up @@ -149,6 +156,141 @@ code reviewers are expected to suggest proper channels for contributors to discu
* If no consensus can be reached on the mailing list,
voting at the link:https://www.jenkins.io/project/governance-meeting/[Jenkins Governance Meeting] can be used to get a final decision.

=== Bringing pull requests to closure

==== Motivation
basil marked this conversation as resolved.
Show resolved Hide resolved

An obvious goal of the project is to deliver value to end users,
without which end users would cease use of the delivered software.
A pull request represents potential value for end users,
value which is only realized when the pull request is merged and delivered in a shipping release.
basil marked this conversation as resolved.
Show resolved Hide resolved

[cols="1,1"]
|===
basil marked this conversation as resolved.
Show resolved Hide resolved
|Optimal Outcome|Suboptimal Outcome

|When a pull request is merged and delivered in a shipping release, users are rewarded with this value.
|Inversely, when a pull request remains unmerged and unreleased for an extended period of time, users are deprived of this value.
|===

Another explicit goal of the project is to encourage both new and seasoned contributors alike.

[cols="1,1"]
|===
|Optimal Outcome|Suboptimal Outcome

|When a submission that is ready for merge is approved, merged, and released in a timely fashion, the contributor is more likely to contribute again.
|Inversely, when a submission that is ready for merge languishes without timely approval, merge, and release, the contributor is less likely to contribute again.
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
|When the contributor of a submission that is not _yet_ ready for merge is provided with clear, actionable, and justified feedback and when, after the action has been taken, the submission is subsequently reviewed again, approved, merged, and released in a timely fashion, the contributor is more likely to contribute again.
|Inversely, when the contributor of a submission that is not _yet_ ready for merge is provided feedback without reasoning or asked questions that do not ultimately lead to a clear action item, the contributor is less likely to improve the quality of the submission.
|When contributors and reviewers successfully negotiate scope, the contributor is more likely to complete the submission.
|Inversely, when contributors and reviewers fail to negotiate a middle ground regarding scope, the contributor is less likely to complete the submission.
|When an impractical submission is reviewed and explicitly rejected with reasoning in a timely fashion, the contributor is more likely to improve the quality of future submissions.
|Inversely, when an impractical submission is ignored without an explicit rejection or rejected explicitly without reasoning, the contributor is less likely to improve the quality of future submissions.
|===

For these reasons, core maintainers are expected not only to review pull requests but also to bring them to closure in a timely fashion,
either by merging ready pull requests towards weekly releases
or by closing pull requests that are not ready for merge after an extended period of time.
As part of the process of bringing pull requests to closure,
core maintainers are expected to steer discussions towards the identification of clear action items with reasoning
and to explicitly reject with reasoning pull requests for which there are no clear and justified action items or for which such action items remain incomplete after an extended period of time.

==== Indicating intent to close
basil marked this conversation as resolved.
Show resolved Hide resolved

Core maintainers communicate their intention to bring a pull request to closure by adding themselves to the pull request in the **Assignees** field,
basil marked this conversation as resolved.
Show resolved Hide resolved
through which they make a commitment to work with the contributor to either merge the pull request or to explicitly reject it.
To avoid ambiguity, at most one (1) core maintainer should be assigned to a pull request.
Only core maintainers should be assigned to pull requests,
since a non-maintainer would be unable to fulfill the commitment by merging the pull request or explicitly rejecting it.
To avoid making commitments on behalf of others that cannot be fulfilled,
core maintainers should only assign pull requests to themselves and not to other core maintainers.
An exception to the above would be if, following the adoption of this system, a pull request is brought to closure but remains unassigned.
In that case, any core maintainer can retroactively assign the pull request to the core maintainer who closed it for tracking purposes.
If a core maintainer does not regularly close pull requests,
the maintainer should consider whether it is appropriate to remain on the core team
in light of the responsibility to closing pull requests that such membership implies.
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not convinced that should be part of the documentation. That could add more pressure on the people, leading to less engagement.

Inactive core maintainers will not even read that documentation ;)


==== Providing actionable and justified feedback

Once assigned to a pull request, a core maintainer should make every reasonable effort to drive the pull request to closure in a timely fashion.
If further action is needed before the pull request can be accepted, this action should be explicitly requested along with the reasoning behind it.
Contributors are far more likely to successfully complete action items when the reasoning behind the request is explicit and cogent.
It is perfectly reasonable for the assignee or any other reviewer to ask questions,
but the ultimate goal of these questions should be to arrive at clear and justified action item(s);
otherwise, the process can languish for an extended period of time.
It is the responsibility of the assignee to steer the discussion towards concrete and justified action item(s).

==== Providing confirmation that feedback has been addressed

Once any requested actions have been taken, the assignee should make every reasonable effort to provide explicit confirmation that each action item has been completed.
This gives contributors positive reinforcement and confidence that their submission is moving forward through the process,
ultimately making them more likely to complete the process and contribute again.
If the assignee cannot provide such confirmation in a timely fashion,
the assignee should consider whether it is appropriate to remain assigned to the pull request
in light of the commitment to the contributor that such an assignment implies.

==== Negotiating scope

Not every pull request reaches a state of perfection at the end of the review process.
Sometimes, requests are made that, while justified, represent an additional amount of work that the contributor may not be willing to do.
In some cases it is critical to complete the additional work, but in others "you aren't gonna need it" (YAGNI).
In such cases, the assignee should make a good faith effort to negotiate with the contributor to find a reasonable middle ground that is "good enough."
Failure to negotiate successfully can often chase contributors away.
If the additional work is simple enough and the submission is not moving forward,
the assignee may consider occasionally giving the contributor a lift by completing the additional work,
though this is not expected in the general case
and would not be fair to the assignee if a large amount of additional work is necessary.

==== Voting process

A pull request can often serve as a catalyst for a discussion in which several possible paths forward are identified.
When there is no clear consensus among the core maintainers about the path forward,
the assignee should call for a vote.
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
While only core maintainers have formally binding votes, any interested parties are generally encouraged to vote, even if their votes are merely advisory.
basil marked this conversation as resolved.
Show resolved Hide resolved
To avoid ambiguity, it is preferred that votes be done using https://www.apache.org/foundation/voting.html#expressing-votes-1-0-1-and-fractions[Apache conventions].
basil marked this conversation as resolved.
Show resolved Hide resolved
A -1 vote by a core maintainer stops a pull request in its tracks.
This constitutes a veto, and it can neither be overruled nor overridden under normal circumstances.
basil marked this conversation as resolved.
Show resolved Hide resolved
Vetoes stand until and unless the core maintainer withdraws the veto.
To ensure that vetoes are used prudently,
basil marked this conversation as resolved.
Show resolved Hide resolved
the core maintainer must provide with the veto a technical justification showing why the change is bad
(e.g., opens a security exposure, negatively affects performance, etc.).
A veto without a justification is invalid and has no weight.

==== Acceptance [[acceptance]]

Once a pull request has reached the point where it is ready for merge, it is time to begin the countdown period by applying the `ready-for-merge` label.
To avoid ambiguity, this label should only be applied by a core maintainer who actually intends to merge the pull request.
Non-maintainers, including members of the core PR reviewers team, should not start the countdown period,
as this sends a signal to the contributor that their submission will be merged soon when in fact there may not be a core maintainer who has committed to merging it.
To avoid making commitments on behalf of others that cannot be fulfilled,
the `ready-for-merge` label should be applied by the assignee and not by another core maintainer.
If the pull request does not have an assignee, applying the `ready-for-merge` label implies self-assignment,
and this self-assignment may retroactively be made explicit by another core maintainer for tracking purposes.
Please be mindful that people are more likely to contribute again when they are thanked for their contribution.
An example acceptance message is as follows:

> This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the https://github.com/jenkinsci/jenkins/blob/master/docs/MAINTAINERS.adoc#merge-process[merge process documentation] for more information about the merge process. Thanks!

==== Rejection

Not all pull requests reach the point where they are ready for merge.
In some cases, the pull request is close to being ready, but one or more justified action items remain incomplete.
In other cases, negotiations regarding scope have reached an impasse.
In other cases, the pull request is very far from being ready or is completely impractical, and no progress is being made.
When a pull request is not ready for merge after an extended period of time,
the assignee is responsible for completing the rejection process,
first by applying the `stalled` label, then by applying the `proposed-for-closed` label, and finally by closing the pull request with a rejection message.
This process should be undertaken with the utmost care and respect
in order to ensure that the contributor feels welcome to contribute again.
At minimum, the reasoning behind the rejection should be stated in objective and factual terms.
If the pull request might be accepted again in the future once additional action item(s) have been completed,
these should be stated to allow for the original author or a different author to complete the pull request.
basil marked this conversation as resolved.
Show resolved Hide resolved
Please be mindful that people are more likely to contribute again when they are thanked for their contribution.
An example rejection message is as follows:

> I am closing this PR due to <insert reasoning here>. On behalf of the core team, I would like to thank you for your contribution. Even though this PR did not make it across the finish line, it was a promising start! I continue to encourage you (or anyone else who is interested) to pick up this effort and drive it to completion. Thanks!

=== Review non-goals

Code reviews do NOT pursue the following goals:
Expand Down Expand Up @@ -358,17 +500,9 @@ For these reasons, the following changes should not be merged during the week be
If the change is ready, but it is not a good time, consider labelling the pull request with the `on-hold` label.
Make sure to add a comment explaining why it was put on hold.

==== Step 3: Marking for merge

Once the checklist is passed, a Core PR Reviewer or a Maintainer can mark the pull request for merge.

* `ready-for-merge` label is set
* An explicit comment is added to the pull request so that other repository watchers are notified.
Example: _Thanks to all contributors! We consider this change as ready to be merged towards the next weekly release. It may be merged after 24hours if there is no negative feedback_

==== Step 4: Merge!
==== Step 3: Acceptance

A Core Maintainer merges the change after allowing sufficient time for comment (if needed).
Once the checklist is passed, the PR is eligible to begin xref:acceptance[the acceptance process].
After that, the change will be landed in the next weekly release.
LTS backporting, if needed, will be handled separately by the release team.

Expand Down