-
Notifications
You must be signed in to change notification settings - Fork 43
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 management column for application dependencies table #1541
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1541 +/- ##
=======================================
Coverage 40.46% 40.46%
=======================================
Files 145 145
Lines 4636 4636
Branches 1088 1088
=======================================
Hits 1876 1876
Misses 2746 2746
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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.
Structure looks good. 2 comments.
const labelValue = getParsedLabel(label).labelValue; | ||
return labelValue === "java"; | ||
}); | ||
const isJavaFile = dependency.name.endsWith(".jar"); |
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.
-
Probably better to use
appDependency.dependency.name
just to keep the sourced data in the same place for the function. Or move the function and the TD render to a column render helper. Those helpers can be in the same file. That's what I'm doing in 🐛 Render dependency versions as links #1540. -
I don't see any dependency names ending with ".jar". All the names I see are
mavenGroupId.mavenArtifactId
. I assume theappDependency.dependency.provider
string is supposed to be the language, but I've only seen empty string values for my analyses.
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.
@sjd78
@pranavgaikwad mentioned that we only see ".jar" deps when we are using a binary file as the input.
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.
Oh that is an interesting distinction.
So only "java" things where sources are not available will have a name with a .jar
suffix.
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.
Updated the PR to include the render function helper. As far as checking available sources, not sure that I have a grasp on how we should go about considering that for the management column.
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
I'm ok with moving along with the current calculation and adjusting with another PR in future if needed.
= Add management column for application dependencies table Signed-off-by: ibolton336 <[email protected]> PR updates Signed-off-by: ibolton336 <[email protected]> Add optional chaining for empty labels Signed-off-by: ibolton336 <[email protected]> filter out language labels Signed-off-by: ibolton336 <[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.
As long as the column isJavaDependency
tests are good, LGTM
Missing column "Management" in the Dependencies side drawer #1339
Depends on konveyor/tackle2-addon-analyzer#62