-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): Support cache-from and cache-to flags #24024
Conversation
e36e147
to
104ba88
Compare
8eaad6b
to
7eab644
Compare
* @default - no cache from options are passed to the build command | ||
* @see https://docs.docker.com/build/cache/backends/ | ||
*/ | ||
readonly cacheFrom?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we more strongly type this rather than just a string array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the pattern of the other arguments as these don't have a firm schema. I think there is a general shape I could use if we want a bit more of a structure though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheRealAmazonKendra I updated the PR to have a more strongishly-type cache option. Lmk how it looks!
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
7 similar comments
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
Pull request has been modified.
Updated so that options go from: cacheFrom: ['type=registry,ref=<ref>'] to cacheFrom: [{ type: 'registry', params: { ref: '<ref>' }}] I wasn't sure if CDK/JSII supported something like: interface DockerCacheOption {
readonly type: string;
readonly [key: string]: string;
} which arguably would be more concise and still correct and would then look like: cacheFrom: [{ type: 'registry', ref: '<ref>' }}] |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
A potential extra evolution of this could be formalizing docker cache option as a class so you could potentially have |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally I'd like to see a unit test that asserts against the asset manifest that the build command will be formatted as expected, IE that --cache-from/to are passed correctly to the container build command.
An example of something similar https:/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-ecr-assets/test/tarball-asset.test.ts#L40
Otherwise I think this looks good!
Pull request has been modified.
|
||
expect(Object.keys(manifest.dockerImages ?? {}).length).toBe(1); | ||
expect(manifest.dockerImages?.[asset.assetHash]?.source.cacheFrom?.length).toBe(1); | ||
expect(manifest.dockerImages?.[asset.assetHash]?.source.cacheFrom?.[0]).toStrictEqual({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrArnoldPalmer followed the pattern in tar assets, lmk no if test naming and tests lgty!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh okay I misunderstood a bit what the asset manifest contained, and thought we would be able to assert against the raw CLI command IE docker build --cache-from mycachething .
, but this is still good coverage so TY for implementing it!
If there's a way to accomplish that, I'd love too but I didn't see an easy way 😅 Edit: actually, let me take a peek at the private docker logic just in case 👀 |
Found https:/RichiCoder1/aws-cdk/blob/feat/ecr-assets-cache-flags/packages/cdk-assets/test/docker-images.test.ts#L390, gonna see if I can easily add a test there for the other end |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@MrArnoldPalmer I'm so sorry, I did actually discover a bug when adding additional tests. Should I do a quick follow on PR? Edit: creating a new PR, will link shortly |
Created #24524 as the followup PR to fix the issue I found |
Follow up to #24024, fixes an issue where cache args were not correctly prefixed and adds additional testing. Apologies for the second PR, I realized there was an issue literally as the auto-merge happened! ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This adds the `--cache-from` and `--cache-to` flag options. --- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https:/aws/aws-cdk/blob/main/CONTRIBUTING.md?rgh-link-date=2022-12-09T23%3A48%3A14Z) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https:/aws/aws-cdk/blob/main/CONTRIBUTING.md/?rgh-link-date=2022-12-09T23%3A48%3A14Z#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https:/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md?rgh-link-date=2022-12-09T23%3A48%3A14Z)? * [x] 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_
Follow up to aws#24024, fixes an issue where cache args were not correctly prefixed and adds additional testing. Apologies for the second PR, I realized there was an issue literally as the auto-merge happened! ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
In #24024, we added the ability to specify Docker cache flags during ecr-asset builds. However, it was not added to the `DockerImage` class. This PR adds the ability to specify the `--cache-from` and `--cache-to` flag to `DockerImage` builds. This logic was primarily lifted directly from #24024. Fixup of issues from #25925 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This adds the
--cache-from
and--cache-to
flag options.All Submissions:
Adding new Construct Runtime Dependencies:
New Features
Have you added the new feature to an integration test?
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