Skip to content

Commit

Permalink
fix(config): user-agent properly shows ci
Browse files Browse the repository at this point in the history
The way we were flattening user-agent back into itself meant that any
values that were dependent on other config items would never be seen,
since we have to re-flatten the item for each one it depends on.

We also weren't re-flattening the user-agent when setting workspaces or
workspace, which were things that affected the final result.

This does change the main config value of `user-agent` but not the
flattened one.  We are not using the main config value anywhere (which
is correct).

PR-URL: #3754
Credit: @wraithgar
Close: #3754
Reviewed-by: @nlf
  • Loading branch information
wraithgar committed Sep 14, 2021
1 parent 291d977 commit b4aac34
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 9 deletions.
12 changes: 11 additions & 1 deletion lib/utils/config/definitions.js
Original file line number Diff line number Diff line change
Expand Up @@ -2053,10 +2053,14 @@ define('user-agent', {
.replace(/\{workspaces\}/gi, inWorkspaces)
.replace(/\{ci\}/gi, ciName ? `ci/${ciName}` : '')
.trim()

// We can't clobber the original or else subsequent flattening will fail
// (i.e. when we change the underlying config values)
// obj[key] = flatOptions.userAgent

// user-agent is a unique kind of config item that gets set from a template
// and ends up translated. Because of this, the normal "should we set this
// to process.env also doesn't work
obj[key] = flatOptions.userAgent
process.env.npm_config_user_agent = flatOptions.userAgent
},
})
Expand Down Expand Up @@ -2140,6 +2144,9 @@ define('workspace', {
a workspace which does not yet exist, to create the folder and set it
up as a brand new workspace within the project.
`,
flatten: (key, obj, flatOptions) => {
definitions['user-agent'].flatten('user-agent', obj, flatOptions)
},
})

define('workspaces', {
Expand All @@ -2151,6 +2158,9 @@ define('workspaces', {
Enable running a command in the context of **all** the configured
workspaces.
`,
flatten: (key, obj, flatOptions) => {
definitions['user-agent'].flatten('user-agent', obj, flatOptions)
},
})

