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: add guide to maintaining npm #16541

Closed
wants to merge 2 commits into from

Conversation

MylesBorins
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 27, 2017

## Step 1: Clone npm

$ git clone https://github.com/npm/npm.git
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we use the triple ` instead of the indentation? I think it's more common in our guides.

Copy link
Member

Choose a reason for hiding this comment

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

```console
$ git clone https://github.com/npm/npm.git
```

Would be nice


## Step 3: Rimraf old npm

$ cd ../node
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would add a comment explaining that the assumption is that the node repo is in the same directory of the npm repo.

Copy link
Member

Choose a reason for hiding this comment

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

Or just cd /path/to/node

$ git checkout vX.Y.Z
$ make release

## Step 3: Rimraf old npm
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

@MylesBorins
Copy link
Contributor Author

updated based on comments. Would still like to have a sign off from @iarna or @zkat before landing this

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2017
@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

Ping @iarna @zkat

@MylesBorins do you want to wait further?

Copy link
Member

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

@zkat zkat left a comment

Choose a reason for hiding this comment

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

Looks good! Couple of minor bits but otherwise 👍


```console
$ git remote update -p
$ git reset --hard origin master
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use master. This should be latest.


```console
$ tar zxf /path/to/npm/release/npm-x.y.z.tgz
$ git add -f npm
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be -A, to make sure files are both added and removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was doing it with -A it wasn't adding all the files. I believe add -f will add and remove.

Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

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

As of git 2.0, git add adds removed files as well as added files.

git add -f is about adding gitignored files, not sure we'd want that. @MylesBorins which files weren't being added?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if some files are missing, we should probably modify a rule in our .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated and opened #17363

$ ./tools/license-builder.sh
# The following commands are only neccessary if there are changes
$ git add .
$ git commit -m "doc: update npm LICENSE using license-builder.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth warning folks that this might end up including changes to -other- licenses, and that wouldn't be appropriate for this commit. They should make sure to check that any changes are actually npm-check related.


```console
$ git checkout vX.Y.Z
$ make release
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a note to run npm dist-tag ls npm and make sure this is the latest dist-tag. latest on git is usually released as next when it's time to downstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed below

$ ./configure
$ make -j4
$ ./tools/license-builder.sh
# The following commands are only neccessary if there are changes
Copy link
Contributor

Choose a reason for hiding this comment

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

sp: necessary

@MylesBorins
Copy link
Contributor Author

Updated with requested changes. PTAL

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

LGTM apart from the git add -f thing, which I'm not sure about.

$ git checkout master
$ git remote update -p
$ git reset --hard origin master
$ git checkout -b npm-x.y.z
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just

git remote update -p
git checkout -b npm-x.y.z origin/master

Copy link
Member

Choose a reason for hiding this comment

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

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Nov 26, 2017 via email

@MylesBorins
Copy link
Contributor Author

I've opened #17363 to help with the problem of files in the npm tree not being tracked by default

@gibfahn
Copy link
Member

gibfahn commented Nov 28, 2017

I think an npm update has gotten into the branch...

@MylesBorins
Copy link
Contributor Author

lol. This is what I get for trying to test things and multi tasking. I'll update when I get back to computer

@MylesBorins
Copy link
Contributor Author

@gibfahn ptal. This should be in working order now, I'll land in 24 hours if no one has any complaints. As this primarily mirrors the guide found on the npm wiki I don't think we need to block on the gitignore PR to land this

@gibfahn
Copy link
Member

gibfahn commented Nov 29, 2017

@MylesBorins went ahead and pushed my suggestion, remove it if you disagree (I'm assuming it simply got lost). LGTM.

This has 11 (!) approvals, and you've addressed all of @zkat's suggestions, so this should be good to land.

@mhdawson
Copy link
Member

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

👍

@MylesBorins
Copy link
Contributor Author

landed in f2eaf87
@zkat any interest in updating the npm wiki?

MylesBorins added a commit that referenced this pull request Nov 30, 2017
This is based on the guide found on their wiki

refs: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs

PR-URL: #16541
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2017
MylesBorins added a commit that referenced this pull request Dec 12, 2017
This is based on the guide found on their wiki

refs: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs

PR-URL: #16541
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins added a commit that referenced this pull request Dec 12, 2017
This is based on the guide found on their wiki

refs: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs

PR-URL: #16541
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
This is based on the guide found on their wiki

refs: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs

PR-URL: #16541
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
This is based on the guide found on their wiki

refs: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs

PR-URL: #16541
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
This is based on the guide found on their wiki

refs: https://github.com/npm/npm/wiki/CLI-Team-Process#submitting-the-new-latest-x-to-nodejs

PR-URL: #16541
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.