-
Notifications
You must be signed in to change notification settings - Fork 371
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
feat: fetch Maven metadata from specified repositories #1286
Conversation
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.
A few initial comments
internal/resolution/client/client.go
Outdated
// UpdateRegistries updates the registries to fetch data. | ||
UpdateRegistries(registries []string) |
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.
Not sure about using []string
here: npm for example would want a map for {"@scope": "url"}
(if we were to implement it), and both would eventually need authentication information per url.
Could this take in a Manifest
? (but we'd also need to work with lockfiles...)
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 think it makes sense to make this function take a slice of struct instead of a slice of string - there is also Maven specific information I want to add to this struct e.g. if it is allowed to download snapshots.
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.
Looks mostly good with some nits/questions.
@@ -151,6 +151,16 @@ func (c *MavenRegistryClient) MatchingVersions(ctx context.Context, vk resolve.V | |||
return resolve.MatchRequirement(vk, versions), nil | |||
} | |||
|
|||
func (c *MavenRegistryClient) UpdateRegistries(registries []Registry) error { | |||
for _, reg := range registries { | |||
if err := c.api.AddRegistry(reg.URL); err != nil { |
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.
do we need to clear any existing registries? Update
implies we will overwrite all existing ones?
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.
We don't want to clear the existing registries - maybe this should be renamed to AddRegistries
and have another SetRegistries
to clear and set the registries (though not sure if this has any use case)
I mark this PR to draft - the registries should not be added when importing dependencies, but that is now done by |
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.
LGTM - just a couple of small things
#1286 adds support for Maven registry during resolution. As a follow up, this PR updates the documentation for transitive scanning about specifying data source during resolution as well as specifying Maven registry. This PR also corrects the deps.dev API version we are using. We also need to update the documentation in #1181.
#1045
There are repositories defined in a Maven pom.xml. When looking for an artifact, these repositories are searched one by one until the artifact is found. Maven Central is the default registry to try at the last.
To support this behaviour, this PR:
MavenRegistryAPIClient
host a list of registries besides the default registryUpdateRegistries
toDependencyClient
to update the registriesfix
scan
to align with what we have forfix
TODO: