Integrate upstream as a fork to address Windows connection issues, build caching #537

AaronFriel · 2023-03-13T07:14:27Z

By commit, this pull request:

Adds the upstream as a fork and updates dependencies to the latest upstream SDKs for Docker.
Fixes Provider fails on Windows when using explicit providers without DOCKER_HOST set #536 by using client context var detection, removing unix specific defaults.
Fixes failing to pull from other registries during a multi-stage build, e.g.: when using multiple private registries. Note: the other registries must be logged in via docker login.
Fixes Support multi-stage inline layer caching in BuildKit operation #520 by adding the session auth service to BuildKit, and removing sources of variance in tarring files.
Fixes Image resource fails on Windows with relative paths to build context #538, fixes Image resource on Windows without default platform results in invalid platform selection #535 by adding context dir resolution and fixing platform part joining.
Adds multi-stage build caching test.

The Docker client connection is now initialized with better options. * In bridge schema: Removed the unix socket default value for `Host`. * In the Image resource: API version negotiation is enabled. * In the upstream Provider resource: remove an assumed input Host value, and use `client.FromEnv` to provide safe defaults for all platforms.

Any stage pulling from another registry may have an auth configuration, supplying these in `ImageBuildOptions` allows the daemon to use these in addition to the target server auth.

github-actions · 2023-03-13T07:24:22Z

Does the PR have any schema changes?

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

* Sets tar options to remove uid, gid from files. * Uses session-based auth with injected registry auth. Prior to this commit, verbose logs would report errors importing cache manifests from images built with arg `BUILDKIT_INLINE_CACHE=1`: ``` [builder 1/6] FROM docker.io/library/node:alpine@sha256:4a3a2ccfa801ce6960e7fc29fc5e5a1ed896b633e4731cdb87b4e1a1e9ad246e digest: sha256:13a7ee18c229d7dd37fc02dee457f4e525951359df4cad0db5eeb56079cc806b importing cache manifest from 616138583583.dkr.ecr.us-west-2.amazonaws.com/docker-provider-test-11ec639:latest error: unexpected status code [manifests latest]: 401 Unauthorized ```

The context dir must be validated and made into an absolute dir. Methods from `docker/cli/image/build` are used to massage the input path and validate it before using it in build options.

github-actions · 2023-03-13T07:33:55Z

Does the PR have any schema changes?

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

github-actions · 2023-03-13T08:00:19Z

Does the PR have any schema changes?

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

iwahbe

Looks good to me. LGTM!

guineveresaenger

This LGTM - will be curious to see how our 30s assertion holds up in CI.

Please, can you plain merge commit, so we keep the commit history?

guineveresaenger · 2023-03-13T23:44:32Z

examples/examples_go_test.go

+//
+// There is some measurement error here, as we're measuring Docker via `pulumi up` and step 2 will
+// have to download the first stage from the repository. To minimize this the same Pulumi program is
+// used and the each stage uses a small image as the base.


I have noticed that sometimes with small images, using a custom cache can be slower than building "from scratch" - i.e. pulling the public image. Are we worried about this here?

I could go a step further and make this a YAML program - no Go compilation step. That would be faster yet.

I could go a step further and make this a YAML program

That's a great point. Actually, should we use YAML as the default language to author tests across our org going forward?

guineveresaenger · 2023-03-13T23:46:49Z

examples/examples_go_test.go

