Tag docker target images with a unique name #31

CyrusNajmabadi · 2018-10-17T19:20:33Z

We now tag all our docker pushed builds with a tag equal to their image-id. i.e. we'll end up tagging a build like so:

<awsid>.dkr.ecr.<region>.amazonaws.com/image-name:<image-id> or
<awsid>.dkr.ecr.<region>.amazonaws.com/image-name:<user-tag>-<image-id>

By adding teh image-id into the tag name, we then have a unique and stable name for the target build that can be known at preview time. It also means that any time docker produces a new image id, we end up replacing the image. This helps us avoid needing to hack things in at the cloud layer where we place hte image-id in the environment variable map in order to get pulumi to want to replace the cloud.Service resource.

As part of this change docker.buildAndPushImage now retursn that entire target name, instead of just returning the image-id like it used to before.

swgillespie · 2018-10-17T22:22:35Z

docker/docker.ts

+    // this tag doesn't require actually pushing the image, nor does it require communicating with
+    // some external system, making it suitable for unique identification, even during preview.
+    // This also means that if docker produces a new imageId, we'll get a new name here, ensuring that
+    // resources will be appropriately replaced.


What resources?

will document. but htings like cloud.Service.

swgillespie · 2018-10-17T22:23:59Z

docker/image.ts

        });

-        this.id = imageData.apply(d => d.id);
-        this.digest = imageData.apply(d => d.digest);
+        this.imageName = imageData.apply(d => d.uniqueTargetName);


OK, so the idea here is that consumers can key off of imageName and expect to get replaced or updated if the image changes?

lukehoban · 2018-10-18T18:26:18Z

docker/docker.ts

-        cacheFrom = pullCacheAsync(imageName, cacheFromParam, login, repositoryUrl, logResource);
-    } else {
-        cacheFrom = Promise.resolve(undefined);
+        cacheFrom = await pullCacheAsync(imageName, cacheFromParam, login, repositoryUrl, logResource);


Did we have to change this? I'm a little nervous that this subtly changes some execution behaviour that we depended on. (We now block on the pullCacheAsync instead of running it in parallel with other work below?). Given that we don't have much test coverage around the cacheFrom support, I'm inclined to not change it unless we have to - and if we do, to ideally add some additional test coverage.

We do not. I'll revert this. It seemed like this existed at a time when we didn't have async/await here and were instead using Outputs. We cleaned that up at one point, but this stuff still remained. changing this made things clearer for me. However, you're correct this isn't necessary, so i'll revert :)

lukehoban · 2018-10-18T18:27:12Z

docker/docker.ts

+
+    if (!repositoryUrl) {
+        throw new ResourceError("Expected repository URL to be defined during push", logResource);
+    }


Why do we need this? The parameter is marked non-optional.

I was preserving/moving this from existing code. But you're right that it shouldn't be necessary in either case. will remove.

lukehoban · 2018-10-18T18:29:08Z

docker/docker.ts

+    // some external system, making it suitable for unique identification, even during preview.
+    // This also means that if docker produces a new imageId, we'll get a new name here, ensuring that
+    // resources (like docker.Image and cloud.Service) will be appropriately replaced.
+    const uniqueTargetName = createTargetName(repositoryUrl, tag, imageId);


Perhaps uniqueImageName or uniqueTaggedImageName?

lukehoban · 2018-10-18T18:31:36Z

docker/docker.ts

        }
    }

-    return buildResult.id;
+    return uniqueTargetName;


Is returning this instead of id a breaking change? I think not - that it is still a unique identifier that is sufficient for any use case of the previous return value - but just want to make sure.

right. it's only 'breaking' in the sense if somoene took a hard dependency on this exactly being docker's image ID. but we never doc'ed that, so it would be their issue for doing this. It's not an issue for our own docker.Image or cloud.Service consumers.

docker/docker.ts

lukehoban · 2018-10-18T18:39:03Z

docker/docker.ts

+    // A tag name must be valid ASCII and may contain lowercase and uppercase letters, digits,
+    // underscores, periods and dashes. A tag name may not start with a period or a dash and may
+    // contain a maximum of 128 characters.
+    const fullTag = pieces.join("-").replace(/[^-_.a-zA-Z0-9]/, "").substr(0, 128);


Do we have to do this? If the user provided a tag - it's their fault if it isn't legal and I don't think we should try to "fix" it. If the imageID from Docker is not legal to use here - we should probably do a more intentional transformation on it. Or if we are paranoid that it's format may change arbitrarily - at least only do this on the imageId component?

that's a fair point. The major concern is simply the length. If the caller provides a long tag length, and we jam on the hash, and we go past 128, then that's going to blowup. But perhaps we just let that blowup in that case....

lukehoban · 2018-10-18T18:40:49Z

docker/docker.ts

+        // That's fine, just move onto the next stage.
+        const { code } = await runCommandThatCanFail(
+            "docker", ["pull", image], logResource,
+            /*reportFullCommand:*/ true, /*reportErrorAsWarning:*/ true);