define('yes', {
Expand Down
40 changes: 39 additions & 1 deletion smoke-tests/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
const fs = require('fs')
const { promisify } = require('util')
const execAsync = promisify(require('child_process').exec)
const { resolve } = require('path')
const { join, resolve } = require('path')
const t = require('tap')
const rimraf = promisify(require('rimraf'))

const normalizePath = path => path.replace(/[A-Z]:/, '').replace(/\\/g, '/')
const cwd = normalizePath(process.cwd())
Expand Down Expand Up @@ -47,6 +48,43 @@ const exec = async cmd => {
const readFile = filename =>
String(fs.readFileSync(resolve(localPrefix, filename)))

// this test must come first, its package.json will be destroyed and the one
// created in the next test (npm init) will create a new one that must be
// present for later tests
t.test('npm install sends correct user-agent', async t => {
const pkgPath = join(localPrefix, 'package.json')
const pkgContent = JSON.stringify({
name: 'smoke-test-workspaces',
workspaces: ['packages/*'],
})
fs.writeFileSync(pkgPath, pkgContent, { encoding: 'utf8' })

const wsRoot = join(localPrefix, 'packages')
fs.mkdirSync(wsRoot)

const wsPath = join(wsRoot, 'foo')
fs.mkdirSync(wsPath)

const wsPkgPath = join(wsPath, 'package.json')
const wsContent = JSON.stringify({
name: 'foo',
})
fs.writeFileSync(wsPkgPath, wsContent, { encoding: 'utf8' })
t.teardown(async () => {
await rimraf(`${localPrefix}/*`)
})

const cmd = `${npmBin} install fail_reflect_user_agent`
await t.rejects(exec(cmd), {
stderr: /workspaces\/false/,
}, 'workspaces/false is present in output')

const wsCmd = `${npmBin} install fail_reflect_user_agent --workspaces`
await t.rejects(exec(wsCmd), {
stderr: /workspaces\/true/,
}, 'workspaces/true is present in output')
})

t.test('npm init', async t => {
const cmd = `${npmBin} init -y`
const cmdRes = await exec(cmd)
Expand Down
8 changes: 7 additions & 1 deletion smoke-tests/server.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* istanbul ignore file */
const {join, dirname} = require('path')
const {join, dirname, basename} = require('path')
const {existsSync, readFileSync, writeFileSync} = require('fs')
const PORT = 12345 + (+process.env.TAP_CHILD_ID || 0)
const http = require('http')
Expand Down Expand Up @@ -150,6 +150,12 @@ const startServer = () => new Promise((res, rej) => {
}

const f = join(__dirname, 'content', join('/', req.url.replace(/@/, '').replace(/%2f/i, '/')))
// a magic package that causes us to return an error that will be logged
if (basename(f) === 'fail_reflect_user_agent') {
res.setHeader('npm-notice', req.headers['user-agent'])
res.writeHead(404)
return res.end()
}
const isCorgi = req.headers.accept.includes('application/vnd.npm.install-v1+json')
const file = f + (
isCorgi && existsSync(`${f}.min.json`) ? '.min.json'
Expand Down
4 changes: 2 additions & 2 deletions tap-snapshots/test/lib/config.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ exports[`test/lib/config.js TAP config list --json > output matches snapshot 1`]
"unicode": false,
"update-notifier": true,
"usage": false,
"user-agent": "npm/{NPM-VERSION} node/{NODE-VERSION} {PLATFORM} {ARCH} workspaces/false",
"user-agent": "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}",
"version": false,
"versions": false,
"viewer": "{VIEWER}",
Expand Down Expand Up @@ -296,7 +296,7 @@ umask = 0
unicode = false
update-notifier = true
usage = false
user-agent = "npm/{NPM-VERSION} node/{NODE-VERSION} {PLATFORM} {ARCH} workspaces/false"
user-agent = "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}"
; userconfig = "{HOME}/.npmrc" ; overridden by cli
version = false
versions = false
Expand Down
30 changes: 26 additions & 4 deletions test/lib/utils/config/definitions.js
Original file line number Diff line number Diff line change
Expand Up @@ -747,15 +747,15 @@ t.test('user-agent', t => {
definitions['user-agent'].flatten('user-agent', obj, flat)
t.equal(flat.userAgent, expectNoCI)
t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')

obj['ci-name'] = 'foo'
obj['user-agent'] = definitions['user-agent'].default
const expectCI = `${expectNoCI} ci/foo`
definitions['user-agent'].flatten('user-agent', obj, flat)
t.equal(flat.userAgent, expectCI)
t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')

delete obj['ci-name']
obj.workspaces = true
Expand All @@ -764,15 +764,15 @@ t.test('user-agent', t => {
definitions['user-agent'].flatten('user-agent', obj, flat)
t.equal(flat.userAgent, expectWorkspaces)
t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')

delete obj.workspaces
obj.workspace = ['foo']
obj['user-agent'] = definitions['user-agent'].default
definitions['user-agent'].flatten('user-agent', obj, flat)
t.equal(flat.userAgent, expectWorkspaces)
t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')
t.end()
})

Expand Down Expand Up @@ -853,3 +853,25 @@ t.test('package-lock-only', t => {
t.strictSame(flat, { packageLock: false, packageLockOnly: false })
t.end()
})

t.test('workspaces', t => {
const obj = {
workspaces: true,
'user-agent': definitions['user-agent'].default,
}
const flat = {}
definitions.workspaces.flatten('workspaces', obj, flat)
t.match(flat.userAgent, /workspaces\/true/)
t.end()
})

t.test('workspace', t => {
const obj = {
workspace: ['workspace-a'],
'user-agent': definitions['user-agent'].default,
}
const flat = {}
definitions.workspace.flatten('workspaces', obj, flat)
t.match(flat.userAgent, /workspaces\/true/)
t.end()
})

0 comments on commit b4aac34

Please sign in to comment.