-
Notifications
You must be signed in to change notification settings - Fork 13
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
Generate SBOM using buildkit's builtin mechanism #83
Generate SBOM using buildkit's builtin mechanism #83
Conversation
Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
DCO signoff is missing. |
@eriknordmark it is not missing for this PR. There are 2 old commits with wrong sign-off string and I'm not sure what to do about it. Summary Commit sha: 0c3194d, Author: Mikhail Malyshev, Committer: Mikhail Malyshev; Expected "Mikhail Malyshev mikem@zededa.com", but got "Nikolay Martyanov nikolay@zededa.com". |
That's odd. So the DCO checker is broken? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except that linuxkit cache push
is not yet implemented. Working on it. Real Soon Now ™️
b3f5f9b
to
16ec9f3
Compare
tried |
- --sbom=true is not compatible with --load because docker doesn't support full OCI spec. Instead we export TAR file in OCI format and later load it linuxkit cache - since the image is now hosted by linuxkit cache we should use 'cache push' command to push it to dockerhub registry Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
16ec9f3
to
6353f50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Did you build it and do you see the correct sbom info? You should be able to look into the cache directly and see it.
@yash-zededa it seems we need to upgrade buildkit on github runner. https://github.com/lf-edge/eve-kernel/actions/runs/8128820721/job/22215156330?pr=83
|
nvm, we are running the latest buildkit anyway. |
Makefile.eve
Outdated
@@ -47,16 +47,21 @@ help: Makefile | |||
@echo " clean: remove generated files" | |||
@echo | |||
|
|||
.PHONY: ensure-builder | |||
ensure-builder: | |||
docker builder inspect eve-kernel-builder || docker builder create --name eve-kernel-builder --driver docker-container --bootstrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend saving the builder to a Makefile var and using that here and in the build target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend redirecting the output of inspect
to /dev/null
, or it gets messy. You don't care about the output, only if it returns 0 or 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an @
at the beginning of the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend saving the builder to a Makefile var and using that here and in the build target
this was just a POC :). Will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend redirecting the output of
inspect
to/dev/null
, or it gets messy. You don't care about the output, only if it returns 0 or 1.
it may be useful to troubleshoot issues with CI/CD in future
Makefile.eve
Outdated
@echo "Building kernel version $(BRANCH):$(VERSION)-$* with compiler $*" | ||
docker buildx build \ | ||
--builder=eve-kernel-builder \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, use a var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deitch should we fix a build-kit version to something like 0.12.5 ? I could name --builder eve-kernel-builder-$(BK_VERSION)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is a good idea.
Our build may depend on buildx features e.g. --sbom so we need to make sure we have a correct builder version. This is not a big problem for local build but a big one for GH runners. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
94aa2db
to
732e980
Compare
|
@deitch @rene @eriknordmark it seems all issues are now solved, we can merge this PR if there are no objections |
ae347d3
into
eve-kernel-amd64-v6.1.38-generic
linuxkit cache push
is not implemented yet see Add 'cache push' command to push images by tag linuxkit/linuxkit#3990linuxkit cache push
requires one tiny fix to avoid extra image tag creation when using cache push, make push of arch-specific tags optional linuxkit/linuxkit#3996This PR should address #50