Skip to content

feat(core): Support cache-from and cache-to flags in DockerImage#25925

Closed
alukach wants to merge 3 commits intoaws:mainfrom
alukach:feat/dockerbuild-enable-cache-flags
Closed

feat(core): Support cache-from and cache-to flags in DockerImage#25925
alukach wants to merge 3 commits intoaws:mainfrom
alukach:feat/dockerbuild-enable-cache-flags

Conversation

@alukach
Copy link
Copy Markdown
Contributor

@alukach alukach commented Jun 9, 2023

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.


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
Copy Markdown

gitpod-io bot commented Jun 9, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team June 9, 2023 19:18
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Jun 9, 2023
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jul 9, 2023
@mrgrain mrgrain reopened this Jul 12, 2023
@mrgrain mrgrain added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Jul 12, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review July 12, 2023 10:57

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 2858407
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@mrgrain
Copy link
Copy Markdown
Contributor

mrgrain commented Jul 12, 2023

TS syntax error preventing build. Closing again.

@alukach Feel free to open a new PR that fixes the issues

aws-cdk-lib: aws-lambda-nodejs/test/bundling.test.ts:28:56 - error TS2345: Argument of type '{ image: string; cp: () => string; run: () => void; toJSON: () => string; }' is not assignable to parameter of type 'DockerImage'.
aws-cdk-lib:   Property 'cacheOptionToFlag' is missing in type '{ image: string; cp: () => string; run: () => void; toJSON: () => string; }' but required in type 'DockerImage'.
aws-cdk-lib:  28   jest.spyOn(DockerImage, 'fromBuild').mockReturnValue({
aws-cdk-lib:                                                            ~
aws-cdk-lib:  29     image: 'built-image',
aws-cdk-lib:     ~~~~~~~~~~~~~~~~~~~~~~~~~
aws-cdk-lib: ... 
aws-cdk-lib:  32     toJSON: () => 'built-image',
aws-cdk-lib:     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
aws-cdk-lib:  33   });
aws-cdk-lib:     ~~~
aws-cdk-lib:   core/lib/bundling.ts:432:11
aws-cdk-lib:     432   private cacheOptionToFlag(option: DockerCacheOption): string {
aws-cdk-lib:                   ~~~~~~~~~~~~~~~~~
aws-cdk-lib:     'cacheOptionToFlag' is declared here.
aws-cdk-lib: aws-lambda-nodejs/test/bundling.test.ts:485:59 - error TS2322: Type 'BundlingDockerImage' is not assignable to type 'DockerImage'.
aws-cdk-lib: 485   const tryBundle = bundler.local?.tryBundle('/outdir', { image: Runtime.NODEJS_14_X.bundlingDockerImage });
aws-cdk-lib:                                                               ~~~~~
aws-cdk-lib:   core/lib/bundling.ts:35:12
aws-cdk-lib:     35   readonly image: DockerImage;
aws-cdk-lib:                   ~~~~~
aws-cdk-lib:     The expected type comes from property 'image' which is declared here on type 'BundlingOptions'
aws-cdk-lib: aws-lambda/lib/lambda-version.ts:180:19 - warning JSII5019: The property name "version" conflicts with the declaring class "Version". This will result in renaming the class to "_Version" in C#. Consider renaming "version".
aws-cdk-lib: 180   public readonly version: string;
aws-cdk-lib:                       ~~~~~~~
aws-cdk-lib:   aws-lambda/lib/lambda-version.ts:114:14
aws-cdk-lib:     114 export class Version extends QualifiedFunctionBase implements IVersion {
aws-cdk-lib:                      ~~~~~~~
aws-cdk-lib:     The declaring class is introduced here
aws-cdk-lib: aws-lambda/lib/runtime.ts:284:5 - error TS2741: Property 'cacheOptionToFlag' is missing in type 'BundlingDockerImage' but required in type 'DockerImage'.
aws-cdk-lib: 284     this.bundlingImage = this.bundlingDockerImage;
aws-cdk-lib:         ~~~~~~~~~~~~~~~~~~
aws-cdk-lib:   core/lib/bundling.ts:432:11
aws-cdk-lib:     432   private cacheOptionToFlag(option: DockerCacheOption): string {
aws-cdk-lib:                   ~~~~~~~~~~~~~~~~~
aws-cdk-lib:     'cacheOptionToFlag' is declared here.
aws-cdk-lib: aws-lambda/test/code.test.ts:413:83 - error TS2345: Argument of type '() => { cp: jest.Mock<any, any, any>; image: string; run: jest.Mock<any, any, any>; toJSON: jest.Mock<any, any, any>; }' is not assignable to parameter of type '(path: string, options?: DockerBuildOptions | undefined) => DockerImage'.
aws-cdk-lib:   Property 'cacheOptionToFlag' is missing in type '{ cp: jest.Mock<any, any, any>; image: string; run: jest.Mock<any, any, any>; toJSON: jest.Mock<any, any, any>; }' but required in type 'DockerImage'.
aws-cdk-lib: 413       fromBuildMock = jest.spyOn(cdk.DockerImage, 'fromBuild').mockImplementation(() => ({
aws-cdk-lib:                                                                                       ~~~~~~~~
aws-cdk-lib: 414         cp: cpMock,
aws-cdk-lib:     ~~~~~~~~~~~~~~~~~~~
aws-cdk-lib: ... 
aws-cdk-lib: 417         toJSON: jest.fn(),
aws-cdk-lib:     ~~~~~~~~~~~~~~~~~~~~~~~~~~
aws-cdk-lib: 418       }));
aws-cdk-lib:     ~~~~~~~~
aws-cdk-lib:   core/lib/bundling.ts:432:11
aws-cdk-lib:     432   private cacheOptionToFlag(option: DockerCacheOption): string {
aws-cdk-lib:                   ~~~~~~~~~~~~~~~~~
aws-cdk-lib:     'cacheOptionToFlag' is declared here.
aws-cdk-lib: core/lib/bundling.ts:338:92 - error TS2339: Property 'cacheOptionToFlag' does not exist on type 'typeof DockerImage'.
aws-cdk-lib: 338       ...(options.cacheFrom ? [...options.cacheFrom.map(cacheFrom => ['--cache-from', this.cacheOptionToFlag(cacheFrom)]).flat()] : []),
aws-cdk-lib:                                                                                                ~~~~~~~~~~~~~~~~~
aws-cdk-lib: core/lib/bundling.ts:339:49 - error TS2339: Property 'cacheOptionToFlag' does not exist on type 'typeof DockerImage'.
aws-cdk-lib: 339       ...(options.cacheTo ? ['--cache-to', this.cacheOptionToFlag(options.cacheTo)] : []),
aws-cdk-lib:                                                     ~~~~~~~~~~~~~~~~~
aws-cdk-lib: core/lib/bundling.ts:432:11 - error TS6133: 'cacheOptionToFlag' is declared but its value is never read.
aws-cdk-lib: 432   private cacheOptionToFlag(option: DockerCacheOption): string {
aws-cdk-lib:               ~~~~~~~~~~~~~~~~~
aws-cdk-lib: core/test/bundling.test.ts:141:11 - error TS6133: 'cacheFrom1' is declared but its value is never read.
aws-cdk-lib: 141     const cacheFrom1 = {
aws-cdk-lib:               ~~~~~~~~~~
aws-cdk-lib: core/test/bundling.test.ts:144:11 - error TS6133: 'cacheFrom2' is declared but its value is never read.
aws-cdk-lib: 144     const cacheFrom2 = {
aws-cdk-lib:               ~~~~~~~~~~

@alukach
Copy link
Copy Markdown
Contributor Author

alukach commented Jul 12, 2023

@mrgrain thanks for taking a look at this. #26337 should resolve those issues.

mergify bot pushed a commit that referenced this pull request Aug 25, 2023
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants