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 run update-notifier in CI environment #33

Closed
wants to merge 4 commits into from
Closed

cli: don't run update-notifier in CI environment #33

wants to merge 4 commits into from

Conversation

sibiraj-s
Copy link

This Prevents update notifier from running in CI environment

@sibiraj-s sibiraj-s requested a review from a team as a code owner July 28, 2018 07:16
@olore
Copy link
Contributor

olore commented Jul 28, 2018

Nice one @sibiraj-s. This and #32 will make the update-notifier work the same as pnpm

@zkat
Copy link
Contributor

zkat commented Jul 30, 2018

Ahh, I commented on #32 before seeing this PR. @olore feel free to ignore my comments related to CI itself.

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.

So far so good! Thanks a bunch for submitting this PR, and welcome! Just a couple of changes and I think this is ready to go. 🎉

bin/npm-cli.js Outdated
@@ -80,9 +80,10 @@
) {
const pkg = require('../package.json')
let notifier = require('update-notifier')({pkg})
const isCI = require('is-ci')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather use ci-info, since it's what is-ci itself uses. We don't need yet another package.json floating around for a one-liner by the same author. ci-info exports an isCI property that you can achieve the same thing with.

Copy link
Author

Choose a reason for hiding this comment

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

@zkat . It seems is-ci is already a part of this package(a dependant of update-notifier). having that now won't make a difference?

bin/npm-cli.js Outdated
if (
notifier.update &&
notifier.update.latest !== pkg.version
notifier.update.latest !== pkg.version && !isCI
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mind putting this on the next line for consistency?

package.json Outdated
@@ -177,6 +178,7 @@
"inherits",
"ini",
"init-package-json",
"is-ci",
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! You added it to bundleDependencies. Additionally, we commit our dependencies, and we use package-lock.json so please go ahead and git add -A node_modules/ package-lock.json as well before you commit the ci-info change I asked for above!

@zkat zkat added semver:minor new backwards-compatible feature in-progress labels Jul 30, 2018
@@ -80,9 +80,11 @@
) {
const pkg = require('../package.json')
let notifier = require('update-notifier')({pkg})
const isCI = require('ci-info').isCI
Copy link
Contributor

@iarna iarna Jul 31, 2018

Choose a reason for hiding this comment

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

We already detect CI in a handful of places and currently ci-info doesn't entirely align. Then again, our various detectors don't align with each other. Ideally, I'd like to see those patched to use ci-info as well (but not in this PR).

For reference, our existing CI detectors are here:

'progress': !process.env.TRAVIS && !process.env.CI,

and here:

https://github.com/npm/npm-registry-fetch/blob/03dde52817d00996d8316b155832d8802f338236/config.js#L26

The latter one detects things ci-info doesn't, so ci-info will need patching before we can entirely switch over to it.

There's also:

https://github.com/npm/npm-registry-client/blob/d70399bff938b04cd3ed7052f415c6ed2d6bd275/lib/initialize.js#L15

Which should probably be patched too, though we aren't/won't be using it any more in npm proper.

Copy link

Choose a reason for hiding this comment

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

BTW: The is-ci module is just a convenience wrapper around ci-info, that when used programmatically returns a boolean and when used from the command line uses the exit code to signal if it's being run on a CI server or not.

package.json Outdated
@@ -45,6 +45,7 @@
"cacache": "^11.0.2",
"call-limit": "~1.1.0",
"chownr": "~1.0.1",
"ci-info": "^1.1.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still no package-lock.json change in this PR? We'll need that to land this.

Copy link
Author

Choose a reason for hiding this comment

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

@iarna I got it. The ci-info is already used as a dependecy of update-notifier

update-notifier -> is-ci -> ci-info

Copy link
Contributor

Choose a reason for hiding this comment

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

ohhhhh

that makes sense.

@sibiraj-s
Copy link
Author

@iarna . I raised a PR in ci-info for detecting solano-ci. which is the only ci which is not detected by ci-info and used here.

watson/ci-info#9

@zkat
Copy link
Contributor

zkat commented Aug 13, 2018

Once @watson gets around to reviewing/approving the ci-info changes, I think we'll be ready to merge this. So I figure I'll ping him 😅

@zkat
Copy link
Contributor

zkat commented Aug 13, 2018

As a note for @iarna: I think it's fine and pretty much safe that ci-info uses different env var keys for Bamboo. If one is present, the other one is also present. And if you're being that clever about tricking npm into thinking you're in bamboo, then I dunno what to tell ya. I think it's fine to accept this once ci-info gets the solano patch. Everything else seems to be the same.

Additional note to self: I should probably patch npm-registry-fetch to use this as well. Maybe pacote if this gets merged before we upgrade pacote to the n-r-f version.

@watson
Copy link

watson commented Aug 14, 2018

@zkat Thanks a lot of pinging me on this. I'm so bad at checking github notifications - have a huge backlog 😬

@sibiraj-s
Copy link
Author

sibiraj-s commented Aug 14, 2018

ci-info updated to v1.3.1. detects solano-ci

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! Thank you @watson and @sibiraj-s!

zkat added a commit that referenced this pull request Aug 20, 2018
PR-URL: #33
Credit: @sibiraj-s
Reviewed-By: @zkat
zkat added a commit that referenced this pull request Aug 20, 2018
@zkat
Copy link
Contributor

zkat commented Aug 20, 2018

I merged this manually, so github isn't gonna detect it as a merge, but this is now in release-next: cabc0f0

@zkat zkat closed this Aug 20, 2018
zkat added a commit that referenced this pull request Aug 20, 2018
PR-URL: #33
Credit: @sibiraj-s
Reviewed-By: @zkat
zkat added a commit that referenced this pull request Aug 20, 2018
@sibiraj-s sibiraj-s deleted the ci-skip-update-notifier branch August 21, 2018 05:19
@sibiraj-s
Copy link
Author

cool. @zkat I would also suggest to update ci-info to v1.4 before the release, as it detects two more popular CI's

mislav added a commit to mislav/npm-cli that referenced this pull request Sep 3, 2018
This is a followup to npm#33 that changes the logic to literally *not* run update-notifier
in CI environment, instead of running it and omitting the result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants