Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ecr-assets): expose property imageTag separately from imageUri in ECR assets #21582

Merged
merged 12 commits into from
Aug 19, 2022
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ configure it on the asset itself.
Use `asset.imageUri` to reference the image. It includes both the ECR image URL
and tag.

Use `asset.synthesizedTag` to reference only the image tag.

You can optionally pass build args to the `docker build` command by specifying
the `buildArgs` property. It is recommended to skip hashing of `buildArgs` for
values that can change between different machines to maintain a consistent
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ export class DockerImageAsset extends Construct implements IAsset {
*/
public readonly assetHash: string;

/**
* The tag of this asset when it is uploaded to ECR. The tag may differ from the assetHash if a stack synthesizer adds a dockerTagPrefix.
*/
public readonly synthesizedTag: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the name synthesizedTag. What about imageTag instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose synthesizedTag to make it clear that this will differ from assetHash when a synthesizer adds a dockerTagPrefix. I was also worried that a user would see imageUri and imageTag and think that they are distinct instead of knowing imageUri includes the tag (I have done this in my project).

If you feel those concerns are relatively minor, I am fine with imageTag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a public facing field or something that we use in our internal logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a public facing field. I changed it to imageTag everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageTag is good. The relationship between uri and tag is docker domain knowledge. I don't think it would be a good idea to abstract that away.


/**
* The path to the asset, relative to the current Cloud Assembly
*
Expand Down Expand Up @@ -362,6 +367,7 @@ export class DockerImageAsset extends Construct implements IAsset {

this.repository = ecr.Repository.fromRepositoryName(this, 'Repository', location.repositoryName);
this.imageUri = location.imageUri;
this.synthesizedTag = location.imageTag ?? this.assetHash;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/lib/tarball-asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ export class TarballImageAsset extends Construct implements IAsset {
*/
public readonly assetHash: string;

/**
* The tag of this asset when it is uploaded to ECR. The tag may differ from the assetHash if a stack synthesizer adds a dockerTagPrefix.
*/
public readonly synthesizedTag: string;

constructor(scope: Construct, id: string, props: TarballImageAssetProps) {
super(scope, id);

Expand Down Expand Up @@ -78,6 +83,7 @@ export class TarballImageAsset extends Construct implements IAsset {

this.repository = ecr.Repository.fromRepositoryName(this, 'Repository', location.repositoryName);
this.imageUri = location.imageUri;
this.synthesizedTag = location.imageTag ?? this.assetHash;
}
}

24 changes: 24 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,30 @@ describe('image asset', () => {
expect(asset1.assetHash).toEqual('13248c55633f3b198a628bb2ea4663cb5226f8b2801051bd0c725950266fd590');
expect(asset2.assetHash).toEqual('b78978ca702a8eccd37804ce31d76cd83a695b557dbf95aeb109332ee8b1fd32');
});

describe('stack synthesizers', () => { //synthesizedTag and assetHash
scanlonp marked this conversation as resolved.
Show resolved Hide resolved
const stack1 = new Stack();
const stack2 = new Stack(undefined, undefined, {
synthesizer: new DefaultStackSynthesizer({
dockerTagPrefix: 'banana',
}),
});

const directory = path.join(__dirname, 'demo-image-custom-docker-file');

const asset1 = new DockerImageAsset(stack1, 'Asset1', { directory });
const asset2 = new DockerImageAsset(stack2, 'Asset2', { directory });

test('stack with default synthesizer', () => {
expect(asset1.assetHash).toEqual('13248c55633f3b198a628bb2ea4663cb5226f8b2801051bd0c725950266fd590');
expect(asset1.synthesizedTag).toEqual('13248c55633f3b198a628bb2ea4663cb5226f8b2801051bd0c725950266fd590');
});

test('stack with overwritten synthesizer', () => {
expect(asset2.assetHash).toEqual('13248c55633f3b198a628bb2ea4663cb5226f8b2801051bd0c725950266fd590');
expect(asset2.synthesizedTag).toEqual('banana13248c55633f3b198a628bb2ea4663cb5226f8b2801051bd0c725950266fd590');
});
});
});

