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

fix: get correct env from npm config #24664

Merged
merged 10 commits into from
Dec 1, 2022
Merged

Conversation

abcfy2
Copy link
Contributor

@abcfy2 abcfy2 commented Nov 13, 2022

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

Closes: #24556

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 13, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Nov 13, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ryanthemanuel ryanthemanuel left a comment

Choose a reason for hiding this comment

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

Can we add a test for this?

@abcfy2
Copy link
Contributor Author

abcfy2 commented Nov 17, 2022

Can we add a test for this?

I don't know how to add this test case. Because npm config set FOO BARshould execute before test.

If we add this command in unit test, then we should execute this test in sub process.

@abcfy2 abcfy2 force-pushed the develop branch 2 times, most recently from 32ecdb1 to 761cfec Compare November 17, 2022 03:22
@abcfy2
Copy link
Contributor Author

abcfy2 commented Nov 17, 2022

Can we add a test for this?

I try to add a simple unit test for this. You can review again. Thanks.

@mjhenkes mjhenkes self-assigned this Nov 23, 2022
@abcfy2 abcfy2 force-pushed the develop branch 2 times, most recently from 248a3b9 to 5d33f30 Compare November 24, 2022 00:58
`npm config set VAR VAL` will inject `npm_config_var=val` environment
variable. This commit will solve this issue

Closes: cypress-io#24556
@abcfy2
Copy link
Contributor Author

abcfy2 commented Nov 28, 2022

Hi @marktnoonan . Please review my PR and let me know if I lost something. Thanks.

.circleci/workflows.yml Outdated Show resolved Hide resolved
.circleci/workflows.yml Outdated Show resolved Hide resolved
.circleci/workflows.yml Outdated Show resolved Hide resolved
cli/test/lib/util_spec.js Outdated Show resolved Hide resolved
@mjhenkes
Copy link
Member

mjhenkes commented Dec 1, 2022

Yay a green build, @abcfy2 thanks for the PR and being patient while i got the build fixed!

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.

Support lowercase environment variables to make npm config working.
4 participants