Send current project scope & CI status headers on request #14129
Conversation
d17d942
to
b53f7fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far!
var filteredEnv = extend({}, process.env) | ||
ciKeys.forEach(function (key) { delete filteredEnv[key] }) | ||
|
||
var conf = function (extraEnv) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really something – might this be something that can be abstracted out into common for use elsewhere? It seems pretty useful.
if (config.get('scope') === '') { | ||
config.set('scope', getProjectScope(npm.prefix)) | ||
} | ||
if (config.get('scope') !== '' && config.get('scope')[0] !== '@') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on moving this here.
040beea
to
5b4e3f3
Compare
b960147
to
267d7ef
Compare
@@ -21,6 +21,21 @@ The registry URL used is determined by the scope of the package (see | |||
supplied by the `registry` config parameter. See `npm-config(1)`, | |||
`npmrc(5)`, and `npm-config(7)` for more on managing npm's configuration. | |||
|
|||
## Does npm send any information about me back to the registry? | |||
|
|||
When making requests of the registry npm adds two headers with information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd start this with the word Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... of course! =D
continous integration environment, "false" otherwise. This is detected by | ||
looking for the following environment variables: `CI`, `TDDIUM`, | ||
`JENKINS_URL`, `bamboo.buildKey`. This is used to gather better metrics on | ||
how npm is used by humans, versus build farms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might say bots instead of build farm here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I would think of bots as something entirely different than CI…
@@ -21,6 +21,21 @@ The registry URL used is determined by the scope of the package (see | |||
supplied by the `registry` config parameter. See `npm-config(1)`, | |||
`npmrc(5)`, and `npm-config(7)` for more on managing npm's configuration. | |||
|
|||
## Does npm send any information about me back to the registry? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd also mention if the data sent is tied to username or package. i.e. is it "anonymized", not taken into account with any other data. also is the scope and the ci data going to be analyzed together or kept separate. people might worry about rate limiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can kind of see where your going with this but I'm at a loss as to what to do exactly?
"data sent is tied to username or package" I don't really know what you mean by "tied"? If you're authed then your authentication info can be traced back to a username, and of course the package name you're installing is the primary bit of the request…
This seems to get into the what the registry will do with this information. (The answer right now is "ignore it".)
As for what we might do with it? What do we need to say beyond the tentative descriptions I've given below?
Something like? "We choose not to correlate the information in these headers with any authenticated accounts you're using."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that last line sounds super good.
organization. | ||
* `Npm-In-CI` – Set to "true" if npm believes this install is running in a | ||
continous integration environment, "false" otherwise. This is detected by | ||
looking for the following environment variables: `CI`, `TDDIUM`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe link to the code that does this? i imagine people will want to argue technically about how this is incomplete and/or not accurate so it may be helpful to tell them where a PR would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
842df62
to
df78055
Compare
@@ -21,6 +21,21 @@ The registry URL used is determined by the scope of the package (see | |||
supplied by the `registry` config parameter. See `npm-config(1)`, | |||
`npmrc(5)`, and `npm-config(7)` for more on managing npm's configuration. | |||
|
|||
## Does npm send any information about me back to the registry? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that last line sounds super good.
organization. | ||
* `Npm-In-CI` – Set to "true" if npm believes this install is running in a | ||
continous integration environment, "false" otherwise. This is detected by | ||
looking for the following environment variables: `CI`, `TDDIUM`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
df78055
to
ac20b31
Compare
Credit: @iarna PR-URL: npm/npm-registry-client#129 PR-URL: npm/npm-registry-client#145
Credit: @iarna PR-URL: #14129 Reviewed-By: @ashleygwilliams
ac20b31
to
4420cf8
Compare
Credit: @iarna PR-URL: #14129 Reviewed-By: @ashleygwilliams
This has been merged into |
TODO: