fix(assets): docker asset versions are pushed to separate repositories (

aws#4537) * fix(core): upload ecr assets to separate repositories Derive repository name from asset id, not asset source hash. We need to derive the repository name from the asset id otherwise the CDK will create a new repository each time the image changes (which will make layer caching useless). Fixes aws#4535 * move default for repositoryName to DockerImageAsset * repositoryName * add test * allow breaking change for addDockerImageAsset * non breaking * add test for addDockerImageAsset * more tests * remove useless diff * simplify * test.assets.ts
alukach · Oct 24, 2019 · 8484114 · 8484114
1 parent 406dc8e
commit 8484114
Show file tree

Hide file tree

Showing 6 changed files with 152 additions and 6 deletions.
diff --git a/packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts b/packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts
@@ -96,7 +96,7 @@ export class DockerImageAsset extends Construct implements assets.IAsset {
       directoryName: staging.stagedPath,
       dockerBuildArgs: props.buildArgs,
       dockerBuildTarget: props.target,
-      repositoryName: props.repositoryName,
+      repositoryName: props.repositoryName || `cdk/${this.node.uniqueId.replace(/[:/]/g, '-').toLowerCase()}`,
       sourceHash: staging.sourceHash
     });
 

diff --git a/packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts b/packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts
@@ -1,6 +1,7 @@
 import { expect, haveResource, SynthUtils } from '@aws-cdk/assert';
 import iam = require('@aws-cdk/aws-iam');
-import { App, Lazy, Stack } from '@aws-cdk/core';
+import { App, Construct, Lazy, Resource, Stack } from '@aws-cdk/core';
+import { ASSET_METADATA } from '@aws-cdk/cx-api';
 import fs = require('fs');
 import { Test } from 'nodeunit';
 import path = require('path');
@@ -28,6 +29,27 @@ export = {
     test.done();
   },
 
+  'repository name is derived from node unique id'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    class CoolConstruct extends Resource {
+      constructor(scope: Construct, id: string) {
+        super(scope, id);
+      }
+    }
+    const coolConstruct = new CoolConstruct(stack, 'CoolConstruct');
+
+    // WHEN
+    new DockerImageAsset(coolConstruct, 'Image', {
+      directory: path.join(__dirname, 'demo-image'),
+    });
+
+    // THEN
+    const assetMetadata = stack.node.metadata.find(({ type }) => type === ASSET_METADATA);
+    test.deepEqual(assetMetadata && assetMetadata.data.repositoryName, 'cdk/coolconstructimage78ab38fc');
+    test.done();
+  },
+
   'with build args'(test: Test) {
     // GIVEN
     const stack = new Stack();
@@ -41,7 +63,7 @@ export = {
     });
 
     // THEN
-    const assetMetadata = stack.node.metadata.find(({ type }) => type === 'aws:cdk:asset');
+    const assetMetadata = stack.node.metadata.find(({ type }) => type === ASSET_METADATA);
     test.deepEqual(assetMetadata && assetMetadata.data.buildArgs, { a: 'b' });
     test.done();
   },
@@ -60,7 +82,7 @@ export = {
     });
 
     // THEN
-    const assetMetadata = stack.node.metadata.find(({ type }) => type === 'aws:cdk:asset');
+    const assetMetadata = stack.node.metadata.find(({ type }) => type === ASSET_METADATA);
     test.deepEqual(assetMetadata && assetMetadata.data.target, 'a-target');
     test.done();
   },

diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts
@@ -532,7 +532,7 @@ export class Stack extends Construct implements ITaggable {
       params = new DockerImageAssetParameters(this.assetParameters, asset.sourceHash);
 
       const metadata: cxapi.ContainerImageAssetMetadataEntry = {
-        id: this.node.uniqueId,
+        id: asset.sourceHash,
         packaging: 'container-image',
         path: asset.directoryName,
         sourceHash: asset.sourceHash,

diff --git a/packages/@aws-cdk/core/test/test.assets.ts b/packages/@aws-cdk/core/test/test.assets.ts
@@ -0,0 +1,77 @@
+import cxapi = require('@aws-cdk/cx-api');
+import { Test } from 'nodeunit';
+import { FileAssetPackaging, Stack } from '../lib';
+import { toCloudFormation } from './util';
+
+export = {
+  'addFileAsset correctly sets metadata and creates S3 parameters'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+
+    // WHEN
+    stack.addFileAsset({
+      fileName: 'file-name',
+      packaging: FileAssetPackaging.ZIP_DIRECTORY,
+      sourceHash: 'source-hash'
+    });
+
+    // THEN
+    const assetMetadata = stack.node.metadata.find(({ type }) => type === cxapi.ASSET_METADATA);
+
+    test.equal(assetMetadata && assetMetadata.data.path, 'file-name');
+    test.equal(assetMetadata && assetMetadata.data.id, 'source-hash');
+    test.equal(assetMetadata && assetMetadata.data.packaging, FileAssetPackaging.ZIP_DIRECTORY);
+    test.equal(assetMetadata && assetMetadata.data.sourceHash, 'source-hash');
+
+    test.deepEqual(toCloudFormation(stack), {
+      Parameters: {
+        AssetParameterssourcehashS3BucketE6E91E3E: {
+          Type: 'String',
+          Description: 'S3 bucket for asset "source-hash"'
+        },
+        AssetParameterssourcehashS3VersionKeyAC4157C3: {
+          Type: 'String',
+          Description: 'S3 key for asset version "source-hash"'
+        },
+        AssetParameterssourcehashArtifactHashADBAE418: {
+          Type: 'String',
+          Description: 'Artifact hash for asset "source-hash"'
+        }
+      }
+    });
+
+    test.done();
+
+  },
+
+  'addDockerImageAsset correctly sets metadata and creates an ECR parameter'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+
+    // WHEN
+    stack.addDockerImageAsset({
+      sourceHash: 'source-hash',
+      directoryName: 'directory-name',
+      repositoryName: 'repository-name'
+    });
+
+    // THEN
+    const assetMetadata = stack.node.metadata.find(({ type }) => type === cxapi.ASSET_METADATA);
+
+    test.equal(assetMetadata && assetMetadata.data.packaging, 'container-image');
+    test.equal(assetMetadata && assetMetadata.data.path, 'directory-name');
+    test.equal(assetMetadata && assetMetadata.data.sourceHash, 'source-hash');
+    test.equal(assetMetadata && assetMetadata.data.repositoryName, 'repository-name');
+
+    test.deepEqual(toCloudFormation(stack), {
+      Parameters: {
+        AssetParameterssourcehashImageName3B572B12: {
+          Type: 'String',
+          Description: 'ECR repository name and tag for asset "source-hash"'
+        }
+      }
+    });
+
+    test.done();
+  },
+};
diff --git a/packages/aws-cdk/lib/api/toolkit-info.ts b/packages/aws-cdk/lib/api/toolkit-info.ts
@@ -108,7 +108,7 @@ export class ToolkitInfo {
   public async prepareEcrRepository(asset: cxapi.ContainerImageAssetMetadataEntry): Promise<EcrRepositoryInfo> {
     const ecr = await this.props.sdk.ecr(this.props.environment.account, this.props.environment.region, Mode.ForWriting);
     let repositoryName;
-    if ( asset.repositoryName ) {
+    if (asset.repositoryName) {
       // Repository name provided by user
       repositoryName = asset.repositoryName;
     } else {

diff --git a/packages/aws-cdk/test/test.docker.ts b/packages/aws-cdk/test/test.docker.ts
@@ -55,6 +55,53 @@ export = {
     test.done();
   },
 
+  async 'derives repository name from asset id'(test: Test) {
+    // GIVEN
+
+    let createdName;
+
+    const sdk = new MockSDK();
+    sdk.stubEcr({
+      describeRepositories() {
+        return { repositories: [] };
+      },
+
+      createRepository(req) {
+        createdName = req.repositoryName;
+
+        // Stop the test so that we don't actually docker build
+        throw new Error('STOPTEST');
+      },
+    });
+
+    const toolkit = new ToolkitInfo({
+      sdk,
+      bucketName: 'BUCKET_NAME',
+      bucketEndpoint: 'BUCKET_ENDPOINT',
+      environment: { name: 'env', account: '1234', region: 'abc' }
+    });
+
+    // WHEN
+    const asset: cxapi.ContainerImageAssetMetadataEntry = {
+      id: 'Stack:Construct/ABC123',
+      imageNameParameter: 'MyParameter',
+      packaging: 'container-image',
+      path: '/foo',
+      sourceHash: '0123456789abcdef',
+    };
+
+    try {
+      await prepareContainerAsset('.', asset, toolkit, false);
+    } catch (e) {
+      if (!/STOPTEST/.test(e.toString())) { throw e; }
+    }
+
+    // THEN
+    test.deepEqual(createdName, 'cdk/stack-construct-abc123');
+
+    test.done();
+  },
+
   async 'passes the correct target to docker build'(test: Test) {
     // GIVEN
     const toolkit = new ToolkitInfo({