Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More SBOM logic improvements #1459

Merged
merged 8 commits into from
Aug 28, 2024
Merged

Conversation

luhring
Copy link
Contributor

@luhring luhring commented Aug 28, 2024

This PR continues to improve the state of melange's SBOM code:

  1. Fixes the issue where our external references used PACKAGE_MANAGER instead of PACKAGE-MANAGER.
  2. Breaks apart "SBOM generation" logic from "writing the SBOM back into the APK's filesystem" logic. The hope is we can then use this seam to implement tests in the near future.

Copy link
Member

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

super nitty nits, but are there tests that exist? Could we add some if they don't?

pkg/sbom/implementation.go Outdated Show resolved Hide resolved
pkg/sbom/implementation.go Outdated Show resolved Hide resolved
pkg/sbom/implementation.go Outdated Show resolved Hide resolved
pkg/build/build.go Show resolved Hide resolved
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
@luhring
Copy link
Contributor Author

luhring commented Aug 28, 2024

super nitty nits, but are there tests that exist? Could we add some if they don't?

@vaikas There are no tests, and there haven't ever been as far as I can tell. 😞

We can add an E2E test that asserts the SBOM in an APK against a goldenfile or something. From what I can tell there's not yet a great place to put in tests for our SBOM logic, because of how tangled the data flow is throughout these giant structs that depend on mutations using data from all sorts of places. But I think this PR makes progress toward a more "testable" state, FWIW.

vaikas
vaikas previously approved these changes Aug 28, 2024
Copy link
Member

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Thank you!

jdolitsky
jdolitsky previously approved these changes Aug 28, 2024
There appears to be an upstream issue with this package that should not block melange CI: https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/515

Also, it looks like this has happened before, see melange PR chainguard-dev#1143 that was later reverted in PR chainguard-dev#1147.

Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
@luhring luhring dismissed stale reviews from jdolitsky and vaikas via c932c79 August 28, 2024 19:22
@luhring luhring enabled auto-merge August 28, 2024 19:28
@luhring luhring merged commit 5507bf0 into chainguard-dev:main Aug 28, 2024
35 checks passed
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.

3 participants