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: literally don't run update-notifier in CI environment #66

Closed
wants to merge 1 commit into from

Conversation

mislav
Copy link

@mislav mislav commented Sep 3, 2018

This is a followup to #33 that changes the logic to literally not run update-notifier in CI environment, instead of running it and omitting the result.

Checking environment variables is a “cheap” check and, as such, should ideally come before other, potentially more expensive checks such as those that hit the filesystem or the network.

/cc @sibiraj-s @zkat

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.
@mislav mislav requested a review from a team as a code owner September 3, 2018 13:26
@sholladay
Copy link

update-notifier already disables itself in CI automatically:
https://github.com/yeoman/update-notifier/blob/d3718349e5c541face4a37c80feebb505de31b7c/index.js#L40-L42

@mislav
Copy link
Author

mislav commented Oct 1, 2018

Thank you! In that case, the check in npm should be removed entirely, instead of having it in the wrong place?

@sibiraj-s
Copy link

update-notifier already disables itself in CI automatically

update-notifier caches the data and updates itself when called the next time. this behaviour is inconstant(may be because of is-ci). somehow it still escapes. thats why i added a check here

instead of having it in the wrong place?

Yes. I could have added before initiating update notifier.

but, before any discussion check #61. I think this PR can be closed as this is already taken care of https://github.com/npm/cli/pull/61/files#diff-7e8e97e11751b950f7b30f4c8e969207R32

@mislav
Copy link
Author

mislav commented Oct 29, 2018

@sibiraj-s Yes, it looks like #61 supersedes this PR. This can be closed once that is merged.

@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

I'm closing this one in favor of #61.

@zkat zkat closed this Nov 13, 2018
@mislav mislav deleted the patch-1 branch August 2, 2021 09:31
antongolub pushed a commit to antongolub-forks/npm-cli that referenced this pull request May 18, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants