-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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(java): Adding the ability to find properties in maven profiles (activeByDefa… #7298
base: main
Are you sure you want to change the base?
feat(java): Adding the ability to find properties in maven profiles (activeByDefa… #7298
Conversation
3405bd6
to
7ef4f0e
Compare
@DmitriyLewen reminder on this please |
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.
@coheigea Thanks for your help with java
!
Sorry for delay.
It looks good to me.
I left couple comments about priority.
props := p.content.Properties | ||
props = utils.MergeMaps(props, p.profileProperties()) | ||
return utils.MergeMaps(props, p.projectProperties()) |
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.
maven documentation is not always accurate.
we need to check what priority mvn
has for properties
profiles := p.content.Profiles | ||
logger := log.WithPrefix("pom") | ||
|
||
projectProperties := make(map[string]string) | ||
for _, profile := range profiles.Profile { | ||
activation := profile.Activation | ||
if activation.ActiveByDefault == enabled { | ||
logger.Debug("Adding properties from profile", log.String("id", profile.ID)) |
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:
We may not create a new variable every time
profiles := p.content.Profiles | |
logger := log.WithPrefix("pom") | |
projectProperties := make(map[string]string) | |
for _, profile := range profiles.Profile { | |
activation := profile.Activation | |
if activation.ActiveByDefault == enabled { | |
logger.Debug("Adding properties from profile", log.String("id", profile.ID)) | |
projectProperties := make(map[string]string) | |
for _, profile := range p.content.Profiles.Profile { | |
if profile.Activation.ActiveByDefault == enabled { | |
log.WithPrefix("pom").Debug("Adding properties from profile", log.String("id", profile.ID)) |
for k, v := range profile.Properties { | ||
projectProperties[k] = v | ||
} |
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.
If 2 profiles contains same variable:
- does
mvn
overwrite value? - does
mvn
use 1 found value?
…ult only)
Currently Trivy is not capable of parsing properties in Maven profiles. It is a pretty common scenario to have different versions of the same dependency in multiple maven profiles, e.g. maybe to support different framework versions with a product.
This PR adds the ability to find properties in a maven profile, but only if it's "activeByDefault". It's impossible to determine otherwise activation properties of maven.
@DmitriyLewen PTAL and let me know if you approve the general approach and I'll add tests.