Add Buildkit Option #434

guineveresaenger · 2022-12-03T00:16:01Z

This PR makes an opinionated choice about Docker Buildkit.
Background: The Docker client can run in two separate modes, BuilderV1 which is the classic Docker builder, and BuilderBuildKit, which is the version of the client that providers BuildKit support.
These modes are set via the Version field on the ImageBuildOptions. When this field is unset or nil, the Docker client runs in BuilderV1 mode by default.

In general, it appears as though folks are moving away from using the older Docker builder, and therefore this PR implements the buildkit mode as the default behavior for this resource.
Users may still set their builderVersion to the BuilderV1 mode, but must do so explicitly.
Invalid builder versions will return an error on pulumi up.

This functionality in v3.x.x was handled via the env field, which asked users to provide DOCKER_BUILDKIT=1 as an environment variable for the Docker CLI to pick up.
Since this version of Image relies on the Docker client, not the CLI, this particular field is no longer necessary in this form.
I believe making builderVersion its own schematized type field is a lot more user friendly, more tightly scoped, and allows us to explicitly document this feature.
Furthermore, we will be able to deprecate the env field safely, and it is additionally the hope of this PR author that builderVersion is much more intuitive for users than env

Fixes #422.

add buildkit types field
Add logic for Builder Version on client. Add input marshaling tests
Update Image description
Regenerate schema and SDKs

github-actions · 2022-12-03T00:19:54Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

viveklak · 2022-12-03T00:33:56Z

This functionality in v3.x.x was handled via the env field, which asked users to provide DOCKER_BUILDKIT=1 as an environment variable for the Docker CLI to pick up.

I imagine there might be other uses of the env field to pass environment variables/flags to the builder? Or does invoking through the docker client not really recognize those environment variables anyway? That might help define whether we deprecate the field. I agree with your reasoning that specifying the builder explicitly is more intuitive.

viveklak · 2022-12-03T00:35:55Z

provider/cmd/pulumi-resource-docker/schema.json

@@ -3039,6 +3039,11 @@
                    },
                    "description": "An optional map of named build-time argument variables to set during the Docker build. This flag allows you to pass built-time variablesthat can be accessed like environment variables inside the RUN instruction."
                },
+                "builderVersion": {
+                    "type": "string",


Should we make this an enum instead? https://www.pulumi.com/docs/guides/pulumi-packages/schema/#complextype

viveklak · 2022-12-03T00:38:56Z

provider/resources.go

+								"`BuilderV1` - the first generation builder in docker daemon\n" + "" +
+								"`BuilderBuildKit - the builder based on moby/buildkit project\n " +
+								"Defaults to `BuilderBuildKit`.",
+							TypeSpec: schema.TypeSpec{Type: "string"},


See above comment on perhaps representing this as an enum instead?

t0yv0 · 2022-12-05T15:15:57Z

I have filed a feature request for pulumi/pulumi/sdk/go to build some helper methods to simplify or eliminate the marshaling code here pulumi/pulumi#11530

t0yv0 · 2022-12-05T15:16:38Z

provider/image.go


 	// build can be nil, a string or an object; we will also use reasonable defaults here.
 	var build Build
 	if b.IsNull() {
-		// use the default build context
+		//// use the default build context


t0yv0 · 2022-12-05T15:17:28Z

provider/image_test.go

@@ -162,7 +163,7 @@ func TestMarshalBuildAndApplyDefaults(t *testing.T) {
 			"extraOptions": resource.NewArrayProperty([]resource.PropertyValue{}),
 		})

-		actual := marshalBuildAndApplyDefaults(input)
+		actual, _ := marshalBuildAndApplyDefaults(input)


A bit more verbose but could use

actual, err := ... assert.NoError(t, err)

t0yv0 · 2022-12-05T15:19:22Z

This makes a lot of sense and LGTM except I echo Vivek's desire to try to make it an enum for better discoverability. I also wonder if we accumulate this in pending release notes somewhere? This would be great to highlight to users as part of release notes ("here's how it used to work, and here is how it works now").

t0yv0 · 2022-12-05T15:30:05Z

I was also curious about this:

type Build struct {
   ...
   Env            map[string]string `pulumi:"env"`
   Args           map[string]*string `pulumi:"args"` // why *string here? is there a special meaning for nil?
}

…nerate schema

github-actions · 2022-12-05T18:06:17Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

guineveresaenger · 2022-12-05T18:11:00Z

@t0yv0 - re: the string pointer - yes! pretty much exactly this for ImageBuildOptions which we pass to the Docker client.

viveklak · 2022-12-05T23:01:28Z

provider/cmd/pulumi-resource-docker/schema.json

+                },
+                {
+                    "name": "BuilderBuildKit",
+                    "description": "The builder based on moby/buildkit project",


Should we also mention that this is the default in the description?

guineveresaenger added 4 commits December 2, 2022 11:30

add first buildkit types field

7601099

Add logic for Builder Version on client. Add input marshaling tests

1c621c9

Update Image description

8390c85

Regenerate schema and SDKs

0c94958

guineveresaenger requested review from viveklak and t0yv0 December 3, 2022 00:16

viveklak reviewed Dec 3, 2022

View reviewed changes

t0yv0 reviewed Dec 5, 2022

View reviewed changes

guineveresaenger added 2 commits December 5, 2022 10:02

make BuilderVersion an enum. Add err check on tests. Fix up tests. Ge…

88f3cb9

…nerate schema

Add SDK

b80f7b5

guineveresaenger requested review from t0yv0 and viveklak December 5, 2022 18:58

viveklak reviewed Dec 5, 2022

View reviewed changes

viveklak approved these changes Dec 5, 2022

View reviewed changes

guineveresaenger merged commit 7cfa185 into master Dec 5, 2022

guineveresaenger deleted the buildkit branch December 5, 2022 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Buildkit Option #434

Add Buildkit Option #434

guineveresaenger commented Dec 3, 2022

github-actions bot commented Dec 3, 2022

viveklak commented Dec 3, 2022

viveklak Dec 3, 2022

viveklak Dec 3, 2022

t0yv0 commented Dec 5, 2022

t0yv0 Dec 5, 2022

t0yv0 Dec 5, 2022

t0yv0 commented Dec 5, 2022

t0yv0 commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

guineveresaenger commented Dec 5, 2022 •

edited

Loading

viveklak Dec 5, 2022

Add Buildkit Option #434

Add Buildkit Option #434

Conversation

guineveresaenger commented Dec 3, 2022

github-actions bot commented Dec 3, 2022

Does the PR have any schema changes?

viveklak commented Dec 3, 2022

viveklak Dec 3, 2022

Choose a reason for hiding this comment

viveklak Dec 3, 2022

Choose a reason for hiding this comment

t0yv0 commented Dec 5, 2022

t0yv0 Dec 5, 2022

Choose a reason for hiding this comment

t0yv0 Dec 5, 2022

Choose a reason for hiding this comment

t0yv0 commented Dec 5, 2022

t0yv0 commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

Does the PR have any schema changes?

guineveresaenger commented Dec 5, 2022 • edited Loading

viveklak Dec 5, 2022

Choose a reason for hiding this comment

guineveresaenger commented Dec 5, 2022 •

edited

Loading