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.

Sign up for GitHub

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

Jump to bottom

meta: clarify process for breaking changes #7955

Closed
wants to merge 2 commits into from
Closed

meta: clarify process for breaking changes #7955

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 3, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

meta

Description of change

Fixes: #7848

@Trott Trott added the meta Issues and PRs related to the general management of the project. label Aug 3, 2016
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 3, 2016
@Trott
Copy link
Member Author

Trott commented Aug 3, 2016

/cc @nodejs/ctc

@Trott Trott mentioned this pull request Aug 3, 2016
Closed
@jasnell
Copy link
Member

jasnell commented Aug 3, 2016

Lgtm

yorkie
yorkie reviewed Aug 3, 2016
View reviewed changes
COLLABORATOR_GUIDE.md
Breaking changes (that is, pull requests that require an increase in the
major version number, known as `semver-major` changes) must be elevated for
review by the CTC. This does not necessarily mean that the PR must be put onto
the CTC meeting agenda. If multiple CTC members approve (`LGTM`) the PR and no
Copy link
Contributor

@yorkie yorkie Aug 3, 2016 •
edited
Loading

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"?

Copy link
Member Author

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
Copy link
Member

ChALkeR commented Aug 3, 2016

The main part LGTM, but I have a question about «trivial changes» (commented inline).
/cc @bnoordhuis, perhaps.

@bnoordhuis
Copy link
Member

(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
Copy link
Contributor

mscdex commented Aug 3, 2016 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member Author

Trott commented Aug 3, 2016

I've removed the change to the "trival changes" description in the paragraph about the 48/72 hour rule. PTAL. So far, we've got LGTMs from @jasnell and @ChALkeR but no one else...

@misterdjules
Copy link

LGTM.

@addaleax
Copy link
Member

addaleax commented Aug 3, 2016

LGTM

1 similar comment
@mscdex
Copy link
Contributor

mscdex commented Aug 3, 2016

LGTM

@jasnell jasnell mentioned this pull request Aug 3, 2016
Closed
2 tasks
@jasnell
Copy link
Member

jasnell commented Aug 3, 2016

Thinking out loud: it might make sense to introduce new ctc-review and tsc-review labels for issues that (a) currently require review but have not yet been explicitly added to the meeting agenda.

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2016

LGTM

@Trott
Copy link
Member Author

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.

Trott added a commit to Trott/io.js that referenced this pull request Aug 5, 2016
@Trott
meta: clarify process for breaking changes
e3e3588 
Fixes: nodejs#7848
PR-URL: nodejs#7955
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member Author

Trott commented Aug 5, 2016

Landed in e3e3588

@Trott Trott closed this Aug 5, 2016
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@Trott @cjihrig
meta: clarify process for breaking changes
5cc4b0e 
Fixes: #7848
PR-URL: #7955
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Sep 9, 2016
@Trott
meta: clarify process for breaking changes
52be1d3 
Fixes: #7848
PR-URL: #7955
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
@Trott
meta: clarify process for breaking changes
d21db24 
Fixes: #7848
PR-URL: #7955
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@Trott @rvagg
meta: clarify process for breaking changes
3d3cb78 
Fixes: #7848
PR-URL: #7955
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@Trott @MylesBorins
meta: clarify process for breaking changes
9d9bcd7 
Fixes: #7848
PR-URL: #7955
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Closed
@Trott Trott mentioned this pull request Apr 12, 2018
Closed
3 tasks
@Trott Trott deleted the major branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Role of CTC in semver-major changes needs clarification