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

Find the DESCRIBES relationship by looking through attached packages #189

Merged
merged 4 commits into from
Jun 29, 2024

Conversation

DanielOjalvo
Copy link
Contributor

No description provided.

sounds reasonable

Co-authored-by: John Speed Meyers <[email protected]>
# Check if any of the "DESCRIBES" relationships describe a Package
describes_package = any(
"Package" in rel.related_spdx_element_id for rel in describes_relationships
rel.related_spdx_element_id in spdx_id_set for rel in describes_relationships
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is certainly superior. Thank you!

@jspeed-meyers
Copy link
Collaborator

@DanielOjalvo, thank you! Can you please use black and pylint to fix the minor formatting issues?

You can get more information here.

@jspeed-meyers
Copy link
Collaborator

QUESTION: Is this a breaking change or not? The tests pass, but I worry changing the internal logic of a key function SHOULD be a breaking change. Hmmm. Thoughts from anyone?

@jspeed-meyers jspeed-meyers linked an issue Jun 10, 2024 that may be closed by this pull request
@jspeed-meyers
Copy link
Collaborator

One other question, which was raised in the discussion in #186: Is the intent of this check actually to determine if there is at least one root node in an SPDX document? This question and its answer is probably out of scope for this PR and issue. To me, it sounds like this should be a discussion of how the SPDX NTIA mapping document defines this check. I worry that the written definition ("The document must DESCRIBES at least one package") and the intent I read in the related issue thread aren't the same, but maybe I am confused. (I am often confused!)

Anyways, I wanted to discuss this before merging this change.

@jspeed-meyers
Copy link
Collaborator

@goneall: Can you please review this PR too? For the actual code review, can you please take a narrow view, simply checking if the logic of the code matches the current written intent of the SPDX NTIA mapping? And for answering the other questions, can you please your broad SPDX expertise and advise? Thank you, as always.

Copy link
Collaborator

@jspeed-meyers jspeed-meyers left a comment

Choose a reason for hiding this comment

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

THANK YOU!

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

I agree this is a more precise approach validating the NTIA minimum is being followed. There may be cases where the described relationship is only to a file which, in my opinion, would not be a valid NITA minimum SBOM since it doesn't describe a package.

@goneall
Copy link
Member

goneall commented Jun 15, 2024

cc: @kestewart - Kate - let me know if you agree / disagree with my above conclusion.

@jspeed-meyers
Copy link
Collaborator

jspeed-meyers commented Jun 15, 2024

Thank you, @goneall. I'll give Kate time to weigh in before merging. [EDITED MY COMMENT. I confused myself. Please ignore.]

@jspeed-meyers
Copy link
Collaborator

I'll merge on Friday, June 28, 2024 unless I hear any objections. I won't cut a new release (3.0.0 I suppose) immediately. I'll wait a month or so to see if there are any other bugs that are breaking changes that emerge.

@jspeed-meyers jspeed-meyers merged commit 3b485ac into spdx:main Jun 29, 2024
6 checks passed
@jspeed-meyers jspeed-meyers mentioned this pull request Jun 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check_dependency_relationships test does not seem correct
3 participants