Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
meta: clarify process for breaking changes#7955
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott commented Aug 3, 2016
/cc @nodejs/ctc |
jasnell commented Aug 3, 2016
Lgtm |
COLLABORATOR_GUIDE.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the "multiple" mean ">= 2 CTC members"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. More than one.
ChALkeR commented Aug 3, 2016
The main part LGTM, but I have a question about «trivial changes» (commented inline). |
bnoordhuis commented Aug 3, 2016
(A priori assumption: small incremental changes > big non-incremental changes.) I think some pragmatism around the 48/72 hour rule is necessary because it discourages small incremental changes. It becomes much more attractive to pile up everything in big potpourri pull requests - something I've been guilty of - but that makes code reviews and release management more difficult. I wouldn't mind dropping the 48 hour rule. If one or two maintainers sign off on a pull request and the CI is green, it should be good to go, right? |
mscdex commented Aug 3, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
IMHO I'd prefer to keep the 48/72 hour rule. It gives interested parties a chance to chime in on changes that they may feel strongly about. I also recall instances where code changes were landed too quickly (talking non-security issues here) and those changes ended up causing problems of varying degrees... |
evanlucas commented Aug 3, 2016
Yea, IIRC we implemented that rule because changes were landing too fast. I'm +1 on keeping the rule, with the exception of doc changes (and maybe changes that have broken CI or something that needs a quick revert?). It's hard, because if we have the rule, and the rule has exceptions, those should be perfectly clear. |
jasnell commented Aug 3, 2016
I'm in favor of keeping the 48/72 rule, if for no other reason than it provides time for our global group of contributors to catch up and properly review the changes. |
Trott commented Aug 3, 2016
misterdjules commented Aug 3, 2016
LGTM. |
addaleax commented Aug 3, 2016
LGTM |
1 similar comment
mscdex commented Aug 3, 2016
LGTM |
jasnell commented Aug 3, 2016
Thinking out loud: it might make sense to introduce new |
mhdawson commented Aug 3, 2016
LGTM |
Trott commented Aug 4, 2016
There seems to be consensus, so last call for objections! I'll land in approximately 12 hours or so unless someone swoops in to say this is terrible or something. |
Fixes: nodejs#7848 PR-URL: nodejs#7955 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Trott commented Aug 5, 2016
Landed in e3e3588 |
Fixes: #7848 PR-URL: #7955 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Fixes: #7848 PR-URL: #7955 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Fixes: #7848 PR-URL: #7955 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Fixes: #7848 PR-URL: #7955 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Fixes: #7848 PR-URL: #7955 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Checklist
Affected core subsystem(s)
meta
Description of change
Fixes: #7848