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

[Issue 3759] Support direct buildkit builds without requiring a docker daemon on the build host #4504

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

valerian-roche
Copy link

This PR is currently a draft and does not aim at being merged as is, but I'd like feedback on:

  • is this feature potentially going to be merged?
  • is this okay to add it as a separate entry from docker? The docker part is strictly written as a distinct build/push/manifest system which is not relevant here (as buildx build covers all three). I initially wanted to merge the configuration part but it ended up far too confusing

If this is the correct direction, I'll go on with:

  • fix the run when running goreleaser build (it's currently simply skipped). Build can still be tested through release --snapshot. I'll potentially end up with release --snapshot not using --load and build using --load and failing if a daemon is not present
  • add unittests
    For now I was able to test:
  • successful build only through release --snapshot, including when setting multiple architectures
  • successful push with manifest when running release, including with multiple platforms
  • successful signing of images and manifests

@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 26, 2023
@caarlos0
Copy link
Member

Hey, thanks for the PR! 🤘

This PR is currently a draft and does not aim at being merged as is, but I'd like feedback on:

  • is this feature potentially going to be merged?

Yes! It's something I've been meaning to work in for a while now as well.

  • is this okay to add it as a separate entry from docker? The docker part is strictly written as a distinct build/push/manifest system which is not relevant here (as buildx build covers all three). I initially wanted to merge the configuration part but it ended up far too confusing

Yes. I would probably call it containers instead of docker_buildkits though, as it would allow us to later use the same configuration for other daemonless builders (e.g. kaniko #2322)

I see some code is copied from the docker pipe, maybe we should put it in a lib in the internal package for reuse?

@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 28, 2023
@valerian-roche
Copy link
Author

Yes. I would probably call it containers instead of docker_buildkits though, as it would allow us to later use the same configuration for other daemonless builders (e.g. kaniko #2322)

I updated the PR to use containers and allow the definition of more builders along the way.
I also now support three buildkit modes (the ones I can test):

  • basic docker builder, though it can't do multi-arch pushes for now
  • local docker builder with experimental containerd image store. It's able to do single-step, multi-arch manifests pushes
  • remote kubernetes builder

A missing step today is how to distinguish a goreleaser build run from a goreleaser release run, as I want to always build and import the images for the first one, and only build if not pushing (snapshot) in the second case.

If this looks okay I'll work on the unit tests

@caarlos0
Copy link
Member

A missing step today is how to distinguish a goreleaser build run from a goreleaser release run, as I want to always build and import the images for the first one, and only build if not pushing (snapshot) in the second case.

I don't think we have that now... but seems like something good to have. Will PR it ASAP.

caarlos0 added a commit that referenced this pull request Dec 29, 2023
refs #4504

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
…gh without multi-platform), docker with containerd image store and kubernetes builders
@valerian-roche
Copy link
Author

valerian-roche commented Dec 29, 2023

@caarlos0 Thanks for the commit. I updated the PR with:

  • Common code between docker and container for the build context and platform part
  • Proper handling of multi-arch builds. This requires supporting a new binary path when running multi-arch builds. For compatibility the current root artifacts are still present for single arch builds

If this looks okay to you I'll start working on the tests once you confirm this would be going this direction. I'll also likely split the PR in multiple stacked one if you're okay with me using graphite

@caarlos0
Copy link
Member

hey, sorry for the delay, this does look good to me so far, yes!

thanks for all the work and effort here btw 🙏

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 421 lines in your changes are missing coverage. Please review.

Comparison is base (12469c4) 83.83% compared to head (c009954) 81.32%.
Report is 35 commits behind head on main.

Files Patch % Lines
internal/pipe/container/builder/buildkit.go 0.00% 231 Missing ⚠️
internal/pipe/container/container.go 5.66% 99 Missing and 1 partial ⚠️
internal/pipe/container/builder/helpers.go 0.00% 44 Missing ⚠️
internal/containers/context.go 77.87% 19 Missing and 6 partials ⚠️
internal/containers/platform.go 78.72% 10 Missing ⚠️
internal/pipe/container/builder/api.go 0.00% 5 Missing ⚠️
internal/gio/link.go 50.00% 2 Missing and 1 partial ⚠️
internal/pipe/docker/docker.go 86.36% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4504      +/-   ##
==========================================
- Coverage   83.83%   81.32%   -2.52%     
==========================================
  Files         135      142       +7     
  Lines       12850    13314     +464     
==========================================
+ Hits        10773    10827      +54     
- Misses       1652     2056     +404     
- Partials      425      431       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AlbertoBarba
Copy link

Hey @valerian-roche, is there anything i can do to help u completing this PR?

@valerian-roche
Copy link
Author

Hey, sorry for the delay. I have had conflicting priorities and haven't had anytime to work on unit testing this sadly.
From my manual testing it works:

  • local run on my laptop with a daemon and remote build
  • local run with local daemon and local build
  • CI run with no daemon and remote build

If you have some bandwidth to work on testing that'd be great, otherwise I plan on coming back to the PR later on when I'll have more capacity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants