-
Notifications
You must be signed in to change notification settings - Fork 32
let sbom_package_license match clearyly defined #1491
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
base: main
Are you sure you want to change the base?
Conversation
63f2351 to
0224bef
Compare
| curation: Curation, | ||
| db: &C, | ||
| sbom_id: Uuid, | ||
| node_id: String, |
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.
To my understanding clearly defined doesn't have a real "node ID", right? If that's the case, then we could use a static/magic/const value instead. Like we do it with the CyclondeDX root node. In the context of the "SBOM", that would always be unique.
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 was based on this code that I started using the document ID as the node ID.
https://github.com/trustification/trustify/blob/0224bef3af458bca3be3ce30763915dd0d6789f3/modules/ingestor/src/graph/sbom/clearly_defined.rs#L92
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 noticed that. What about the proposal to use a static value?
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 believe it is also acceptable, but it would be better to use the same method in the same place.
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.
What's the benefit?
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.
At this point, the document id incorporates the content of the coordinates from the curation, such as "crate/cratesio/-/chrono." In my view, this approach is more meaningful in practice.
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.
The "node id" is an unique ID in the context of an SBOM. Having a static node id makes it predictable. As the value itself is not important, it wouldn't matter.
Unless there are reasons against it, I'd like to use a static value.
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.
What static value should we use? “ClearlyDefinedCuration”.
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.
Sounds reasonable.
0224bef to
577a884
Compare
| // }); | ||
| sbom_package_license_list.push(sbom_package_license::ActiveModel { | ||
| sbom_id: Set(self.sbom.sbom_id), | ||
| node_id: Set("ClearlyDefinedCuration".to_string()), |
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.
Can you pull that out into a const please.
| let mut purls = PurlCreator::new(); | ||
| let mut licenses = LicenseCreator::new(); | ||
|
|
||
| // TODO: Since the node id cannot be obtained here, it’s not possible to replace purl_license_assertion with sbom_package_license. |
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.
Is that comment still valid?
| // let mut assertions = Vec::new(); | ||
| let mut sbom_package_license_list = Vec::new(); | ||
|
|
||
| let _a: SbomInformation = (&curation).into(); |
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 seems to be dead code.
6b251ab to
7226472
Compare
| use trustify_entity::sbom_package_license; | ||
| use uuid::Uuid; | ||
|
|
||
| const CLEARLYDEFINEDCURATION: &str = "ClearlyDefinedCuration"; |
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.
Can you please use "uppercase snake case": CLEARLY_DEFINED_CURATION
7226472 to
739ae40
Compare
739ae40 to
ec744f4
Compare
3399ef7 to
ec744f4
Compare
No description provided.