doc: numerous edits #11
Conversation
Numerous edits based on feedback received thus far. 1. Clarifications to sign-off rule 2. Remove review-required 3. Structure improvements 4. Addressing security related commits and exceptions to process and versioning rules for security related commits 5. Suggested LTS policy (suggested by jasnell) 6. Other minor edits
@Fishrock123's wording is better.
| @@ -103,7 +101,7 @@ Before a new major release master is branched for LTS releases of the prior majo | |||
|
|
|||
| Additionally there are branches for stable release lines prior to 1.0 of minor versions. Example: `0.8.x`, `0.10.x`, `0.12.x`. | |||
|
|
|||
| All release branches other than master are managed by the LTS Working Group and fall under their LTS release policy. | |||
| All branches other than master are managed by the LTS Working Group and fall under the LTS release policy. | |||
There was a problem hiding this comment.
We could have feature branches or branches that are trying unstable versions of v8, and those wouldn't be handled by LTS, that's why this was originally "release" branches.
There was a problem hiding this comment.
Good point. I'll revert that wording.
Per @mikeals comment
|
|
||
| Specific Collaborators can be requested to review any PR by using an *@-mention* either within the PR text itself or the associated comment stream. | ||
| If any Collaborator feels that a particular PR is controversial and needs to be reviewed further, the Collaborator can call attention to the Pull Request by attaching either the *tsc-agenda* tag (to put the Pull Request on the TSC agenda) or a Working Group specific tag to indicate that the PR requires review from a specific Working Group. PR’s that are put onto the TSC or WG agenda cannot be landed until the tagged WG or TSC has the opportunity to review and decide using the Consensus Seeking Model. |
There was a problem hiding this comment.
tsc-agenda should usually be a last resort. it actually draws attention away.
|
Looking better to me! :D |
|
|
||
| Before any Pull Request can be accepted into the project, it must be signed-off by at least two Collaborators (the submitting / sponsoring Collaborator and one additional Collaborator with sufficient expertise to evaluate the specific PR). | ||
| Pull Requests from an existing Collaborator must be signed-off on by at least one other Collaborator with sufficient expertise to evaluate the specific PR. Collaborators are encouraged to seek additional review and sign-off for non-trivial changes. A Trivial Change is any which fixes minor bugs or improves performance without affecting API or causing other wide-reaching impact. |
There was a problem hiding this comment.
I would not consider the following comment a blocker: my experience with "hard requirement"-style signoff rules – ones that don't allow for circumvention when necessary – is that commits that need to be made speedily (for whatever reason) are blocked unnecessarily and undue stress is added to the process.
This is better phrased as "should" instead of "must," where "should" is taken to mean "9 times out of 10, you should get signoff." The implication is that if you're not getting sign-off, you're assuming full liability for the changeset. If this is abused, you can have your commit bit removed. IOW: it's zero-risk to get a changeset signed-off on, but takes time; whereas for situations that require expediency the risk is higher but the delay is shorter.
There was a problem hiding this comment.
How many commits has io.js taken without sign-off? I can only remember one.
There was a problem hiding this comment.
Basically only release commits. (Which, by the way, are the exception.)
I think Trevor commited a fix once.. That's about it. EDIT: There's also a test by Rod for the fd leak.
The thing is, even on io.js the release process isn't so fast as to necessitate this. If anyone really needs a patch within that time, they can just patch it themselves.
Required +1 review on potential commits has caught at least a few things in io.js. (It has also missed others, no review is 100% perfect.)
There was a problem hiding this comment.
Does the risk of an unreviewed commit outweigh the risk of a delayed
commit? Moving fast isn't always worth the liability.
On Apr 5, 2015 5:16 PM, "Jeremiah Senkpiel" [email protected]
wrote:
In README.md
#11 (comment):-Before any Pull Request can be accepted into the project, it must be signed-off by at least two Collaborators (the submitting / sponsoring Collaborator and one additional Collaborator with sufficient expertise to evaluate the specific PR).
+Pull Requests from an existing Collaborator must be signed-off on by at least one other Collaborator with sufficient expertise to evaluate the specific PR. Collaborators are encouraged to seek additional review and sign-off for non-trivial changes. A Trivial Change is any which fixes minor bugs or improves performance without affecting API or causing other wide-reaching impact.Basically only release commits. (Which, by the way, are the exception.)
I think Trevor commited a fix once.. That's about it.
The thing is, even on io.js the release process isn't so fast as to
necessitate this. If anyone really needs a patch within that time, they
can just patch it themselves.Required +1 review on potential commits has caught at least a few things
in io.js. (It has also missed others, no review is 100% perfect.)—
Reply to this email directly or view it on GitHub
https://github.com/jasnell/dev-policy/pull/11/files#r27781751.
There was a problem hiding this comment.
Note also that force pushes to fix commit metadata happen without +1.
Required +1 review on potential commits has caught at least a few things in io.js.
I would ascribe that to review, not to required review.
There was a problem hiding this comment.
9 times out of 10 review is the best way to go. However, humans being humans, there will always be exceptions to this rule – I err towards encoding that understanding into the document. That way we can point to the document solely, instead of having a document plus some oral history about when it's okay to bend or break the rule.
There was a problem hiding this comment.
That way we can point to the document solely, instead of having a document plus some oral history about when it's okay to bend or break the rule.
I agree with this entirely, that's actually why I'm suggesting that we just remove the "trivial" language and change it to "release commits." As it stands I think we actually culturally enforce a policy of review for even trivial changes, the one Trevor landed was pretty trivial and still lead to Ben and Fedor stepping in saying that everything should be reviewed.
There was a problem hiding this comment.
Ah, yes. Addressing the exceptions explicitly (release commits, fixing metadata) alleviates my concerns.
Additional cleanup and restructuring based on the ongoing conversation.
|
Additional changes made. Going to land this but please keep the comments coming. This has been very helpful |
Numerous edits based on feedback received thus far.
versioning rules for security related commits