-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
Introduce "collection" projects for better usage of hierarchical view #2041 #3258
base: master
Are you sure you want to change the base?
Introduce "collection" projects for better usage of hierarchical view #2041 #3258
Conversation
Has there been any updates/progress on the feature in completion of the 4.11 release? |
Looking forward for this feature 👍 |
0aa204e
to
ddc5c2b
Compare
@nscuro I wonder why the unit test still fails. I don't see a change that could influence the parent of a project not being serialized? |
@rkg-mm FWIW there can be subtle changes in how serialization libraries (i.e. Jackson) handle certain fields, when those libraries are updated. Or how the persistence framework fetches fields implicitly. |
Yea so, the PRs backend & frontend seem to be working well. But I have no clue about this unit test and will need some help here. The test explicitly tests the now not working condition, so I guess its important that it works, but I don't see a change that causes the failure.
|
* @author Ralf King | ||
* @since 4.11.0 | ||
*/ | ||
public enum ProjectCollectionLogic { |
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.
It would be good to add some documentation about this. In #2041 you mention a few use cases that these address, having those be part of the docs will definitely help others.
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.
Docs added. However, since I'm stuck to Windows and there is no build script for the docs in windows, I could not really test if this integrates properly. Can you check this?
src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java
Outdated
Show resolved
Hide resolved
@@ -538,6 +547,7 @@ public Project updateProject(Project transientProject, boolean commitIndex) { | |||
project.setDescription(transientProject.getDescription()); | |||
project.setVersion(transientProject.getVersion()); | |||
project.setClassifier(transientProject.getClassifier()); | |||
project.setCollectionLogic(transientProject.getCollectionLogic()); |
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.
Changing the collection logic from say AGGREGATE_DIRECT_CHILDREN
to HIGHEST_SEMVER_CHILD
can drastically change metric values on existing projects. There will be no indication as to if and when the logic was changed, which can make it tricky to explain why values changed so much.
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.
That's correct, but also it would be an intended change. Idk what could be done about this except add such change to the logfile, but that would only be helpful for deep analysis of such issue.
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.
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.
I like that idea, but at least on a first look the frontend chart lib doesn't support this. not sure how difficult this would be to add on top of it. Any experience with that? I never used that component.
From data perspective in backend it should not be too complicated.
edit: or would be adding it to the charts tooltip be enough? Also it seems there are options to style datapoints differently. Maybe we could give such points a different styling + add the info in the tooltip?
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.
I don't have too strong opinions on how we highlight it, but I do think that highlighting it one way or the other would be good.
Tooltip on it's own might be too hidden, but styling the datapoints sounds good.
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.
@nscuro would you be fine with creating a new issue to add this visibility later as further enhancement, and for now rely on log file for visibility in case someone wonders? we could also add a hint in the project edit UI when someone changes the logic, that informs him of this for now?
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.
Logging is probably fine for an MVP implementation, yes. But ultimately we will really need some kind of indicator in the UI.
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.
@nscuro I found a way to show it in the metrics chart. Only issue is that the line gets invisible when hovered. No Idea why. But I think this is fine as it is, looks like its intended :D
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.
Ok found the reason for the visibility issue and fixed it. Also extended it to the other charts now. Looks like this:
Hint: The blue line at 0 will no longer be there, I improved the backend to not record the initial entry after an update to the new version anymore as changed collection logic, my DB just has these entries already
src/main/java/org/dependencytrack/tasks/metrics/ProjectMetricsUpdateTask.java
Outdated
Show resolved
Hide resolved
src/main/java/org/dependencytrack/model/ProjectCollectionLogic.java
Outdated
Show resolved
Hide resolved
I debugged the area in question multiple times now and still don't quite understand how this is happening. The parent is part of the response when I comment out these lines: https://github.com/DependencyTrack/dependency-track/pull/3258/files#diff-75cd29a0a085d84dff3cb4794242e0d1b400385f1acfae172346c3e6780629b0R582-R590 Something about those lines causes |
oh come on.. Idk what IntelliJ is doing but whenever I rebase this branch on your master I end up having all changes from master in my PR again. What am I doing wrong here? I don't get it... edit: ok fixed it again with some hacky workaround. still don't know what I do wrong here... |
efe2c29
to
ecfc394
Compare
@rkg-mm How do you do the rebase? I usually run I never had this particular problem happen, but can imagine IntelliJ might do some unexpected things when merging/rebasing via UI. |
In IntelliJ UI I select the upstream master and select "Rebase (my branch) onto master" then push the changes to my remote. The strange thing happens here already, as suddenly my own remote has changes not recognized by my local branch and needs to either be merged (which I tried before and ended up with double changes on same files) or I select "rebase anyway" which avoids double merging. But in both cases I still end up with all those unwanted changes again. Maybe I should try the console attempt next time... |
Fixed by your suggestion |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more |
@rkg-mm We have to schedule this for 4.12 to allow us for sufficient time to review and test. |
no problem, I understand. Lets just hope the release cycle is a bit shorter this time :D |
Is there a set release cadence? or other logic for determining when releases are completed? |
There is not at the moment. We originally planned to do monthly releases, but ended up stretching that (by far...) yet again. A problem is that the core team has certain items they want (and have to work on) in a given release, while said core team also needs to review and test community contributions. It's a luxury problem, but it means that more time is needed to meet both our own, and the community's expectations. This particular PR was planned for inclusion in 4.11 for a long time, but it also touches on the very core of how DT behaves, in turn requiring more review and testing than other, smaller PRs. And of course contributors also don't always have time to answer our annoying requests. :) We're still in the process of figuring out how to best plan and size releases. Going forward, we're labelling issues with T-shirt sizes, according to their effort. Aim being to limit the number of high-effort items per release, enabling faster cycles. |
Re-assigning to 4.13 milestone in order to reduce the pressure on contributors to get this finished whilst allowing v4.12.0 to be released quicker. |
0cfdd8c
to
cb0b75b
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
@nscuro apart from the one proposal in the UI PR which I added some question for, this should be finished now. Can you review it again please? |
Nice to see this PR moving forward. We run a previous version of the PR in our experimental setup. Will update with the latest version and provide feedback. Some minor thing that we noticed and that would be great if that could be changed or supported as well: when modeling your products in a parent / child relationship like that: You are forced to set a classifier for the parent project, though it is purely a project container and the classifier does not make any sense in this context. If the classifier could be made optional or better reflect the concept of a project container, it would be nice and avoid confusion when looking at the project hierarchy. |
So basically you want to have an option which marks the parent as a collection project, hence removes the component uploads etc. but does not calculate any values based on children? what should be displayed inside the parent then? nothing? |
…CollectionLogics. Modifies project metrics updates to consider collection project logics. Added several events to trigger project metrics updates to update collection projects if child projects change in a relevant way. Signed-off-by: Ralf King <[email protected]>
Add test cases for new metrics update logic for collection projects. Signed-off-by: Ralf King <[email protected]>
… all projects Signed-off-by: Ralf King <[email protected]>
Signed-off-by: Ralf King <[email protected]>
… wrongly influence numbers Signed-off-by: Ralf King <[email protected]>
Signed-off-by: Ralf King <[email protected]>
Signed-off-by: Ralf King <[email protected]>
* Add validation to prevent invalid states of collection projects (prevent collection project having Components or services), including several unit tests covering these scenarios. Signed-off-by: Ralf King <[email protected]>
…ject actually changed. Signed-off-by: Ralf King <[email protected]>
Signed-off-by: Ralf King <[email protected]>
…" flag in projects (to be implemented with issue 4148) Signed-off-by: Ralf King <[email protected]>
…his feature Signed-off-by: Ralf King <[email protected]>
…nt in UI to visualize why drastic changes in numbers occured. Signed-off-by: Ralf King <[email protected]>
…change in UI for first new entry. Signed-off-by: Ralf King <[email protected]>
5acc770
to
5fc4375
Compare
Signed-off-by: Ralf King <[email protected]>
Signed-off-by: Ralf King <[email protected]>
so the behavior is just fine as it is right now. It feels just odd that when modeling your projects with dependency track you are forced to enter some data for a project whose only purpose is to act as a container for nested projects. Right now you have to select at least a classifier afaict, so in our case (see the screenshot) we have selected Application, though it is not an Application, but the project that consists of various software products. When selecting that project, you should see all vulnerabilities of child projects as done right now. This was discussed yesterday in the community call and its a really small thing but people asked me to raise it in this ticket as well. |
Signed-off-by: Ralf King <[email protected]>
…_VERSION_CHILDREN to match other 2 names * Add missing test for AGGREGATE_LATEST_VERSION_CHILDREN logic * Update outdated tests with additional property Signed-off-by: Ralf King <[email protected]>
@nscuro |
Signed-off-by: Ralf King <[email protected]>
I added a NONE classifier now after watching the community meetings on youtube. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
fyi: a package of this PR is available at https://github.com/users/netomi/packages/container/dtrack-apiserver/289607235?tag=4.13.0-SNAPSHOT ghcr.io/netomi/dtrack-apiserver:4.13.0-SNAPSHOT |
Hello, It's give me 2 questions/thoughts:
|
Looping through the complete child-subtrees (and on updates of a project through all parents to see if there is any metrics to update) would have a significant performance effect in large instances, which is why I limited possible rules to direct children only.
Unless I made a mistake (but I think I tested this) the propagation should go to the direct parent (due to above reasons). But once the parents metrics change, this will trigger an update of the parent's parent, if this also is a calculated project, and so on, so you can build up whole calculated trees this way. You can see the logic in this screenshot https://raw.githubusercontent.com/DependencyTrack/dependency-track/f14fc9a005743f74b5a8ae5812792eac3b956cc5/docs/images/screenshots/collection-projects-structure.png |
Description
This change introduces logic for "collection projects". Those are basically projects used as parent for other projects that shall not hold any own component or vulnerability data, but instead get calculated from child projects using different configurable aggregation logics.
Corresponding Frontend PR with Screenshots: DependencyTrack/frontend#658
Addressed Issue
resolves #2041
resolves #657
resolves #2410
Additional Details
Checklist
This PR fixes a defect, and I have provided tests to verify that the fix is effective