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

Conversation

scanlonp
Copy link
Contributor

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:

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

@gitpod-io
Copy link

gitpod-io bot commented Aug 12, 2022

@github-actions github-actions bot added the p2 label Aug 12, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 12, 2022 18:34
@scanlonp 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
/**
* 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 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

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
@mergify mergify bot dismissed corymhall’s stale review August 15, 2022 16:40

Pull request has been modified.

@TheRealAmazonKendra 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 scanlonp added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 15, 2022
@scanlonp 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 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
@scanlonp
Copy link
Contributor Author

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.

/**
* 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.

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


/**
* 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.

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.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 15, 2022 23:29

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Aug 19, 2022

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
Copy link
Collaborator

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 mergify bot merged commit 5f32e0f into aws:main Aug 19, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 19, 2022

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).

josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
…n ECR assets (aws#21582)

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:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants