-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: validate now accepts components with external bom #308
Conversation
077d948
to
484cd14
Compare
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.
@mmarseu I faced two issues while testing: If e.g. version
is empty, an error for the component is still thrown, because the if
is after the try-catch
, where you throw the warning, contradicting the statement from the warning.
Further: if the url
is not a valid bom-link
as described on the cdx website, respectively specification, you ignore it. So I could write some gibberish, even just an empty url
and still would ignore the validation. Though this hopefully is only an issue if providing an externalReference
manually.
I don't really see the problem with this particular behavior. If you have an empty But your argument brought me to a larger issue I hadn't considered. Obviously, the user shouldn't be required to include all of the mandatory fields in a component which references an external SBOM. That is the entire point of this PR. AFAIK, this applies to:
That is by design. Bom-linking isn't the only way to reference an external SBOM. As far as I am concerned, a URL is also fine. The stock CDX spec already ensures that the URL is either a valid IRI-reference or a bom-link. Unfortunately, it is actually not easy to find a string that isn't a valid IRI-reference, so it's hard to verify that this works. Try |
8563c66
to
63d2d3a
Compare
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.
lgtm, just rebase it please, then I will approve it 😁
00c8e0b
to
4906cc5
Compare
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.
now lgtm
The What of this PR is straight-forward: It makes the
validate
command accept components als valid which have an external reference of typebom
, even if those components do not fulfil the requirements of our custom schema.The program now outputs a warning to inform the user that the externally referenced BOM cannot be checked.
The How merits a little explanation, I guess, to help with the review. Because the schema diffs look much more confusing than they are.
The custom schemas now wrap all of our custom requirements on components in a big if-then-else construct, where the if tests whether the component has an external reference to a BOM.
version
below)This marker requirement can be detected in the validation routine. When jsonschema produces an error for this particular marker, the tool only writes a warning but otherwise ignores the error.
Note on
version
: If the removal of theversion
requirements trips you up when you see it all the way at the top of the diff, don't worry. I have only moved it into the else clause, because an externally referenced BOM would be where the version is declared.Misc changes:
pytest-subtests
, a small pytest plugin which enables support for unittest's subtest feature when running the tests through pytest. The alternative would be to either not use subtests (but I think the feature is very useful in this case to run tests against all spec versions at once) or to rewrite the tests to native pytest which has a comparable feature.