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

cli: don't check for updates to npm when we are updating npm itself #32

Merged

Conversation

@olore
Copy link
Contributor

@olore olore commented Jul 27, 2018

@olore olore requested a review from npm/cli-team as a code owner Jul 27, 2018
Copy link
Contributor

@zkat zkat left a comment

I think we need to do a bit more with this before it's mergeable. Thanks for taking the time to do this, though! It's super helpful to make sure this improves.

@@ -75,6 +75,7 @@
npm.load(conf, function (er) {
if (er) return errorHandler(er)
if (
!npm.argv.includes('npm') &&

This comment has been minimized.

@zkat

zkat Jul 30, 2018
Contributor

You'll note that pnpm also makes sure it's a relevant command, as well as whether we're in global mode. I think both of those check are worth doing.

I also agree with what pnpm is doing as far as disabling this on CI. The ci-info package by @watson seems to do a pretty good job at detecting this, so maybe that's worth pulling in (/cc @iarna).

This comment has been minimized.

@olore

olore Jul 31, 2018
Author Contributor

Thanks @zkat! I made updates to check install/update and if global mode.

Disregarding the CI stuff here based on comments on #33

@olore olore force-pushed the olore:invalid-notification-about-available-update branch from 7c90e10 to 692ce93 Jul 31, 2018
@olore olore force-pushed the olore:invalid-notification-about-available-update branch from 692ce93 to d905508 Jul 31, 2018
@olore
Copy link
Contributor Author

@olore olore commented Aug 1, 2018

@zkat I think we're all set. Let me know if anything else.

@zkat zkat force-pushed the npm:latest branch from 4f801d8 to 14bd214 Aug 2, 2018
@zkat zkat changed the base branch from latest to release-next Aug 3, 2018
@zkat
zkat approved these changes Aug 3, 2018
@zkat
Copy link
Contributor

@zkat zkat commented Aug 3, 2018

Looks great! Thanks. 🎉

@zkat zkat merged commit d811461 into npm:release-next Aug 3, 2018
2 checks passed
2 checks passed
@travis-ci
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zkat zkat removed the in-progress label Aug 3, 2018
@zkat zkat mentioned this pull request Aug 15, 2018
4 of 4 tasks complete
@zkat zkat mentioned this pull request Aug 29, 2018
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants