-
Notifications
You must be signed in to change notification settings - Fork 593
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
Account for maven bundle plugin and fix filename matching #2220
Conversation
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
f2920df
to
237cffc
Compare
Signed-off-by: Alex Goodman <[email protected]>
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 - thanks for carving out this path to get a better artifact ID and the subsequent groupID changes. The examples were really easy to follow and the testing added is really clear.
I had a questions about the following:
When attempting to find if a pom.xml matches with an existing package, allow for the artifact ID to be a suffix of the filename (not just the filename as a prefix of the artifact ID as it is today)
Memory: I thought we had experimented with this in the past and found it surfaced too many incorrect pom.xml. Basically, suffix checks were too lose and the naming conventions in java were too broad to allow this kind of pom to be checked. Yes we might miss some things where this case exists, but limiting the data here was correct in that we were not getting more wrong hits than correct hits.
What was the trigger for making this change?
@@ -237,7 +237,7 @@ func (j *archiveParser) guessMainPackageNameAndVersionFromPomInfo() (name, versi | |||
projects, _ := pomProjectByParentPath(j.archivePath, j.location, pomMatches) | |||
|
|||
for parentPath, propertiesObj := range properties { | |||
if propertiesObj.ArtifactID != "" && j.fileInfo.name != "" && strings.HasPrefix(propertiesObj.ArtifactID, j.fileInfo.name) { | |||
if artifactIDMatchesFilename(propertiesObj.ArtifactID, j.fileInfo.name) { |
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.
+1 to untangle this many thanks
Ahh - I'll fix the merge commit - apologies |
Signed-off-by: Christopher Phillips <[email protected]>
Essentially it was from taking a closer look at some of the artifacts referenced in the attached issues.
I can't recall the specifics of what was experimented on from 3-ish months ago, but this seems different than the original implementation from about a year ago. |
* incorporate anchore/syft#2220 Signed-off-by: Alex Goodman <[email protected]> * incorporate .net core improvements Signed-off-by: Alex Goodman <[email protected]> --------- Signed-off-by: Alex Goodman <[email protected]>
* account for maven bundle plugin and fix filename matching Signed-off-by: Alex Goodman <[email protected]> * add in-repo jar tests based on metadata to cover anchore#2130 Signed-off-by: Alex Goodman <[email protected]> * tests: fix test merge commit Signed-off-by: Christopher Phillips <[email protected]> --------- Signed-off-by: Alex Goodman <[email protected]> Signed-off-by: Christopher Phillips <[email protected]> Co-authored-by: Christopher Angelo Phillips <[email protected]> Co-authored-by: Christopher Phillips <[email protected]>
This PR makes a few changes to the java archive cataloger:
Bundle-SymbolicName
field to get a group ID and artifact ID. Then we can extract the artifact ID as the package name.Blocked until the following are verified:
Closes #2217
Fixes #2130