+		if err := exec.Command("docker", "image", "prune").Run(); err != nil {
+			return fmt.Errorf("error pruning dangling images: %w", err)
+		}
+		if err := exec.Command("docker", "builder", "prune", "-a").Run(); err != nil {


Would a prune -af be appropriate here? Or, rather: is there ever a situation where in a test we'd need to force clear the cache?

Unlike the other prune commands, -f removes prompts for confirmation. It must detect that it's not in an interactive terminal and skips the prompt.

examples/examples_go_test.go

guineveresaenger · 2023-03-13T23:50:59Z

examples/examples_go_test.go

+//
+// We expect Step 1 to take more time than the sleep duration, ensuring we don't have a local cache.
+//
+// With `--cache-from` and a decent network connection, we expect Step 2 to take less time than the


This is such an excellent code comment; thank you! 🎉

guineveresaenger · 2023-03-13T23:52:22Z

examples/examples_go_test.go

@@ -66,7 +139,7 @@ func TestDockerfileGo(t *testing.T) {

 	opts := base.With(integration.ProgramTestOptions{
 		Dependencies: []string{
-			"github.com/pulumi/pulumi-docker/sdk/v4",
+			"github.com/pulumi/pulumi-docker/sdk/v4=../sdk",


Do we still need this with the replace directive below?

Yes and no. ProgramTest will copy the example program to /tmp/$randomDir, and the replace directive would not work from that location (../sdk => /tmp/sdk).

Using this mode of Dependencies is just a little more explicit, and it will fail more loudly on major version updates if we forget to update it.

examples/examples_go_test.go

sdk/java/src/main/java/com/pulumi/docker/Image.java

mikhailshilkov · 2023-03-14T19:27:22Z

Curious - what are the pros of opening a single PR for this as opposed to 8 separate PRs? (potentially, stacked the way @abhinav does in pu/pu) Is that mainly for the speed of iteration?

AaronFriel · 2023-03-15T19:50:08Z

Speed yes, and I think it's helpful when making large PRs such as this to have a sync with reviewers. @guineveresaenger and I spoke on Monday before her review.

AaronFriel added 3 commits March 12, 2023 23:03

provider: fork upstream, patch to docker v23.0.1

1d5ddc2

image: Fix failing to pull from other registries

0829eef

Any stage pulling from another registry may have an auth configuration, supplying these in `ImageBuildOptions` allows the daemon to use these in addition to the target server auth.

AaronFriel requested review from guineveresaenger and iwahbe March 13, 2023 07:15

AaronFriel added 4 commits March 13, 2023 00:28

image: Fix Windows usage of Image resource

91b79ab

The context dir must be validated and made into an absolute dir. Methods from `docker/cli/image/build` are used to massage the input path and validate it before using it in build options.

examples: Add multi stage build caching test

1c5a64d

Makefile: fix checks failing due to upstream uninit

aff750b

AaronFriel force-pushed the friel/fast-follows branch from 869bfdc to 56534cd Compare March 13, 2023 07:30

Makefile: generate docs using latest provider schema

2bc208c

AaronFriel force-pushed the friel/fast-follows branch from 56534cd to 2bc208c Compare March 13, 2023 07:55

iwahbe approved these changes Mar 13, 2023

View reviewed changes

guineveresaenger approved these changes Mar 14, 2023

View reviewed changes

AaronFriel merged commit efb56a7 into master Mar 14, 2023

AaronFriel deleted the friel/fast-follows branch March 14, 2023 16:12

guineveresaenger mentioned this pull request Mar 27, 2023

AWS Private ECR Pull, Build, Push - not pulling in CI #566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate upstream as a fork to address Windows connection issues, build caching #537

Integrate upstream as a fork to address Windows connection issues, build caching #537

AaronFriel commented Mar 13, 2023 •

edited

Loading

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

iwahbe left a comment

guineveresaenger left a comment

guineveresaenger Mar 13, 2023

AaronFriel Mar 14, 2023

mikhailshilkov Mar 14, 2023

guineveresaenger Mar 13, 2023

AaronFriel Mar 14, 2023

guineveresaenger Mar 13, 2023

guineveresaenger Mar 13, 2023

AaronFriel Mar 14, 2023

mikhailshilkov commented Mar 14, 2023

AaronFriel commented Mar 15, 2023

Integrate upstream as a fork to address Windows connection issues, build caching #537

Integrate upstream as a fork to address Windows connection issues, build caching #537

Conversation

AaronFriel commented Mar 13, 2023 • edited Loading

github-actions bot commented Mar 13, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 13, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 13, 2023

Does the PR have any schema changes?

iwahbe left a comment

Choose a reason for hiding this comment

guineveresaenger 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

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

mikhailshilkov commented Mar 14, 2023

AaronFriel commented Mar 15, 2023

AaronFriel commented Mar 13, 2023 •

edited

Loading