Split the sbom.Format interface by encode and decode use cases#2186
Merged
Split the sbom.Format interface by encode and decode use cases#2186
Conversation
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
3c7aa51 to
e92c3a1
Compare
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
kzantow
reviewed
Oct 6, 2023
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
kzantow
previously approved these changes
Oct 13, 2023
Contributor
kzantow
left a comment
There was a problem hiding this comment.
Overall, I think this looks good. 🎉
The one general comment I had is that I don't think we should have nearly as many Default functions for this stuff. I think it should be either: a) a New constructor, or b) another constructor function that takes a configuration. Providing multiple ways to get default config AND default format encoders confuses library consumers, when they should be using the configs and constructors, not any of the default functions (aside from getting syft lib config defaults).
Lastly, I would avoid the static defaults on the decoders, too, so this doesn't have to also later get refactored to get configuration passed in.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
agreed to sync later next week about more changes
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
GijsCalis
pushed a commit
to GijsCalis/syft
that referenced
this pull request
Feb 19, 2024
…re#2186) * split up sbom.Format into encode and decode ops Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * update cmd pkg to inject format configs Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * bump cyclonedx schema to 1.5 Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * redact image metadata from github encoder tests Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * add more testing around format decoder identify Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * add test case for format version options Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix cli tests Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix CLI test Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * [wip] - review comments Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * keep encoder creation out of post load function Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * keep decider and identify functions Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * add a few more doc comments Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * remove format encoder default function helpers Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * address PR feedback Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * move back to streaming based decode functions Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * with common convention for encoder constructors Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix tests and allow for encoders to be created from cli options Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix cli tests Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix linting Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * buffer reads from stdin to support seeking Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Today there is a single unified
sbom.Formatinterface allowing for identification, encoding, and decoding use cases. This PR splits this interface down the middle into two use cases: encoding and decoding (where encoding would subsume the identification use case).Why do this?
Primarily, there is one core problem with how formats are constructed today: they do not allow for injection of configuration. This means that if there are any specific behavioral differences that are desired for how an SBOM is presented then the abstraction must be broken to allow for injecting that information (this is done today for templating).
Secondarily, the abstraction can lead to bad assumptions for the decoding use case. The encoding use case requires a format ID and specific version, however, when decoding neither is really needed. That is, given a pile of bytes an object should be able to produce and SBOM or not. Having a single object where you have a strong ID and version coupled with the decoder makes it seem like you need a format object per-version-per-format-id in order to work, but this is not the case.
Changes made in this PR:
sbom.Formatinterface intosbom.FormatEncoderandsbom.FormatDecoderinterfaces.Validateformat functions withIdentify, which more closely tracks the reason for this functions existence.sbom.Decoderandsbom.Encoderdefinitions. The names are not reused to prevent consumer confusion on this breaking change.sbom.FormatEncoderconstruction in theoption.Outputobject, allowing for application configuration to be injected into the objects that are used to encode SBOMs (the main driver behind this change).option.MultiOutputandoption.SingleOutputinto a singleoption.Outputobject. Both original objects have a need to craft encoders when makingsbom.Writerobjects. This refactor was done to make this addition simpler.formatspackage toformatto be consistent with other lib packages.testutilto be able to reuse the same fixture file for all format tests.Note for reviewers
the bump to the cyclonedx schemas make the PR diff look much worse than it is:
Fixes #2112