feat: write sbom result to disk#822
Conversation
Codecov Report
@@ Coverage Diff @@
## main #822 +/- ##
==========================================
- Coverage 51.37% 51.10% -0.27%
==========================================
Files 44 44
Lines 3354 3381 +27
==========================================
+ Hits 1723 1728 +5
- Misses 1408 1428 +20
- Partials 223 225 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| } | ||
|
|
||
| if bo.SBOMDir != "" { | ||
| if bo.SBOMDir == "." { |
There was a problem hiding this comment.
We shouldn't special-case ., since users might also do ../foo etc. Look into what --image-refs does, so we're at least consistent. If that doesn't support .. etc (I doubt it does), that's okay, but we should be consistent.
| if g.sbomDir != "" { | ||
| fp := fmt.Sprintf("%s.%s", filepath.Join(g.sbomDir, appFilename(ref.Path())), g.sbomType) | ||
| log.Printf("Writing an SBOM to %s", fp) | ||
| if err := os.WriteFile(fp, sbom, 0644); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this be part of the sbomber impl's responsibility? It already knows the type of SBOM it's generating, and can also know its file name extension.
There was a problem hiding this comment.
yes, it can but wouldn't it cause code duplication? Because I need to add this logic into all of the somber impls. WDYT?
There was a problem hiding this comment.
I updated the code according to your feedback, can you please take a look? Thx
developer-guy
force-pushed
the
feature/728
branch
4 times, most recently
from
7c85f0c to
1395e4c
Compare
| -P, --preserve-import-paths Whether to preserve the full import path after KO_DOCKER_REPO. | ||
| --push Push images to KO_DOCKER_REPO (default true) | ||
| --sbom string The SBOM media type to use (none will disable SBOM synthesis and upload, also supports: spdx, cyclonedx, go.version-m). (default "spdx") | ||
| --sbom-dir string The directory to write the SBOM to. |
There was a problem hiding this comment.
| --sbom-dir string The directory to write the SBOM to. | |
| --sbom-dir string Path to file where the SBOM will be written. |
I'd like to think about making this more consistent with --image-refs if possible, which takes a path to a file, instead of a directory.
That may bite us later if/when we end up generating multiple SBOMs, but I'm not sure we'll do that anyway. If we do, we can only write the first one to disk or something. WDYT?
There was a problem hiding this comment.
I'd like to think about making this more consistent with --image-refs if possible, which takes a path to a file instead of a directory.
That's also ok for me. It should change the logic of determining the file path, that's all (I guess?). But what if I'm building more than one binary configured via the builds section?
There was a problem hiding this comment.
Yeah good point. You could be ko resolveing, or eventually ko build ./...ing, so there might be multiple SBOMs.
In that case a dir sounds better, with files named after the app. So let's keep it like this.
There was a problem hiding this comment.
Is there anything left that you want me to do? Or are we all good 🕺🏻
There was a problem hiding this comment.
What do you think about adding an e2e test in e2e.yaml? You could even have it --push=false with --sbom-dir then check that the file exists and can be read with jq . <file>.
There was a problem hiding this comment.
I have added a test, can you please take a look at them? Thx
developer-guy
force-pushed
the
feature/728
branch
2 times, most recently
from
adcfc0d to
5fe3926
Compare
developer-guy
force-pushed
the
feature/728
branch
8 times, most recently
from
483642f to
47087bb
Compare
developer-guy
force-pushed
the
feature/728
branch
5 times, most recently
from
fb4374c to
aa6219b
Compare
developer-guy
force-pushed
the
feature/728
branch
from
aa6219b to
45b86f5
Compare
developer-guy
force-pushed
the
feature/728
branch
from
45b86f5 to
39ab81a
Compare
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
developer-guy
force-pushed
the
feature/728
branch
from
39ab81a to
b57104e
Compare
3 participants
Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com
Fixes #728