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

doc,meta: reduce npm PR wait period to one week #29922

Merged
merged 0 commits into from Oct 12, 2019

Conversation

@Trott
Copy link
Member

Trott commented Oct 10, 2019

The two-week wait period for merging npm releases is one of those rule
exceptions that would be great to get rid of (in my opinion at least).
There are too many exceptions to our rules and they tend to be scattered
across multiple documents. People don't feel confident they know the
rules, thus hampering both project velocity and Collaborator confidence.
It also means I (and perhaps others?) get lots of pings about whether
this or that can land, etc.

This particular issue has come up a few times lately, and is
specifically calling for an exception-to-the-exception so that the
latest version of npm can be released along with Node.js 13.0.0.
Refs: #29885 (comment)

I propose here reducing the wait period from two weeks to one week. If,
after some amount of time, there seems to be no problems caused by this
change, we can consider further reducing the wait period to 48 hours to
align it with all other change requests. Even if you think that is going
too far, hopefully we can at least get it reduced to a week, as the
second week of the waiting period is usually just the PR sitting around
with an occasional ping from someone about whether/when it can land.

@nodejs/npm @nodejs/tsc

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Oct 10, 2019

Writing an unnecessarily-detailed three-paragraph explanation/justification for a one-line change is very on-brand for me, I think.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Oct 10, 2019

Copy link
Member

addaleax left a comment

I’d also be perfectly fine with dropping the waiting time.

@lpinca
lpinca approved these changes Oct 10, 2019
@Trott Trott added the author ready label Oct 10, 2019
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Oct 10, 2019

Why do we wait 2 weeks? Is it to see if the people updating npm via the npm cli find that the release is unstable?

I'm wondering why npm doesn't use the same rules as every PR.

If we discover we shipped an unstable npm, we would treat it like any other bug. I'm not sure it needs any specialness at all, though I defer to past experience. But not too past... if an npm update caused an issue once 2 years ago... well, other things have also caused issues.

@targos
targos approved these changes Oct 10, 2019
@targos

This comment has been minimized.

Copy link
Member

targos commented Oct 10, 2019

If we discover we shipped an unstable npm, we would treat it like any other bug.

+1, I'm also in favor of dropping the exception.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Oct 10, 2019

Why do we wait 2 weeks? Is it to see if the people updating npm via the npm cli find that the release is unstable?

I'm wondering why npm doesn't use the same rules as every PR.

I don't remember the exact details. I have a vague recollection that #21594 was a response to something but it doesn't document what it was and my memory doesn't recall the details.

If we discover we shipped an unstable npm, we would treat it like any other bug. I'm not sure it needs any specialness at all, though I defer to past experience. But not too past... if an npm update caused an issue once 2 years ago... well, other things have also caused issues.

The current wait period in the policy only applies to landing on master ("The specific Node.js release streams the new version will be able to land into are at the discretion of the release and LTS teams.").

I'm also in favour of dropping the exception. The risk of an unstable npm making it into an LTS release should be mitigated by the existing separate LTS policy for changes (in general) to live on a current release for two weeks.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Oct 10, 2019

I reduced to one week rather than eliminating the requirement entirely because I thought there's be resistance to eliminating it entirely. I guess maybe I over-estimated the resistance that would meet.

Copy link
Member

mcollina left a comment

LGTM

Copy link
Member

sam-github left a comment

People seemed pretty agreeable to removing the exception entirely, but I'm fine with this, too.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Oct 12, 2019

I’d be ok with no wait for patch and minor releases, and 1 or 2 weeks for major releases (no coordinated launch).

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Oct 12, 2019

Just to answer the question, the rule was put in place because there were a series of issues where npm itself was broken or the updates broke node.js in some way. If we're confident that there are improved QA checks in place now, then +1 to reducing the rule to 1 week

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Oct 12, 2019

Landed in 02b3722

@Trott Trott closed this Oct 12, 2019
@Trott Trott force-pushed the Trott:npm-wait-period-reduction branch from d8d600b to 02b3722 Oct 12, 2019
@Trott Trott merged commit 02b3722 into nodejs:master Oct 12, 2019
@Trott Trott mentioned this pull request Nov 8, 2019
3 of 3 tasks complete
targos added a commit that referenced this pull request Nov 8, 2019
The two-week wait period for merging npm releases is one of those rule
exceptions that would be great to get rid of (in my opinion at least).
There are too many exceptions to our rules and they tend to be scattered
across multiple documents. People don't feel confident they know the
rules, thus hampering both project velocity and Collaborator confidence.
It also means I (and perhaps others?) get lots of pings about whether
this or that can land, etc.

This particular issue has come up a few times lately, and is
specifically calling for an exception-to-the-exception so that the
latest version of npm can be released along with Node.js 13.0.0.
Refs: #29885 (comment)

I propose here reducing the wait period from two weeks to one week. If,
after some amount of time, there seems to be no problems caused by this
change, we can consider further reducing the wait period to 48 hours to
align it with all other change requests. Even if you think that is going
too far, hopefully we can at least get it reduced to a week, as the
second week of the waiting period is usually just the PR sitting around
with an occasional ping from someone about whether/when it can land.

PR-URL: #29922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
The two-week wait period for merging npm releases is one of those rule
exceptions that would be great to get rid of (in my opinion at least).
There are too many exceptions to our rules and they tend to be scattered
across multiple documents. People don't feel confident they know the
rules, thus hampering both project velocity and Collaborator confidence.
It also means I (and perhaps others?) get lots of pings about whether
this or that can land, etc.

This particular issue has come up a few times lately, and is
specifically calling for an exception-to-the-exception so that the
latest version of npm can be released along with Node.js 13.0.0.
Refs: #29885 (comment)

I propose here reducing the wait period from two weeks to one week. If,
after some amount of time, there seems to be no problems caused by this
change, we can consider further reducing the wait period to 48 hours to
align it with all other change requests. Even if you think that is going
too far, hopefully we can at least get it reduced to a week, as the
second week of the waiting period is usually just the PR sitting around
with an occasional ping from someone about whether/when it can land.

PR-URL: #29922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.