use build as common API for build scenarios#10374
Conversation
eefed69 to
be48461
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## v2 #10374 +/- ##
==========================================
+ Coverage 53.41% 53.44% +0.03%
==========================================
Files 104 104
Lines 8942 8919 -23
==========================================
- Hits 4776 4767 -9
+ Misses 3646 3632 -14
Partials 520 520
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
1d219b1 to
1d7be81
Compare
|
Yessss this was a refactor that was really needed ❤️ I'll take a better look later + approve |
f0d360b to
58d5796
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
pkg/compose/build.go
Outdated
| if err != nil { | ||
| return err | ||
|
|
||
| if buildkitEnabled, err := s.dockerCli.BuildKitEnabled(); err != nil || !buildkitEnabled { |
There was a problem hiding this comment.
Minor: it might be good to bring this out of the "loop" (InDependencyOrder)
Also, I think it's probably safe to fail and propagate the error:
https://github.com/docker/cli/blob/3c9e0073dd3ab31783a9d27d5fc26b70d59db367/cli/command/cli.go#L171 (tl;dr it can only fail if you do something like DOCKER_BUILDKIT=not_a_bool
Or, at the very least, a warning that we're falling back would be nice.
| if service.Image == "" && service.Build == nil { | ||
| return nil, fmt.Errorf("invalid service %q. Must specify either image or build", service.Name) | ||
| func (s *composeService) prepareProjectForBuild(project *types.Project, images map[string]string) error { | ||
| err := api.BuildOptions{}.Apply(project) |
There was a problem hiding this comment.
Do we need to actually pass the api.BuildOptions down?
In Apply we use Pull / NoCache, which I assume are for the global CLI --pull & --no-cache flags respectively
There was a problem hiding this comment.
we only expose those flags for build command. up --build only applies options set by compose file
pkg/compose/build.go
Outdated
| NoCache: service.Build.NoCache || options.NoCache, | ||
| Pull: service.Build.Pull || options.Pull, |
There was a problem hiding this comment.
This is also done when mutating the project & service in api.BuildOptions::Apply()
Do we need both?
There was a problem hiding this comment.
yes indeed, reminiscence from an earlier attempt, before I moved it all to Apply :)
pkg/compose/build_classic.go
Outdated
| if len(options.Inputs.NamedContexts) > 0 { | ||
| return "", errors.Errorf("this builder doesn't support additional contexts, set DOCKER_BUILDKIT=1 to use BuildKit which does") | ||
| if len(service.Build.Secrets) > 0 { | ||
| return "", errors.Errorf("the classic builder doesn't support Secrets, set DOCKER_BUILDKIT=1 to use BuildKit") |
There was a problem hiding this comment.
| return "", errors.Errorf("the classic builder doesn't support Secrets, set DOCKER_BUILDKIT=1 to use BuildKit") | |
| return "", errors.Errorf("the classic builder doesn't support secrets, set DOCKER_BUILDKIT=1 to use BuildKit") |
| } | ||
|
|
||
| func imageBuildOptions(options buildx.Options) dockertypes.ImageBuildOptions { | ||
| func imageBuildOptions(config *types.BuildConfig) dockertypes.ImageBuildOptions { |
There was a problem hiding this comment.
Perhaps rename classicBuildOptions()
There was a problem hiding this comment.
this produces the options for the ImageBuild engine API, so ...
| "gotest.tools/v3/assert" | ||
| ) | ||
|
|
||
| func TestPrepareProjectForBuild(t *testing.T) { |
There was a problem hiding this comment.
💯 for these tests! We definitely need more unit test coverage for some of these complex scenarios like platforms, so this is awesome
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
What I did
from various places we trigger builds (
build,upandwatchcommands), usecomposeService#buildas the single entry point into the (complex) build logic.This allows to manage classic vs buildkit at early stage, and to pass the BuildConfig to classic builder, while we only get it converted into
buildx.Optionsfor buildkit builderTo get
upto only build selected platform,prepareProjectForBuildmutate theprojectto remove unnecessary build config so that the exact samebuildfunction can be used both byupandbuild.Added a test case to make it obvious about this mutation and prevent any regression next time we try to change this codebase :)
Related issue
see #10056 for illustration of the issue using
buildx.Optionsto configure classic builder(not mandatory) A picture of a cute animal, if possible in relation to what you did
