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

Provide more information in get_components_without_* functions #169

Merged
merged 2 commits into from
Dec 28, 2023

Conversation

CsatariGergely
Copy link
Contributor

Optionally return both the name and the SPDXID of problematic elements in get_components_without_* functions.

Closes #168

Optionally return both the name and the SPDXID of problemative elements in get_components_without_* functions

Signed-off-by: Gergely Csatari <gergely.csatari@nokia.com>
@CsatariGergely
Copy link
Contributor Author

One thing what I could not decide if should all the get_components_without_* functions have the returnTuples option? get_components_without_names would add None to the name while get_components_without_identifiers would add None to the SPDXID.
I was not able to create tests to test get_components_without_identifiers.

@jspeed-meyers jspeed-meyers added enhancement New feature or request bug Something isn't working labels Dec 23, 2023
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 for this PR. Two small nits: the CI is failing because of a formatting issue and a lint issue. Can you please fix those? black is the formatter this project uses. pylint is the linter this project uses.

See my questions in the original issue for a more substantive discussion. Regardless though, this is a nice and clear PR. Thank you!

I look forward to discussing this change, or a very similar change, more.

tests/test_checker.py Outdated Show resolved Hide resolved
Comment on lines 254 to 256
# TODO: Not sure how to test this. If any package misses the SPDXID the whole file seems to be invalid.
#components = sbom.get_components_without_identifiers()
#assert components == ["glibc-no-identifier"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this is un-testable, I also think, given the constraints of the spdx-tools python package that powers this project. I've run into this "problem" too. Since I think that's the case, it's probably just worth explaining this situation in the comment directly to help aid future developers.

@@ -74,16 +74,19 @@ def get_components_without_names(self):
components_without_names.append(package.spdx_id)
return components_without_names

def get_components_without_versions(self):
"""Retrieve SPDX ID of components without names."""
def get_components_without_versions(self, returnTuples=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my question in the original issue about whether the option should be returnTuples or something more like returnSDPXIDs. If you think returnTuples is indeed the right way to go, then this code looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my use case the tuples would be preferable. I added a longer explanation to here

ntia_conformance_checker/sbom_checker.py Show resolved Hide resolved
Signed-off-by: Gergely Csatari <gergely.csatari@nokia.com>
@CsatariGergely
Copy link
Contributor Author

Thank you for this PR. Two small nits: the CI is failing because of a formatting issue and a lint issue. Can you please fix those? black is the formatter this project uses. pylint is the linter this project uses.

I've fixed these in dc3c53d. Maybe black and pylint could be mentioned in CONTRIBUTING.md just to avoid extra GitHub Action cycles.

@jspeed-meyers
Copy link
Collaborator

jspeed-meyers commented Dec 28, 2023

Maybe black and pylint could be mentioned in CONTRIBUTING.md just to avoid extra GitHub Action cycles.

Yes, definitely. Good call. Issue created: #170

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.

Nice! Thank you, @CsatariGergely!

@jspeed-meyers
Copy link
Collaborator

@goneall: Can you please take a look too when you have a moment? 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.

LGTM - I like the approach of the optional parameter, should keep things compatible

@jspeed-meyers jspeed-meyers merged commit a4a4ba2 into spdx:main Dec 28, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_components_without_* functions shold return the SPDX ID of the component if there is one
3 participants