Skip to content

Conversation

@Frozen-byte
Copy link
Contributor

closes: #1310

@Frozen-byte Frozen-byte requested a review from a team as a code owner December 3, 2024 13:00
src/extractor.ts Outdated
component.evidence = new CDX.Models.ComponentEvidence({
licenses: new CDX.Models.LicenseRepository(this.getLicenseEvidence(dirname(pkg.path), logger))
})
for (const line of this.getCopyrightEvidence(dirname(pkg.path), logger)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like use the same Method as lin 112, but sadly the Stringable is not exported via CDX.Models and the only way I found to create that Set is with the Component Evidence constructor. Maybe someone else have a more elegant way to iterate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would you need the Stringable being exported?

Copy link
Contributor Author

@Frozen-byte Frozen-byte Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I think its more elegant to use something like the following:

      component.evidence = new CDX.Models.ComponentEvidence({
        licenses: new CDX.Models.LicenseRepository(this.getLicenseEvidence(dirname(pkg.path), logger)),
        copyright: new SortableStringables(copyrights)
      })

The SortableStringables is nowhere exported via the CDX object. Importing directly from '_helpers/sortable' will not work since its not exported in any barrel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
If you feel that it is a good idea to export the SortableStringables, then feel free to write an issue for it in the library's repository: https://github.com/CycloneDX/cyclonedx-javascript-library

Copy link
Member

@jkowalleck jkowalleck Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding and exporting a dedicated type for the purpose new CDX.Models.CopyrightRepository?
The thing is, that the SortableStringables is internal for a good reason. it is just no stable/public interface, yet.
see CycloneDX/cyclonedx-javascript-library#1192

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Frozen-byte with version 7.1.0 of the JS library, the needed model was introduced.
see https://github.com/CycloneDX/cyclonedx-javascript-library/releases/tag/v7.1.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this dependency is intended to be bumped to ^7.0 via #1331

please remember to bump to ^7.1 with this PR 👍

Copy link
Contributor Author

@Frozen-byte Frozen-byte Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR was idle for a long time, v8 was already used on the main branch.

Updated to the more elegant way, that uses no iteration loop.

@jkowalleck jkowalleck changed the title feat(Extractor): add copyright to evidence collection feat: add copyright to evidence collection Dec 3, 2024
src/extractor.ts Outdated
try {
pcis = readdirSync(packageDir, {withFileTypes: true})
} catch (e) {
logger?.warn('collecting license evidence in', packageDir, 'failed:', e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"license evidence"? Probably a copy/paste error

@jkowalleck
Copy link
Member

jkowalleck commented Dec 3, 2024

Thank you for working on this feature.

I do not see any test, so I assume this PR is still a work in progress.
Therefore, I will set it to "draft".

Feel free to set it to "ready for review", when all work is done.

@jkowalleck jkowalleck marked this pull request as draft December 3, 2024 13:58
@jkowalleck
Copy link
Member

jkowalleck commented Jan 23, 2025

  • this pr is outdated, it needs to rebase/merge latest master.

@Frozen-byte
Copy link
Contributor Author

Thank you for working on this feature.

I do not see any test, so I assume this PR is still a work in progress. Therefore, I will set it to "draft".

Feel free to set it to "ready for review", when all work is done.

I added the updated snapshots.
Am I supposed to test something in detail? The coverage looked fine for me.
Do you think it is necessary to add a new folder in the integration tests?
I cannot imagine the value of having the additional folder that does not change from the tests done in /feature-issue676.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FEAT: Option to add copyright to SBOM result

2 participants