-
Notifications
You must be signed in to change notification settings - Fork 515
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
General Java cataloger enhancements #247
Conversation
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
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 lot of great work, very nice!
@@ -156,6 +160,7 @@ func (j *archiveParser) discoverMainPackage() (*pkg.Package, error) { | |||
|
|||
// discoverPkgsFromPomProperties parses Maven POM properties for a given parent package, returning all listed Java packages found and | |||
// associating each discovered package to the given parent package. | |||
// nolint:funlen,gocognit |
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 isn't a request for change, but a note for discussion at some point in the future — I think we might be getting into that stage of the code base's life where functions are slowly entering the large/complex territory. I'd love to see us discuss this as a group at some point and think about ways to extract functions and keep the code as readily understandable as possible
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.
My default stance when I run into these points is to break it up into smaller helper functions (which I've had to a few times now)... this one seemed rather non-obvious in how to do so, so punted until the new pattern is more obvious. This time I added more comments to help compensate.
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.
The comments are great. Definitely better than not having them. If you want to talk through this function, just as an exercise, let me know. But to me, it's not something that should block this PR
…ration General Java cataloger enhancements
versionAreaPattern
to the java cataloger and attempts to remove version and extra patterns found as opposed to extract the name (since it is assumed that the name is always at the front of the string)main
(the first/default section that may not have aname
) andnamedSections
(all remaining sections). This more closely follows the MANIFEST.MF specification.alpine:3.12.0
,anchore/test_images:java
,anchore/test_images:py38
,anchore/anchore-engine:v0.8.2
, andjenkins/jenkins:2.249.2-lts-jdk11
images to the compare script in the acceptance tests. The goal was to add java oriented images, however, theanchore/test_image:java
image is alpine based, so a workaround for compare testing needed to be made to account for anchore-engine's stripping of the release values from the version field. This opened up the ability to add a base alpine image. Previous PRs should have put in the python example image, but were blocked from doing so for similar reasons. Thejenkins
image has proven to be an excellent java image example overall while triaging OSS issues.Partially addresses anchore/anchore-engine#681 (tasks 2 & 4)