-
Notifications
You must be signed in to change notification settings - Fork 46
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
🐛 Update indirect Dependency location #557
Conversation
Updating dependency incident location lookup for indirect dependencies to point to "parent" dependency location. E.g. for `maven-javax-to-jakarta-00002` rule. Related to https://issues.redhat.com/browse/MTA-2006 Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.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.
Thank you for the fix, overall looks good. Requesting minor changes
rule-example.yaml
Outdated
xpath: /b:beans | ||
- category: potential | ||
customVariables: [] |
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: can we shrink this rule, just keep the required fields for brevity i.e. remove labels, links, customVariables?
@@ -361,6 +374,19 @@ | |||
resolvedIdentifier: 7873092d39ef741575ca91378a6a21c388363ac8 | |||
extras: | |||
artifactId: logback-core | |||
baseDep: | |||
name: ch.qos.logback.logback-classic |
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: can we produce only the required fields of the baseDep
provider/provider.go
Outdated
@@ -745,6 +745,15 @@ func (dc DependencyCondition) Evaluate(ctx context.Context, log logr.Logger, con | |||
if err == nil { | |||
incident.LineNumber = &location.StartPosition.Line | |||
incident.CodeLocation = &location | |||
} else if baseDep, ok := matchedDep.dep.Extras["baseDep"]; ok { |
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.
since I looked into too many perf issues recently :)... maybe we can make only one call to GetLocation based on either direct or base dep?
Thanks for review @pranavgaikwad, will update! |
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.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.
@aufi Thank you! LGTM
This reverts commit b4d2369.
This slipped through somehow after merging konveyor#351 I should have re-run CI before merging konveyor#557 Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
This slipped through somehow after merging konveyor#351 I should have re-run CI before merging konveyor#557 Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Marek Aufart <aufi.cz@gmail.com>
This slipped through somehow after merging konveyor#351 I should have re-run CI before merging konveyor#557 Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Updating dependency incident location lookup for indirect dependencies to point to "parent" dependency location. E.g. for maven-javax-to-jakarta-00002 rule.
Replaces #532
Related to https://issues.redhat.com/browse/MTA-2006