Use new docker build provider #1278
Conversation
There was a problem hiding this comment.
SDK types are unchanged -- those remain the old Docker API.
The @pulumi/docker typess don't show up in the public API of this component, right? So what do you mean that "SDK types are unchanged"? I assume we can remove the dependency on @pulumi/docker from both the implementation, and update the implied depdendency in the schema (which I assume is only to hint at the internal dependnecy - even though not strictly needed?). Feel like maybe I'm missing something that motivated this comment though?
awsx/package.json
Outdated
| @@ -22,8 +22,9 @@ | |||
| }, | |||
| "//": "Pulumi sub-provider dependencies must be pinned at an exact version because we extract this value to generate the correct dependency in the schema", | |||
| "dependencies": { | |||
| "@pulumi/aws": "6.32.0", | |||
| "@pulumi/aws": "6.9.0", | |||
| "@pulumi/docker": "4.5.1", | |||
There was a problem hiding this comment.
Can we remove this dependency?
There was a problem hiding this comment.
I think removing this will break anyone who indirectly depends on it -- is that OK? In other words I can specify only awsx in package.json but still use docker in my program.
Sorry, this meant that awsx's own wrapper types around Docker are unchanged. Only the internals are changing, the user-facing API remains the same. |
| tags: [canonicalImageName], | ||
| buildArgs: dockerInputs.args, | ||
| cacheFrom: cacheFrom, | ||
| cacheTo: [{ inline: {} }], |
There was a problem hiding this comment.
Do you know if this will increase image sizes substantially? IIRC this is only metadata about the layers so the size increase shouldn't be substantial.
There was a problem hiding this comment.
Great question! It's actually full cache artifacts, so it can increase the size a bit. I think it would be best to take this out and stay as close to the existing behavior of not doing any caching stuff unless cacheFrom is set.
However this makes me realize we probably do want inline caching when cacheFrom is set. Currently it's using a registry cache output, but this isn't compatible with the existing behavior because users currently point this at an image (not a cache artifact) due to the way the old Docker provider did caching. (It would simply pull images to get layers into your local build cache.) In other words we don't want to export a cache to image:latest, but it would be reasonable to push an image with inline caching.
Caching Docker images is hard enough as it is and the old provider's behavior was also easy to get wrong. For example cacheFrom might not do anything without a magic env var:
new docker.Image("my-app-image", {
build: {
args: {
BUILDKIT_INLINE_CACHE: "1",
},
cacheFrom: {
images: ["foo:latest"],
},
context: "app/",
dockerfile: "app/Dockerfile",
},
imageName: "foo:latest",
});So for us to stay as close to the existing behavior while also allowing caching to somewhat work, I think we basically just want to enable this BUILDKIT_INLINE_CACHE behavior when cacheFrom is present.
| if (dockerInputs.context !== undefined) { | ||
| context = dockerInputs.context; | ||
| } | ||
|
|
||
| const dockerImageArgs: docker.ImageArgs = { |
There was a problem hiding this comment.
Are there any new input properties we should expose as well?
There was a problem hiding this comment.
Probably! There's a lot more we can do now, e.g. multiple tags, multiple platforms, more refined caching behavior, etc.
I didn't want to add any of those right now because (a) these are opinionated APIs and opinions take more time to get concensus on, and (b) it's not clear how much (if any) we want to continue investing in this wrapper versus giving users more composable building blocks (like your example of registry auth helpers). The latter is my preference, but it's not my call to make.
|
Are we going forward with this @blampe or should we pick up on AWS squad? What is your confidence that the upgrade is going to be "invisible" to users? Does new Docker Build provider Image have an alias to the old image and is ready to accept old image states and rewire them? Or is there going to be a replacement during upgrade but we believe it is benign?
This would be good but I also wonder if we know enough to cover the upgrade space well enough to test it properly. Perhaps instead we can just include this change in the next major version? |
|
This likely will also fix #1401 |
This replaces the old Docker provider with the new Docker Build provider.
User-facing AWSX types are unchanged.
Behavior is unchanged, except we will now automatically store and use an inline cache if the user hasn't configured anything explicitly.
Fixes #1349
Fixes #1072