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

NPM: fix npmrc generation for v3 package-locks #7175

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

jakecoffman
Copy link
Member

Fixes #6661

Using a v3 package-lock, dependency-urls was returning [] because there is no top-level dependencies property. [].all? turns out is always true, so it was always sticking the line registry=<my-configured-reigstry> into the .npmrc even for those hosted on GHPR which isn't a reverse-proxy style registry. Thus it is not working.

I switched to a regex based approach to extract all of the "resolved" lines. This is probably more correct than the current implementation even for v2 lockfiles as this checks all dependencies, not just the ones at the top level. In the event package-lock v4 removes or renames the resolved lines, I've added a check that dependency-urls isn't empty.

Also once I fixed this, some tests started doubling up on scoped registries, so I had to add a uniq.

@jakecoffman jakecoffman requested a review from a team as a code owner April 26, 2023 13:47
@jakecoffman jakecoffman force-pushed the jakecoffman/fix-v3-npmrc-generation branch from b7f3e67 to 5885112 Compare April 26, 2023 13:50
@yeikel
Copy link
Contributor

yeikel commented Apr 26, 2023

Could you please add a test where the registry URL is behind a path? ie: https://npm.pkg.github.com/myOrg/

See #7030 for more context

@jakecoffman
Copy link
Member Author

jakecoffman commented Apr 26, 2023

@yeikel I could be missing something but this code path doesn't appear to inject registries from the resolved lines. It's using it to identify possible scopes for registries defined as credentials. It's also using it to decide if there's a global registry. In both cases it uses the registry defined in the credentials to write to the npmrc, so if there were a path there it would preserve it.

It looks like #7030 will have the same issue with v3 lockfiles though so I could add some tests around that when we get to that PR to make this same change.

@yeikel
Copy link
Contributor

yeikel commented Apr 26, 2023

@yeikel I could be missing something but this code path doesn't appear to inject registries from the resolved lines. It's using it to identify possible scopes for registries defined as credentials. It's also using it to decide if there's a global registry. In both cases it uses the registry defined in the credentials to write to the npmrc, so if there were a path there it would preserve it.

It looks like #7030 will have the same issue with v3 lockfiles though so I could add some tests around that when we get to that PR to make this same change.

You're right, sorry for the confusion

@jakecoffman jakecoffman merged commit 963242c into main Apr 26, 2023
@jakecoffman jakecoffman deleted the jakecoffman/fix-v3-npmrc-generation branch April 26, 2023 16:28
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependabot Scoped Private Registry (NPM) erroring on public packages
3 participants