build-on-preview skips if push is true

[bandreit]

What happened?

I am mostly trying to replicate the set-up from the docs for pushing to ECR with caching.

The image fails to build on preview with the following warning:

Diagnostics:
  docker-build:index:Image (db-backup-image):
    warning: Skipping preview build because some inputs are unknown.

The reason apparently can be tracked down to the checks that are performed before establishing if build-on-preview is to pe performed.

The deep-equality check performed here fails because of the normalization. Because the push flag is true, the normalization function adds the Exports config - causing an obvious difference in the deep-equality check that follows.

Example

const image = new docker_build.Image(name, {
        tags: [
            pulumi.interpolate`${imageRepository.repositoryUrl}:latest`,
            pulumi.interpolate`${imageRepository.repositoryUrl}:${getCommitSha().slice(0, 7)}`,
        ],
        context: {
            location: solutionPath,
        },
        dockerfile: {
            location: dockerFilePath,
        },
        cacheFrom: [
            {
                registry: {
                    ref: pulumi.interpolate`${imageRepository.repositoryUrl}:cache`,
                },
            },
        ],
        cacheTo: [
            {
                registry: {
                    imageManifest: true,
                    ociMediaTypes: true,
                    ref: pulumi.interpolate`${imageRepository.repositoryUrl}:cache`,
                },
            },
        ],
        platforms: [docker_build.Platform.Linux_amd64],
        buildArgs,
        push: true,
        registries: [
            {
                address: imageRepository.repositoryUrl,
                password: authToken.apply((token) => token.password),
                username: authToken.apply((token) => token.userName),
            },
        ],
    });

Output of pulumi about

CLI          
Version      3.121.0
Go Version   go1.22.4
Go Compiler  gc

Plugins
KIND      NAME          VERSION
resource  aws           6.51.0
resource  awsx          2.14.0
resource  docker        4.5.5
resource  docker        3.6.1
resource  docker-build  0.0.6
language  nodejs        unknown

Host     
OS       darwin
Version  14.6.1
Arch     arm64

Backend        
Name         ****
URL           ****
User         ****
Organizations  
Token type     personal

Dependencies:
NAME                            VERSION
@pulumi/docker-build            0.0.6
@pulumi/pulumi                  3.131.0
prettier                        2.8.8
typescript                      4.5.4
@lego/eslint-config-prettier    9.0.0
@lego/eslint-config-typescript  11.0.0
@pulumi/aws                     6.51.0
@pulumi/awsx                    2.14.0
@types/node                     20.16.1
dotenv                          16.1.4

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

[bandreit]

It seems like I can get around it by explicitly specifying the exports and disabling push by default.

push: false,
exports: [
    {
        registry: {},
    },
],

This runs the build on preview and publishes the image when running up.

[blampe]

Thanks for filing this @bandreit! We'll look into this.

[stazz]

This problem is seriously hampering my usage of the Image resource. I must use load: true to examine some values of the image after the build, but that now conflicts with exports. And multiple exports is not supported.

I could try to create PR as well, but I am actually not even sure how to fix the problem described in this issue. Some patch to add flag to DeepEquals to make it ignore certain changes, or some deeper change to detect buildability instead of just normalise + DeepEqual?

[stazz]

Actually, the more I think about it, the more I am leaning towards this kind of approach:

• Remove shortcut push and load properties altogether
• Enable multiple exports (since buildx supports them: You can export multiple outputs by repeating the flag.)
• Export helper functions to reproduce behaviour of shortcut flags, and let the user use them in exports fields

// Exported by @pulumi/docker-build package
function push(
  args: types.input.ExportRegistryArgs = {},
): docker.types.input.ExportArgs {
  return {
    registry: args,
  };
}

function load(
  args: types.input.ExportDockerArgs = {},
): docker.types.input.ExportArgs {
  return {
    docker: args,
  };
}

// Used in client code
import * as docker from "@pulumi/docker-build";

new docker.Image("...", {
  exports: [
    docker.push(),
    docker.load()
  ],
  ...
});

I think (I admit tho that I haven't looked closely at the codebase) this would remove a lot of edgecase-related plumbing code from the provider, and would be more explicit towards the users of the provider. WDYT?

[blampe]

Remove shortcut push and load properties altogether

Unfortunately now that they're already there we can't take them out, as we try to stay as backwards-compatible as possible. We can make push optional, though -- I'm strongly in favor of that since the Docker CLI doesn't require it, and we've tried to stay as close as possible to the CLI's behavior.

For some background these helpers were initially omitted, partly because of the confusing behavior you're running into here. There were concerns the API would be too unfriendly if users had to explicitly configure every export (which isn't wrong!) so the helpers were added. Opinions were split on whether to require the push helper, so we started with it required because we can always walk that back.

Enable multiple exports (since buildx supports them: You can export multiple outputs by repeating the flag.)

#235

[stazz]

Ah, it makes sense now with some history behind it explained, thanks for shedding the light on it. :)

we try to stay as backwards-compatible as possible.

I can not speak for all of the users of this provider for sure, but at least for me, when the version is beginning with 0, it strongly signals that there might be backwards-incompatible changes introduced between minor or even patch versions, as per semver spec. That spec item says that only version 1.0.0 is the public one, and all the backwards-incompatible changes after that should increase the major version. So at least from that PoV, introducing backwards-incompatible changes during 0.x lifecycle makes sense if it fixes or improves typical usecases or logical errors in the API.

But making push optional would probably help already a lot, if there is strong push (heh!) towards keeping things backwards-compatible even during 0.x era 👍