use `build` as common API for build scenarios #10374

ndeloof · 2023-03-15T16:23:07Z

What I did
from various places we trigger builds (build, up and watch commands), use composeService#build as 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.Options for buildkit builder

To get up to only build selected platform, prepareProjectForBuild mutate the project to remove unnecessary build config so that the exact same build function can be used both by up and build.
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.Options to configure classic builder

(not mandatory) A picture of a cute animal, if possible in relation to what you did

codecov · 2023-03-15T18:24:29Z

Codecov Report

Patch coverage: 85.27% and project coverage change: +0.03 🎉

Comparison is base (03f0ed1) 53.41% compared to head (c48bc95) 53.44%.

❗ Current head c48bc95 differs from pull request most recent head 7a57e2c. Consider uploading reports for the commit 7a57e2c to get more accurate results

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

Impacted Files	Coverage Δ
pkg/compose/watch.go	`0.00% <0.00%> (ø)`
pkg/compose/build_classic.go	`51.94% <80.64%> (-0.97%)`	⬇️
pkg/api/api.go	`49.05% <86.95%> (+29.05%)`	⬆️
pkg/compose/build.go	`76.71% <91.54%> (+0.43%)`	⬆️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

laurazard · 2023-03-16T13:24:02Z

Yessss this was a refactor that was really needed ❤️ I'll take a better look later + approve

Signed-off-by: Nicolas De Loof <[email protected]>

milas

✨ 🧹 👍🏻

milas · 2023-03-21T13:37:10Z

pkg/compose/build.go

-			if err != nil {
-				return err
+
+			if buildkitEnabled, err := s.dockerCli.BuildKitEnabled(); err != nil || !buildkitEnabled {


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.

milas · 2023-03-21T13:42:12Z

pkg/compose/build.go

-		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)


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

we only expose those flags for build command. up --build only applies options set by compose file

milas · 2023-03-21T13:46:19Z

pkg/compose/build.go

+		NoCache:     service.Build.NoCache || options.NoCache,
+		Pull:        service.Build.Pull || options.Pull,


This is also done when mutating the project & service in api.BuildOptions::Apply()

Do we need both?

yes indeed, reminiscence from an earlier attempt, before I moved it all to Apply :)

milas · 2023-03-21T13:47:32Z

pkg/compose/build_classic.go

-	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")


Suggested change

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")

milas · 2023-03-21T13:49:09Z

pkg/compose/build_classic.go

@@ -252,25 +211,18 @@ func isLocalDir(c string) bool {
 	return err == nil
 }

-func imageBuildOptions(options buildx.Options) dockertypes.ImageBuildOptions {
+func imageBuildOptions(config *types.BuildConfig) dockertypes.ImageBuildOptions {


Perhaps rename classicBuildOptions()

this produces the options for the ImageBuild engine API, so ...

milas · 2023-03-21T13:50:46Z

pkg/compose/build_test.go

+	"gotest.tools/v3/assert"
+)
+
+func TestPrepareProjectForBuild(t *testing.T) {


💯 for these tests! We definitely need more unit test coverage for some of these complex scenarios like platforms, so this is awesome

glours

LGTM

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the build_refactor branch 4 times, most recently from eefed69 to be48461 Compare March 15, 2023 18:15

ndeloof force-pushed the build_refactor branch 2 times, most recently from 1d219b1 to 1d7be81 Compare March 16, 2023 11:07

ndeloof marked this pull request as ready for review March 16, 2023 12:32

ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, glours, milas and laurazard and removed request for a team March 16, 2023 12:34

ndeloof force-pushed the build_refactor branch 2 times, most recently from f0d360b to 58d5796 Compare March 17, 2023 08:01

use build as common API for build scenarios

95f6b3d

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the build_refactor branch from 58d5796 to c48bc95 Compare March 21, 2023 11:20

milas approved these changes Mar 21, 2023

View reviewed changes

glours approved these changes Mar 21, 2023

View reviewed changes

Run classic builder with BuildConfig, not buildx.Options

7a57e2c

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the build_refactor branch from c48bc95 to 7a57e2c Compare March 21, 2023 14:06

ndeloof merged commit 6c1f06e into docker:v2 Mar 21, 2023

ndeloof deleted the build_refactor branch March 21, 2023 14:37

johanneswuerbach mentioned this pull request Apr 24, 2023

fix: remove invalid cache-from registry entries #10135

Closed

komima mentioned this pull request Jul 12, 2023

[BUG] Proxy config not applied as build args with classic builder #10803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `build` as common API for build scenarios #10374

use `build` as common API for build scenarios #10374

ndeloof commented Mar 15, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

laurazard commented Mar 16, 2023

milas left a comment

milas Mar 21, 2023

milas Mar 21, 2023

ndeloof Mar 21, 2023

milas Mar 21, 2023

ndeloof Mar 21, 2023 •

edited

Loading

milas Mar 21, 2023

milas Mar 21, 2023

ndeloof Mar 21, 2023

milas Mar 21, 2023

glours left a comment

		NoCache: service.Build.NoCache \|\| options.NoCache,
		Pull: service.Build.Pull \|\| options.Pull,

	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")

use build as common API for build scenarios #10374

use build as common API for build scenarios #10374

Conversation

ndeloof commented Mar 15, 2023 • edited Loading

codecov bot commented Mar 15, 2023 • edited Loading

Codecov Report

laurazard commented Mar 16, 2023

milas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glours left a comment

Choose a reason for hiding this comment

use `build` as common API for build scenarios #10374

use `build` as common API for build scenarios #10374

ndeloof commented Mar 15, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

ndeloof Mar 21, 2023 •

edited

Loading