Is this fixing some latent bug in the existing code?

I will doc. The idea here is that 'pulling' during the cacheefrom code will fail the command the first time (when things aren't cached locally). This ends up with a pulumi update that succeeds, but which prints an error at the end. This simply changes things to not print out that command failure as an 'error' but instead as a 'warning'.

lukehoban · 2018-10-18T18:42:08Z

docker/docker.ts

+    // Trim off the hash kind if there is one.
+    let imageId = inspectResult.trim();
+    const colonIndex = imageId.lastIndexOf(":");
+    imageId = colonIndex < 0 ? imageId : imageId.substr(colonIndex + 1);


Why do we trim this off? Shouldn't we return the full imageID just so that we expose exactly what Docker uses as the image ID?

ImageID is "algorithm:hex". I trim off the algorithm: part since we can't have colons in the tag portion. As we want just a unique tag, the hex portion is sufficient. I could include the algorithm. but it seems really superfluous...

Thoughts?

lukehoban · 2018-10-18T18:46:31Z

docker/docker.ts

+            p.stdin.end(stdin);
+        }
+
+        return;


It makes me nervous that we are touching this code - it has historically had very subtle issues. Did we have to for this change?

@lukehoban ptal. Note: i recommend just looking at the after form of this function, not necessarily the diff. It's actually become quite simple and i think has a lot of behaviors that will be more desirable than what we had before.

lukehoban · 2018-10-18T18:49:33Z

docker/image.ts

-    /**
-     * The pushed image digest.
-     */
-    public digest: pulumi.Output<string | undefined>;


These are breaking changes. Can we mitigate these by deprecating these properties and publishing something sufficiently correct for these two values?

yup. i'll do that.

CyrusNajmabadi · 2018-10-18T20:40:36Z

@lukehoban Ready for anothe rpass.

lukehoban

LGTM - just one remaining concern on the exec changes.

lukehoban · 2018-10-18T20:50:43Z

docker/docker.ts

        });

        p.on("close", code => {
-            logStdErrMessages(code);
+            finish(code);


I don't think it's safe to call finish in both error and close. I believe both will frequently get called?

And it looks like we now never reject - is that really intentional?

ok. have put a lot of effort in doc'ing this. ptal @lukehoban i think the code is where we want it, and actually will give ideal results here.

lukehoban · 2018-10-18T20:52:23Z

docker/image.ts

     */
+    public registryServer: pulumi.Output<string | undefined>;
+
+    /**@deprecated This will have the same value as [imageName], but will be removed in the future. */


Formatting looks a little funny here - but maybe that's okay.

Cyrus Najmabadi added 7 commits October 17, 2018 11:20

Generate a unique tagged name for a docker build.

bba40e4

Update image as well.

722080d

Fix.

45c8693

Rename parmaeter.

abefedf

Don't print an error when doing a pull of a non-existent image.

e5235d0

Cleanup.

47558cb

Wrapping.

0c4221b

CyrusNajmabadi requested review from lukehoban and swgillespie October 17, 2018 21:51

Docs

db2796c

CyrusNajmabadi requested a review from hausdorff October 17, 2018 22:00

swgillespie reviewed Oct 17, 2018

View reviewed changes

Update docs.

9220523

lukehoban reviewed Oct 18, 2018

View reviewed changes

Cyrus Najmabadi added 8 commits October 18, 2018 12:37

Restore previous logic.

7d38214

Remove unnecessary check.

c2ab166

Renames.

303a652

Add docs.

30b25e3

Remove validation on our end.

8a4d668

Docs.

cf01fc9

Docs.

4c915c5

Restore values.

ec9e9cd

lukehoban approved these changes Oct 18, 2018

View reviewed changes

Cyrus Najmabadi added 2 commits October 18, 2018 14:19

Add a bunch of docs

aefbb5b

Formatting.

0f7d461

CyrusNajmabadi merged commit 57c6cae into master Oct 18, 2018

pulumi-bot deleted the uniqueTags branch October 18, 2018 22:06

CyrusNajmabadi mentioned this pull request Oct 19, 2018

Dependencies on docker images run before image is built and pushed #18

Closed

geekflyer mentioned this pull request Oct 30, 2018

Better support for remote caching #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag docker target images with a unique name #31

Tag docker target images with a unique name #31

CyrusNajmabadi commented Oct 17, 2018

swgillespie Oct 17, 2018

CyrusNajmabadi Oct 17, 2018

swgillespie Oct 17, 2018

CyrusNajmabadi Oct 17, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

CyrusNajmabadi commented Oct 18, 2018

lukehoban left a comment

lukehoban Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

lukehoban Oct 18, 2018

Tag docker target images with a unique name #31

Tag docker target images with a unique name #31

Conversation

CyrusNajmabadi commented Oct 17, 2018

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

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

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

CyrusNajmabadi commented Oct 18, 2018

lukehoban 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