feat(ecr-assets): expose property imageTag separately from imageUri in ECR assets by scanlonp · Pull Request #21582 · aws/aws-cdk

scanlonp · 2022-08-12T18:33:42Z

ECR assets has properties imageUri and assetHash. assetHash is used to generate the image tag, and imageUri is the repository and tag concatenated. However, parsing the tag from the URI is not possible since imageUri is a token. Additionally, assetHash and the image tag are not always identical since adding a synthesizer to the stack with a dockerTagPrefix would concatenate the prefix to the hash to make the tag.

It is now possible to get the tag by itself with synthesizedTag.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Update branch

… pscanlo/asset-tags

gitpod-io · 2022-08-12T18:33:50Z

corymhall · 2022-08-15T13:42:45Z

packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts

+  /**
+   * 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;


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

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.

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

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

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.

kaizencc · 2022-08-15T14:27:48Z

packages/@aws-cdk/core/lib/assets.ts

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


Suggested change

* @default ${dockerTagPrefix}${asset.sourceHash}

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

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

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.

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.

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

packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Pull request has been modified.

scanlonp · 2022-08-15T20:42:55Z

Reason for the integ-test exemption: this feature does not change behavior or the CFN template. The imageTag is already written to the manifest. Now it is returned to the asset in addition to imageUri. It is an optional property in the interface, so if it used outside of the context of ecr-assets, it should not have an impact.

The unit tests should cover what an integ-test would.

TheRealAmazonKendra · 2022-08-15T20:46:25Z

packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts

+  /**
+   * 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;


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

TheRealAmazonKendra · 2022-08-15T20:49:30Z

packages/@aws-cdk/core/lib/assets.ts

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


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.

Pull request has been modified.

mergify · 2022-08-19T02:13:28Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-19T02:50:08Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 08e6e7a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-19T02:50:26Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

scanlonp and others added 5 commits August 12, 2022 12:29

Expose imageTag to DockerImageAsset

38e73bc

Finalize API, add property to tarball-asset.ts

b906fa0

Merge pull request #1 from aws/main

bda59c0

Update branch

Added tests

c61d9f2

Merge branch 'pscanlo/asset-tags' of github.com:scanlonp/aws-cdk into…

878b81a

… pscanlo/asset-tags

scanlonp requested review from TheRealAmazonKendra and rix0rrr August 12, 2022 18:34

github-actions bot added the p2 label Aug 12, 2022

aws-cdk-automation requested a review from a team August 12, 2022 18:34

Update README

98f96bd

scanlonp changed the title ~~feat(ecr-assets): added property synthesizedTag to ECR assets~~ chore(ecr-assets): added property synthesizedTag to ECR assets Aug 12, 2022

corymhall previously requested changes Aug 15, 2022

View reviewed changes

kaizencc reviewed Aug 15, 2022

View reviewed changes

packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts

5ecad0d

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

scanlonp added 2 commits August 15, 2022 12:41

Update test description

6cbe3d5

Update test description

05640f2

TheRealAmazonKendra changed the title ~~chore(ecr-assets): added property synthesizedTag to ECR assets~~ chore(ecr-assets): add property synthesizedTag to ECR assets Aug 15, 2022

scanlonp added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 15, 2022

changed name to imageTag from synthesizedTag

bd49e16

scanlonp changed the title ~~chore(ecr-assets): add property synthesizedTag to ECR assets~~ feat(ecr-assets): add property imageTag to ECR assets Aug 15, 2022

scanlonp changed the title ~~feat(ecr-assets): add property imageTag to ECR assets~~ feat(ecr-assets): expose property imageTag separately from imageUri in ECR assets Aug 15, 2022

TheRealAmazonKendra previously requested changes Aug 15, 2022

View reviewed changes

Update documentation on assets.ts

5811a19

TheRealAmazonKendra approved these changes Aug 19, 2022

View reviewed changes

Merge branch 'main' into pscanlo/asset-tags

08e6e7a

mergify bot merged commit 5f32e0f into aws:main Aug 19, 2022

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

Conversation

scanlonp commented Aug 12, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scanlonp commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 19, 2022

Uh oh!

aws-cdk-automation commented Aug 19, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants