Adding a new merge-sboms sub-command #593
Conversation
|
Overall approach LGTM 👍 |
d8b7445 to
3c23f2f
Compare
a6ed34d to
615cc59
Compare
| # i.e. outside of handle_errors() decorator. Simply raising | ||
| # an exception here will not produce correct exit code, thus | ||
| # the explicit call to exception wrapper. | ||
| _bail_out_with_error(InvalidInput("Need at least two different SBOM files")) |
There was a problem hiding this comment.
Raising InvalidInput would have the same effect as wrapping it with _bail_out_with_error here, right? (considering the command is annotated with @handle_errors). This would avoid the need to extracting out _bail_out_with_error.
In case you prefer to keep it extracted, I'd say doing it in a separate commit would be better (but raising the exception here in any case seems more consistent to me).
There was a problem hiding this comment.
No, it won't: with a raise exit code is 1, with the wrapper it is set to 2 for Cachi2Errors. This exception will happen before arguments are passed to the decorated function, thus outside of handle_error scope.
There was a problem hiding this comment.
You're right. I got confused because, when I tested it, it errored on {sbom_file} does not appear to be a valid Cachi2 SBOM, which is already within the decorator's scope.
So a small nitpick is to extract _bail_out_with_error in a commit prior to the main one, but it already LGTM the way it is.
Functionality for merging SBOMs was already present in the codebase, this commit exposes it to a user. Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>
f6198fd
Functionality for merging SBOMs was already present in the codebase, this commit exposes it to a user.
Maintainers will complete the following section
Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:
/ok-to-test(as is the standard for Pipelines as Code)