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

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 10, 2019
@Trott
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
Copy link
Member Author

Trott commented Oct 10, 2019

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 10, 2019
@sam-github
Copy link
Contributor

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

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
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 mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

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

@mcollina
Copy link
Member

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

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

Trott commented Oct 12, 2019

Landed in 02b3722

@Trott Trott closed this Oct 12, 2019
@Trott Trott merged commit 02b3722 into nodejs:master Oct 12, 2019
targos pushed 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 pushed 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>
@Trott Trott deleted the npm-wait-period-reduction branch January 13, 2022 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet