-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: handle npm prefix config environment variables #7070
Conversation
@@ -171,7 +171,9 @@ export default class BaseRegistry { | |||
key = key.replace(/__/g, '.'); | |||
|
|||
// replace underscores with dashes | |||
key = key.replace(/_/g, '-'); |
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.
Can you instead add it in a more generic way that supports __
?
For example: key.replace(/([^_])_/g, '$1-')
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.
That's a good idea. We're probably assuming any key starting with an underscore needs to stay an underscore, but that's probably always true
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.
@arcanis Are the changes satisfactory?
# Conflicts: # CHANGELOG.md
test('correctly escapes environment config variables', () => { | ||
const testCwd = '.'; | ||
const {mockRequestManager, mockRegistries, mockReporter} = createMocks(); | ||
const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter, true, []); |
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 had trouble figuring out where these tests should go. mergeEnv
is an instance method that requires a whole mocked registry to test.
Simplifying this test would involve refactoring the mergeEnv
method to use a utility method, but I decided to reduce the changes to the code and instead have a more complicated test
Yep! Thanks 😃 |
Fixes #4682
Summary
Passing
npm_config_
environment variables changed all underscores (_
) to dashes (-
) which isn't correct behavior for special config variables (_auth
,_authToken
,_username
and_password
). An additional check was added tobase-registry.js
to filter out these config keys from this processing.Test plan
yarn test
- all tests passed except two tests that have nothing with my change - they are integration tests (I assume my environment isn't set up correctly for these to pass? Maybe CI will verify?)