Simplify the SBOM writer interface #1892
Merged
Conversation
This file contains 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
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
kzantow
approved these changes
Jun 23, 2023
Comment on lines
+117
to
+123
| type nopWriteCloser struct { | ||
| io.Writer | ||
| } | ||
|
|
||
| func (n nopWriteCloser) Close() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: organization -- it would probably be good to keep all the sbomMultiWriter stuff together
There was a problem hiding this comment.
yeah, this file got a little awkward, but believe it or not the #1888 PR is about to remove this (the nopWriteCloser) since it will write the the event bus. I split out this work to make the bubbletea PR smaller :)
This was referenced Jun 29, 2023
Merged
Closed
GijsCalis
pushed a commit
to GijsCalis/syft
that referenced
this pull request
Feb 19, 2024
* remove sbom.writer bytes call and consolidate helpers to options pkg Signed-off-by: Alex Goodman <alex.goodman@anchore.com> * dont close stdout Signed-off-by: Alex Goodman <alex.goodman@anchore.com> * remove close operation from multiwriter Signed-off-by: Alex Goodman <alex.goodman@anchore.com> --------- Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
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.
This PR attempts to simplify the
sbom.Writerinterface by removing required methods and consolidating helper functions for writers to thecmd/optionspackage.This reduces the current interface:
To this:
The primary reasons being:
Close()isn't necessary. TheWrite()call should implicitly close any destination.Bytes(...) []bytes ...is ultimately a reading call, not a writing call, which is the opposite meaning of this interfaceAdditionally, many of the helpers around making writers are in the same
sbompackage, however, these helpers seem primarily targeted towards the CLI argument parsing, which should reside under thecmdpackage. This is why many of the writer helpers have been moved tocmd/options.This change cause downstream changes necessary in the
attestcommand so took additional time simplifying sbom.Writer usage in that command.Partially addresses syft 1.0 work.