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

Support lowercase environment variables to make npm config working. #24556

Closed
abcfy2 opened this issue Nov 6, 2022 · 0 comments · Fixed by #24664
Closed

Support lowercase environment variables to make npm config working. #24556

abcfy2 opened this issue Nov 6, 2022 · 0 comments · Fixed by #24664
Labels
E2E Issue related to end-to-end testing type: feature New feature that does not currently exist

Comments

@abcfy2
Copy link
Contributor

abcfy2 commented Nov 6, 2022

Current behavior

npm config set CYPRESS_DOWNLOAD_PATH_TEMPLATE 'https://cdn.npmmirror.com/binaries/cypress/${version}/${platform}-${arch}/cypress.zip' not working in current behavior.

npm config set CYPRESS_DOWNLOAD_PATH_TEMPLATE 'https://cdn.npmmirror.com/binaries/cypress/${version}/${platform}-${arch}/cypress.zip'
DEBUG='cypress:*' npm i cypress

You can see download is still https://download.cypress.io/desktop/10.11.0?platform=linux&arch=x64

Desired behavior

npm config set will inject lowercase environment variables.

E.g: npm config set CYPRESS_DOWNLOAD_PATH_TEMPLATE 'https://cdn.npmmirror.com/binaries/cypress/${version}/${platform}-${arch}/cypress.zip' will inject:

npm_config_cypress_download_path_template=https://cdn.npmmirror.com/binaries/cypress/${version}/${platform}-${arch}/cypress.zip

So I think

cypress/cli/lib/util.js

Lines 532 to 564 in d52e610

getEnv (varName, trim) {
la(is.unemptyString(varName), 'expected environment variable name, not', varName)
const configVarName = `npm_config_${varName}`
const packageConfigVarName = `npm_package_config_${varName}`
let result
if (process.env.hasOwnProperty(varName)) {
debug(`Using ${varName} from environment variable`)
result = process.env[varName]
} else if (process.env.hasOwnProperty(configVarName)) {
debug(`Using ${varName} from npm config`)
result = process.env[configVarName]
} else if (process.env.hasOwnProperty(packageConfigVarName)) {
debug(`Using ${varName} from package.json config`)
result = process.env[packageConfigVarName]
}
// environment variables are often set double quotes to escape characters
// and on Windows it can lead to weird things: for example
// set FOO="C:\foo.txt" && node -e "console.log('>>>%s<<<', process.env.FOO)"
// will print
// >>>"C:\foo.txt" <<<
// see https://github.com/cypress-io/cypress/issues/4506#issuecomment-506029942
// so for sanity sake we should first trim whitespace characters and remove
// double quotes around environment strings if the caller is expected to
// use this environment string as a file path
return trim ? dequote(_.trim(result)) : result
},

Should also support lowercase environment variables.

Test code to reproduce

npm config set CYPRESS_DOWNLOAD_PATH_TEMPLATE 'https://cdn.npmmirror.com/binaries/cypress/${version}/${platform}-${arch}/cypress.zip'
DEBUG='cypress:*' npm i cypress

Cypress Version

10.11.0

Node version

18

Operating System

Linux

Debug Logs

(##################) ⠧ reify:cypress: timing reifyNode:node_modules/lodash Completed in 1475ms
> cypress@10.11.0 postinstall
> node index.js --exec install

  cypress:cli installing Cypress from NPM +0msyNode:node_modules/cypress Completed in 1495ms
  cypress:cli installing with options {} +0msfyNode:node_modules/cypress Completed in 1495ms
  cypress:cli detecting arch { osPlatform: 'linux', osArch: 'x64' } +0ms
  cypress:cli arm uname -m result: { stdout: 'x86_64' }  +34mses/cypress Completed in 1495ms
  cypress:cli version in package.json is 10.11.0, version to install is 10.11.0 +35ms
  cypress:cli Reading binary package.json from: /home/fengyu/.cache/Cypress/10.11.0/Cypress/resources/app/package.json +0ms
  cypress:cli no binary installed under cli version +16ms
  cypress:cli checking local file /tmp/tmp/10.11.0 cwd /tmp/tmp/node_modules/cypress +1ms
  cypress:cli preparing to download and unzip version  10.11.0 to path /home/fengyu/.cache/Cypress/10.11.0 +0ms
Installing Cypress (version: 10.11.0)


  cypress:cli needed Cypress version: 10.11.0 +0ms
  cypress:cli source url https://download.cypress.io/desktop/10.11.0?platform=linux&arch=x64 +1ms
  cypress:cli downloading cypress.zip to "/tmp/cypress-17897.zip" +0ms
  cypress:cli Downloading package {
  url: 'https://download.cypress.io/desktop/10.11.0?platform=linux&arch=x64',
  proxy: null,
  downloadDestination: '/tmp/cypress-17897.zip'
} +7ms
  cypress:cli expected file size 82 +1sng reifyNode:node_modules/cypress Completed in 1495ms
  cypress:cli redirect version: 10.11.0 +0ms
  cypress:cli redirect url: https://cdn.cypress.io/desktop/10.11.0/linux-x64/cypress.zip +0ms
  cypress:cli Downloading package {
  url: 'https://cdn.cypress.io/desktop/10.11.0/linux-x64/cypress.zip',
  proxy: null,
  downloadDestination: '/tmp/cypress-17897.zip'
} +1ms
  cypress:cli expected checksum c33b2a3eba0277da47274aff2e7a4e618531977355951d6f078ebd313d0a25d1807d6f5c30ccac19f0c292a1093f0494eb963e7e9a9db784fba41bd06b77567e +675ms


### Other

_No response_
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 13, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 17, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 17, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 22, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 23, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 24, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
abcfy2 added a commit to abcfy2/cypress that referenced this issue Nov 27, 2022
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
mjhenkes added a commit that referenced this issue Dec 1, 2022
* fix: get correct env from npm config

`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: #24556

* Disable commercial recommendations for system tests.

* Disable commercial recommendations

* Update system-tests/lib/system-tests.ts

* Revert changes

* Update cli/test/lib/util_spec.js

* Update cli/test/lib/util_spec.js

Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2E Issue related to end-to-end testing type: feature New feature that does not currently exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant