Skip to content

fix: do not create SBOM for packages without SBOMable material#3540

Merged
AustinAbro321 merged 7 commits intomainfrom
fix-sbom
Feb 24, 2025
Merged

fix: do not create SBOM for packages without SBOMable material#3540
AustinAbro321 merged 7 commits intomainfrom
fix-sbom

Conversation

@AustinAbro321
Copy link
Member

@AustinAbro321 AustinAbro321 commented Feb 24, 2025

Description

do not create SBOM for packages without SBOMable material, looks like a bug from the create refactor

Related Issue

Fixes #3524

Checklist before merging

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321 AustinAbro321 requested review from a team as code owners February 24, 2025 15:00
@netlify
Copy link

netlify bot commented Feb 24, 2025

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit e501b7e
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/67bcc40a6949870009a79f86

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
}
}

l.Info("package successfully removed", "name", pkg.Metadata.Name)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this in here as an aside. Someone gave me feedback that zarf package remove feels strange since it doesn't give any confirmation that the remove happened successfully, I would agree

@codecov
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 23.52941% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/packager2/create.go 0.00% 9 Missing ⚠️
src/internal/packager2/layout/package.go 60.00% 2 Missing ⚠️
src/cmd/package.go 0.00% 1 Missing ⚠️
src/internal/packager2/remove.go 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/internal/packager2/layout/create.go 40.51% <100.00%> (+0.36%) ⬆️
src/cmd/package.go 42.87% <0.00%> (ø)
src/internal/packager2/remove.go 0.00% <0.00%> (ø)
src/internal/packager2/layout/package.go 40.51% <60.00%> (+0.36%) ⬆️
src/internal/packager2/create.go 0.00% <0.00%> (ø)

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrNoSBOMAvailable has a chance to return some debugging metadata from PackageLayout.GetSBOM. Recommend declaring it as a custom error type. Everything else looks good - test case especially.

require.NoError(t, err)
}

func TestGetSBOM(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +124 to +131
// ErrNoSBOMAvailable is returned when a user tries to access a package SBOM, but it is not available
var ErrNoSBOMAvailable = errors.New("zarf package does not have an SBOM available")

// GetSBOM outputs the SBOM data from the package to the give destination path.
func (p *PackageLayout) GetSBOM(destPath string) (string, error) {
if !p.Pkg.IsSBOMAble() {
return "", ErrNoSBOMAvailable
}
Copy link
Contributor

@mkcp mkcp Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: Let's declare ErrNoSBOMAvailable as a custom error type rather than a named error string. The message could pass back the package name or sha as metadata e.g. :

...
(e *NoSBOMAvailableError) Error() string {
    return fmt.Sprintf("zarf package does not have an SBOM available, package=%s", e.package)
}

https://gobyexample.com/custom-errors

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, will implement now

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AustinAbro321 AustinAbro321 added this pull request to the merge queue Feb 24, 2025
Merged via the queue into main with commit 318c3d3 Feb 24, 2025
26 checks passed
@AustinAbro321 AustinAbro321 deleted the fix-sbom branch February 24, 2025 19:57
nevinaragam pushed a commit to nevinaragam/zarf that referenced this pull request May 20, 2025
…dev#3540)

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: NevinAragam <nevin.aragam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Zarf always populates sbom.tar artifact even if no SBOMs exist

3 participants