Skip to content
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

Get ancestor for displaying dependency tree in relationships #927

Conversation

tarun06
Copy link
Contributor

@tarun06 tarun06 commented Dec 5, 2023

This pr is required by microsoft/sbom-tool#457 which display Hierarchy of package dependency in relatrionship section of SBOM

@tarun06 tarun06 requested a review from a team as a code owner December 5, 2023 09:47
@tarun06 tarun06 changed the title 251 Get ancestor for displaying dependency tree in relationships Get ancestor for displaying dependency tree in relationships Dec 5, 2023
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (715078c) 75.3% compared to head (0433c76) 75.3%.
Report is 2 commits behind head on main.

Files Patch % Lines
...GraphTranslation/DefaultGraphTranslationService.cs 80.0% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #927   +/-   ##
=====================================
  Coverage   75.3%   75.3%           
=====================================
  Files        236     236           
  Lines      10325   10342   +17     
  Branches    1022    1025    +3     
=====================================
+ Hits        7775    7789   +14     
- Misses      2267    2269    +2     
- Partials     283     284    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cobya
Copy link
Contributor

cobya commented Dec 19, 2023

@tarun06 What does this look like in an example ScanManifest output? I'm worried about this change's impact on downstream dependencies

@cobya cobya added status:waiting-on-response Waiting on a response/more information from the user type:refactor Refactoring or improving of existing code labels Dec 19, 2023
@tarun06
Copy link
Contributor Author

tarun06 commented Dec 21, 2023

ScanManifest.json
ScanManifest_WithChange.json

yes, @cobya it does change topLevelReferrers.. Sharing scanmanifest for reference

@cobya
Copy link
Contributor

cobya commented Dec 28, 2023

@tarun06 would you be able to come to Community Meeting 01-03-2024 to discuss? I'd like to get some more context on the change

@tarun06
Copy link
Contributor Author

tarun06 commented Dec 29, 2023

@tarun06 would you be able to come to Community Meeting 01-03-2024 to discuss? I'd like to get some more context on the change

Sure.. Thank you

@tarun06
Copy link
Contributor Author

tarun06 commented Jan 3, 2024

@tarun06 would you be able to come to Community Meeting 01-03-2024 to discuss? I'd like to get some more context on the change

Sure.. Thank you

@cobya i joned the community meeting now.. I am 1 hour late thinking the meeting is at 11:30 IST, Appologies for that.. Let me know if i can answer to queries here. :)

@cobya
Copy link
Contributor

cobya commented Jan 3, 2024

Sure, a couple of things then so we can do this async:

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?
  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)
  • If you are planning on having TopLevelReferrers become the list of ancestors, we should rename it since it's not really accurate anymore

@tarun06
Copy link
Contributor Author

tarun06 commented Jan 4, 2024

Sure, a couple of things then so we can do this async:

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?
  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)
  • If you are planning on having TopLevelReferrers become the list of ancestors, we should rename it since it's not really accurate anymore
  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?

The SBOM tool creates a record of the dependency hierarchy for each package, assigning a unique identifier to each. Developers can easily identify how a transitive dependency is included in their application. Also for fixing security vulnerabilities, teams can identify which package they need to upgrade to resolve the vulnerability on a transitive dependency.

example -> SPDXRef-RootPackage -> Microsoft.EntityFrameworkCore -> Microsoft.Extensions.Caching.Memory ->Microsoft.Extensions.Caching.Abstractions -> Microsoft.Extensions.Primitives -> System.Runtime.CompilerServices.Unsafe
Please check the SBOM PR for complete spdx file generated using this aproach microsoft/sbom-tool#457

  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)

We can create a new field as well.. I opted for "TopLevelReferrers" as I believed it closely resembled its intended meaning. please suggest any name if you have it in mind. Thanks

@tarun06
Copy link
Contributor Author

tarun06 commented Jan 8, 2024

Sure, a couple of things then so we can do this async:

  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?
  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)
  • If you are planning on having TopLevelReferrers become the list of ancestors, we should rename it since it's not really accurate anymore
  • How is SBOM tool planning on using the ancestors information? Parse the graph by looking at each ancestor to see which is the root for each level?

The SBOM tool creates a record of the dependency hierarchy for each package, assigning a unique identifier to each. Developers can easily identify how a transitive dependency is included in their application. Also for fixing security vulnerabilities, teams can identify which package they need to upgrade to resolve the vulnerability on a transitive dependency.

example -> SPDXRef-RootPackage -> Microsoft.EntityFrameworkCore -> Microsoft.Extensions.Caching.Memory ->Microsoft.Extensions.Caching.Abstractions -> Microsoft.Extensions.Primitives -> System.Runtime.CompilerServices.Unsafe Please check the SBOM PR for complete spdx file generated using this aproach microsoft/sbom-tool#457

  • Should we keep this as the existing field or add it as a new one? I'm preferential to a new field because it can be useful to have both sets of information (i.e. give me the full graph vs give me only the component which brings this one)

We can create a new field as well.. I opted for "TopLevelReferrers" as I believed it closely resembled its intended meaning. please suggest any name if you have it in mind. Thanks

taging @cobya

@cobya
Copy link
Contributor

cobya commented Jan 11, 2024

@tarun06 I think we should keep TopLevelReferrers as the list of explicit root only components (the top only), and we can make the new field something like AncestralReferrers (I'm not sold on that, if you have something better feel free) that way anyone who depends on TopLevelReferrers being only root components is unaffected as well.

@tarun06 tarun06 force-pushed the 251-Displays-dependency-tree-in-relationships-section branch 5 times, most recently from 7507a13 to 689d688 Compare January 18, 2024 10:20
@tarun06
Copy link
Contributor Author

tarun06 commented Jan 18, 2024

@cobya i have used AncestralReferrers instead of TopLevelReferrers .. Please review.

@tarun06 tarun06 force-pushed the 251-Displays-dependency-tree-in-relationships-section branch from 689d688 to 0433c76 Compare February 1, 2024 05:40
@cobya cobya added version:minor type:feature Feature (new functionality) and removed status:waiting-on-response Waiting on a response/more information from the user type:refactor Refactoring or improving of existing code labels Feb 1, 2024
@cobya cobya merged commit f48852b into microsoft:main Feb 1, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature Feature (new functionality) version:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants