-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add a new Maven pom.xml extractor #982
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #982 +/- ##
==========================================
+ Coverage 64.05% 64.24% +0.18%
==========================================
Files 146 148 +2
Lines 11977 12088 +111
==========================================
+ Hits 7672 7766 +94
- Misses 3853 3864 +11
- Partials 452 458 +6 ☔ View full report in Codecov by Sentry. |
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 with a question! Also, can you please update your PR title to reflect that this is a version of an extractor that's intended to resolve the full transitive graph?
Also reference #35 in your PR description.
pkg/lockfile/parse-maven-lock.go
Outdated
|
||
// Otherwise return the greatest version matching the constraint. | ||
// TODO: invoke Maven resolver to decide the exact version. | ||
resp, err := e.Client.GetPackage(ctx, &depsdevpb.GetPackageRequest{ |
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.
Question: do you think we'll ever run into cases where we need to parallelise requests to minimize latency?
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 this will be likely to happen - my plan is to make pkg/depsdev
handle all requests (parallelised) to deps.dev.
fwiw I'm not sure that it makes sense to land this in its current form given that it's not being actually being used and violates the "offline" contract, which I don't think we can necessarily address with the current interface so landing it as-is might prematurely lock us into a public API we immediately want to change. |
That's a good point. Is there an easy way we can keep this interface private, while being able to use it ourselves? Alternatively, we can just add some clear documentation to the relevant functions to say: this is experimental and can break. |
Following up on this, the right way to do this for now is probably:
|
For now, I would prefer keeping the new extractor in When it's ready enough, we can think about how to enable this with the |
fwiw if you want to enable experimental support for it without a lot of work, you should be able to add it as a custom parser here |
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.
Thanks! mostly LGTM, added some comments/questions
} | ||
} | ||
|
||
func packageToString(pkg lockfile.PackageDetails) 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.
nit: Add an example in the function comment of what the output looks like.
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.
This is a duplicate from pkg/lockfile
since I reuse the tests there...
if dep.Scope != "" { | ||
pkgDetails.DepGroups = append(pkgDetails.DepGroups, string(dep.Scope)) | ||
} | ||
details[name] = pkgDetails |
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.
Is the dependency name always unique? e.g. could there be the same dependency at different versions (and recorded twice in the Dependencies array)
I'm guessing it's intentional that this is overwriting so that the last version is always the one resolved, can you add a comment a comment here clarifying this.
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 would say don't worry about this too much for now since the logic will be overwritten soon (including how managed dependencies would work).
I copied these code from the current Maven extractor is sort of PoC that deps.dev Maven parser should return the same results if we use the same logic.
if dep.Scope != "" { | ||
pkgDetails.DepGroups = append(pkgDetails.DepGroups, string(dep.Scope)) | ||
} | ||
details[name] = pkgDetails |
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.
Comment here as well
} | ||
|
||
// Otherwise return the greatest version matching the constraint. | ||
// TODO: invoke Maven resolver to decide the exact version. |
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 clarify the difference between what it's currently doing (querying deps.dev) vs invoking the Maven resolver?
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.
For now we only return the greatest version matching the constraint based on the versions returned by deps.dev for range requirements, however, this may not be the version that is in the dependency graph.
To figure out the exact version in the dependency graph, we need help from Maven resolver.
The new Maven lockfile extractor aims to resolve the full Maven dependency graph to provide better transitive support #35. This is an experimental feature for now.
This PR uses deps.dev util package to parse Maven pom.xml, also calls deps.dev API for available versions when resolving a range requirement.