function testDockerDirectoryIsStagedWithoutFilesSpecifiedInDockerignore(app: App, ignoreMode?: IgnoreMode) {
Expand Down
28 changes: 27 additions & 1 deletion packages/@aws-cdk/aws-ecr-assets/test/tarball-asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Template } from '@aws-cdk/assertions';
import * as iam from '@aws-cdk/aws-iam';
import { testFutureBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { App, Stack } from '@aws-cdk/core';
import { App, Stack, DefaultStackSynthesizer } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { TarballImageAsset } from '../lib';

Expand Down Expand Up @@ -136,6 +136,32 @@ describe('image asset', () => {
}).toThrow(/Cannot find file at/);

});

describe('stack synthesizers', () => { //synthesizedTag and assetHash
const stack1 = new Stack();
const stack2 = new Stack(undefined, undefined, {
synthesizer: new DefaultStackSynthesizer({
dockerTagPrefix: 'banana',
}),
});
const asset1 = new TarballImageAsset(stack1, 'MyAsset', {
tarballFile: 'test/demo-tarball/empty.tar',
});
const asset2 = new TarballImageAsset(stack2, 'MyAsset', {
tarballFile: 'test/demo-tarball/empty.tar',
});

test('stack with default synthesizer', () => {
expect(asset1.assetHash).toEqual('95c924c84f5d023be4edee540cb2cb401a49f115d01ed403b288f6cb412771df');
expect(asset1.synthesizedTag).toEqual('95c924c84f5d023be4edee540cb2cb401a49f115d01ed403b288f6cb412771df');
});

test('stack with overwritten synthesizer', () => {
expect(asset2.assetHash).toEqual('95c924c84f5d023be4edee540cb2cb401a49f115d01ed403b288f6cb412771df');
expect(asset2.synthesizedTag).toEqual('banana95c924c84f5d023be4edee540cb2cb401a49f115d01ed403b288f6cb412771df');
});
});

});

function isAssetManifest(x: cxapi.CloudArtifact): x is cxapi.AssetManifestArtifact {
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/core/lib/assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,4 +319,10 @@ export interface DockerImageAssetLocation {
* The name of the ECR repository.
*/
readonly repositoryName: string;

/**
* The tag of the image in Amazon ECR.
* @default ${dockerTagPrefix}${asset.sourceHash}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @default ${dockerTagPrefix}${asset.sourceHash}
* @default `${dockerTagPrefix}${asset.sourceHash}`

does this work? It seems weird to not have the backticks on the default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not want the docs to actually parse the values, just show that it is the two concatenated. There may be a better way to represent that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be documentation that makes sense for other languages. This convention is specific to TypeScript/JavaScript and may not translate properly. This should be descriptive of what the default will be, not a code snippet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to
@default - dockerTagPrefix + asset.sourceHash

I can be more descriptive, say "dockerTagPrefix concatenated with the sourceHash of the asset", but I think the + conveys string concatenation agnostically

*/
readonly imageTag?: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export class AssetManifestBuilder {
role?: RoleOptions,
): DockerImageAssetLocation {
validateDockerImageAssetSource(asset);
const imageTag = dockerTagPrefix + asset.sourceHash;
const imageTag = `${dockerTagPrefix}${asset.sourceHash}`;

// Add to manifest
this.dockerImages[asset.sourceHash] = {
Expand Down Expand Up @@ -114,6 +114,7 @@ export class AssetManifestBuilder {
imageUri: cfnify(
`${account}.dkr.ecr.${region}.${urlSuffix}/${repositoryName}:${imageTag}`,
),
imageTag: cfnify(imageTag),
};
}

Expand Down