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

deps: move from @npmcli/ci-detect to ci-info #5858

Merged
merged 1 commit into from
Nov 16, 2022
Merged

deps: move from @npmcli/ci-detect to ci-info #5858

merged 1 commit into from
Nov 16, 2022

Conversation

wraithgar
Copy link
Member

See watson/ci-info#95 for more context on
achieving parity between these two modules.

This changes npm to use ci-info instead of @npmcli/ci-detect.
Everything that npm currently flags as a CI environment should still be
doing so, so there is no breaking change there.

There is going to be a subtle difference in the ci-name config, which
nothing in npm currently looks at anyways, as well as the ci name that
shows up in the default user-agent string. Some providers will be
slightly different (i.e. circle-ci vs circleci and cirrus vs cirrus-ci)

@wraithgar wraithgar requested a review from a team as a code owner November 15, 2022 14:37
See watson/ci-info#95 for more context on
achieving parity between these two modules.

This changes npm to use `ci-info` instead of `@npmcli/ci-detect`.
Everything that npm currently flags as a CI environment should still be
doing so, so there is no breaking change there.

There is going to be a subtle difference in the `ci-name` config, which
nothing in npm currently looks at anyways, as well as the ci name that
shows up in the default `user-agent` string.  Some providers will be
slightly different (i.e. circle-ci vs circleci and cirrus vs cirrus-ci)
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Nov 15, 2022

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 49.097 ±4.68 26.174 ±0.17 34.313 ±15.05 27.303 ±1.38 4.005 ±0.12 4.252 ±0.10 3.208 ±0.03 16.655 ±0.12 3.248 ±0.01 6.096 ±0.62
#5858 47.700 ±3.60 25.020 ±0.15 35.125 ±16.43 26.618 ±0.39 3.984 ±0.03 4.174 ±0.06 3.260 ±0.05 16.772 ±0.23 3.155 ±0.01 6.374 ±2.25
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 35.200 ±0.72 19.482 ±0.04 18.012 ±0.07 19.253 ±0.82 3.779 ±0.10 3.608 ±0.03 3.331 ±0.08 12.147 ±0.03 3.116 ±0.05 4.316 ±0.03
#5858 33.230 ±1.13 19.117 ±0.32 17.760 ±0.18 18.918 ±0.72 3.654 ±0.10 3.698 ±0.10 3.282 ±0.08 12.121 ±0.04 3.015 ±0.07 4.095 ±0.03

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

one very minor nit you can take or leave, but this looks good otherwise

lib/utils/config/definitions.js Show resolved Hide resolved
@wraithgar wraithgar merged commit a351685 into latest Nov 16, 2022
@wraithgar wraithgar deleted the gar/ci-info branch November 16, 2022 18:41
@github-actions github-actions bot mentioned this pull request Nov 16, 2022
@@ -442,7 +441,7 @@ define('ci-name', {
description: `
The name of a continuous integration system. If not set explicitly, npm
will detect the current CI environment using the
[\`@npmcli/ci-detect\`](https://npm.im/@npmcli/ci-detect) module.
[\`ci-info\`](https://npm.im/@npmcli/ci-info) module.

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

fiddlesticks. search and replace failed me, sorry about that.

this file is the source of truth, changing it and running snapshots will fix the rest.

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