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

use configured global registry for library lookup #5840

Merged

Conversation

jakecoffman
Copy link
Member

Context

Dependabot honors an .npmrc or .yarnrc file to set the global registry. When a global registry is present in one of these files, Dependabot shouldn't make calls to registry.npmjs.org. This library check was doing just that!

Approach

At first I tried using the RegistryFinder's registry method, but that method assumes you're giving it a dependency. The library check is checking the registry for the project itself, and thus won't be present in the dependencies of the project when RegistryFinder goes to look for it.

So rather than change the registry method, I exposed the global_registry method so I could use it in the library check. I also modified it to stop stripping the protocol, since it is valid for enterprises to have an internal registry that isn't secured by TLS. I put the stripping in the first_registry_with_dependency_details which was the only place it was used.

@jakecoffman jakecoffman requested a review from a team as a code owner October 6, 2022 16:23
@jakecoffman
Copy link
Member Author

Hmm it's also worth considering scoped dependencies in this case. I might have to do another pass at this to search the rc files for scopes that match. There doesn't appear to be any scoped registry handling in the RegistryFinder.

@jakecoffman
Copy link
Member Author

I moved global_registry back to being private and created a new public registry_from_rc method which will look up what registry to use from the provided .npmc or .yarnrc file.

@honeyankit
Copy link
Contributor

honeyankit commented Oct 6, 2022

Just for my understanding the global registry == private registry ?

@jakecoffman
Copy link
Member Author

jakecoffman commented Oct 6, 2022

@honeyankit yes typically, although conceivably it could point to anything. In npm and yarn, you can configure a registry globally or scoped. In npm that looks like:

registry=https://example.com                      #global
@dependabot:registry=https://npm.pkg.github.com   #scoped

@pavera
Copy link
Contributor

pavera commented Oct 6, 2022

Are we concerned with changing the default behavior for all dependabot users? I think this seems like the more correct behavior in general, but this does change how dependabot currently behaves in a way users might not expect. Should we put this behind a feature flag?

@jakecoffman
Copy link
Member Author

@pavera I'm not concerned. This code path only influences whether the update strategy is widen ranges or bump versions. Dependabot is already using the private registry for all the other calls.

So folks who don't use an .npmrc won't see a difference. Those that have a registry defined are likely to see more correct behavior, especially those who've published to the private registry, as we can now correctly identify a library that has been published there.

@jakecoffman jakecoffman merged commit 2125e6a into main Oct 7, 2022
@jakecoffman jakecoffman deleted the jakecoffman/use-global-registry-for-library-lookup branch October 7, 2022 16:03
@pavera pavera mentioned this pull request Oct 31, 2022
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.

3 participants