feat(ecr-assets): Support cache-from and cache-to flags #24024

RichiCoder1 · 2023-02-06T05:47:26Z

This adds the --cache-from and --cache-to flag options.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime 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 · 2023-02-06T05:47:35Z

TheRealAmazonKendra · 2023-02-09T19:57:57Z

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

+   * @default - no cache from options are passed to the build command
+   * @see https://docs.docker.com/build/cache/backends/
+   */
+  readonly cacheFrom?: string[];


Can we more strongly type this rather than just a string array?

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!

@TheRealAmazonKendra I updated the PR to have a more strongishly-type cache option. Lmk how it looks!

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

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

aws-cdk-automation · 2023-02-10T05:12:38Z

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.

aws-cdk-automation · 2023-02-11T00:09:24Z

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.

aws-cdk-automation · 2023-02-12T00:10:53Z

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.

aws-cdk-automation · 2023-02-13T00:10:20Z

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.

aws-cdk-automation · 2023-02-14T00:10:21Z

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.

aws-cdk-automation · 2023-02-15T00:09:51Z

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.

aws-cdk-automation · 2023-02-16T00:10:12Z

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.

aws-cdk-automation · 2023-02-17T00:10:34Z

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.

RichiCoder1 · 2023-02-17T02:30:40Z

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>' }}]

aws-cdk-automation · 2023-02-22T00:09:59Z

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.

RichiCoder1 · 2023-02-23T06:45:56Z

A potential extra evolution of this could be formalizing docker cache option as a class so you could potentially have Cache.fromRegistry("myimage:main") and Cache.toInline(). I'm honestly not sure how that would flow through the asset system though. Would aws-ecr-assets have the rich class, but pass the generic struct as I have it defined now into cloud assembly/assets? Also not sure the best API for custom options (since most the backends are experimental atm).

aws-cdk-automation · 2023-03-01T00:10:50Z

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.

MrArnoldPalmer

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://github.com/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.

RichiCoder1 · 2023-03-08T19:17:54Z

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

+
+    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({


@MrArnoldPalmer followed the pattern in tar assets, lmk no if test naming and tests lgty!

MrArnoldPalmer

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!

RichiCoder1 · 2023-03-08T19:35:55Z

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 👀

RichiCoder1 · 2023-03-08T19:40:36Z

Found https://github.com/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

mergify · 2023-03-08T19:59:15Z

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 · 2023-03-08T19:59:22Z

AWS CodeBuild CI Report

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

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

RichiCoder1 · 2023-03-08T20:00:22Z

@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

RichiCoder1 · 2023-03-08T20:24:32Z

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://github.com/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://github.com/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://github.com/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*

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Feb 6, 2023

aws-cdk-automation requested a review from a team February 6, 2023 05:47

github-actions bot added the p2 label Feb 6, 2023

RichiCoder1 force-pushed the feat/ecr-assets-cache-flags branch from e36e147 to 104ba88 Compare February 6, 2023 05:49

RichiCoder1 added 2 commits February 6, 2023 10:35

feat(ecr-assets): Support cache-from and cache-to flags

3011b72

chore: fixup cloud assembly test

7eab644

RichiCoder1 force-pushed the feat/ecr-assets-cache-flags branch from 8eaad6b to 7eab644 Compare February 6, 2023 16:35

RichiCoder1 mentioned this pull request Feb 6, 2023

DockerImageCode: Support automatically using --cache-to and --cache-from #23445

Open

2 tasks

TheRealAmazonKendra previously requested changes Feb 10, 2023

View reviewed changes

Merge branch 'main' into feat/ecr-assets-cache-flags

4886756

fix: strongishly type docker cache options

738553c

RichiCoder1 added 2 commits February 17, 2023 02:39

fix: add missing unsaved changes

060461c

fix: integ tests

0bda8b1

RichiCoder1 requested a review from TheRealAmazonKendra February 17, 2023 03:07

RichiCoder1 added 3 commits February 21, 2023 16:44

Merge branch 'main' into feat/ecr-assets-cache-flags

018424b

Merge branch 'main' into feat/ecr-assets-cache-flags

ee8d345

Merge branch 'main' into feat/ecr-assets-cache-flags

fe20218

RichiCoder1 added 2 commits February 28, 2023 18:21

Merge branch 'main' into feat/ecr-assets-cache-flags

0e6f0ee

Merge branch 'main' into feat/ecr-assets-cache-flags

c2ff00b

MrArnoldPalmer previously requested changes Mar 7, 2023

View reviewed changes

RichiCoder1 added 2 commits March 8, 2023 19:16

chore: add basic unit tests for cache options

bc87be0

Merge branch 'main' into feat/ecr-assets-cache-flags

1e786ee

RichiCoder1 commented Mar 8, 2023

View reviewed changes

RichiCoder1 requested review from MrArnoldPalmer and removed request for TheRealAmazonKendra March 8, 2023 19:18

MrArnoldPalmer approved these changes Mar 8, 2023

View reviewed changes

mergify bot merged commit 4e02566 into aws:main Mar 8, 2023
6 checks passed

RichiCoder1 deleted the feat/ecr-assets-cache-flags branch March 8, 2023 20:05

RichiCoder1 mentioned this pull request Mar 8, 2023

fix(ecr-assets): prefix cache arguments correctly #24524

Merged

jamesmcglinn mentioned this pull request Mar 19, 2023

(aws-ecr-assets): DockerImageAsset cacheTo property causes "unknown flag: --cache-to" error #24685

Open

alukach mentioned this pull request Jun 9, 2023

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

Closed

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

alukach mentioned this pull request Jul 12, 2023

fix(core): support cache-from and cache-to flags in DockerImage #26337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecr-assets): Support cache-from and cache-to flags #24024

feat(ecr-assets): Support cache-from and cache-to flags #24024

RichiCoder1 commented Feb 6, 2023 •

edited

gitpod-io bot commented Feb 6, 2023

TheRealAmazonKendra Feb 9, 2023

RichiCoder1 Feb 10, 2023

RichiCoder1 Feb 17, 2023

aws-cdk-automation commented Feb 10, 2023

aws-cdk-automation commented Feb 11, 2023

aws-cdk-automation commented Feb 12, 2023

aws-cdk-automation commented Feb 13, 2023

aws-cdk-automation commented Feb 14, 2023

aws-cdk-automation commented Feb 15, 2023

aws-cdk-automation commented Feb 16, 2023

aws-cdk-automation commented Feb 17, 2023

RichiCoder1 commented Feb 17, 2023 •

edited

aws-cdk-automation commented Feb 22, 2023

RichiCoder1 commented Feb 23, 2023 •

edited

aws-cdk-automation commented Mar 1, 2023

MrArnoldPalmer left a comment •

edited

RichiCoder1 Mar 8, 2023

MrArnoldPalmer left a comment

RichiCoder1 commented Mar 8, 2023 •

edited

RichiCoder1 commented Mar 8, 2023

mergify bot commented Mar 8, 2023

aws-cdk-automation commented Mar 8, 2023

RichiCoder1 commented Mar 8, 2023 •

edited

RichiCoder1 commented Mar 8, 2023

feat(ecr-assets): Support cache-from and cache-to flags #24024

feat(ecr-assets): Support cache-from and cache-to flags #24024

Conversation

RichiCoder1 commented Feb 6, 2023 • edited

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Feb 6, 2023

TheRealAmazonKendra Feb 9, 2023

Choose a reason for hiding this comment

RichiCoder1 Feb 10, 2023

Choose a reason for hiding this comment

RichiCoder1 Feb 17, 2023

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 10, 2023

aws-cdk-automation commented Feb 11, 2023

aws-cdk-automation commented Feb 12, 2023

aws-cdk-automation commented Feb 13, 2023

aws-cdk-automation commented Feb 14, 2023

aws-cdk-automation commented Feb 15, 2023

aws-cdk-automation commented Feb 16, 2023

aws-cdk-automation commented Feb 17, 2023

RichiCoder1 commented Feb 17, 2023 • edited

aws-cdk-automation commented Feb 22, 2023

RichiCoder1 commented Feb 23, 2023 • edited

aws-cdk-automation commented Mar 1, 2023

MrArnoldPalmer left a comment • edited

Choose a reason for hiding this comment

RichiCoder1 Mar 8, 2023

Choose a reason for hiding this comment

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

RichiCoder1 commented Mar 8, 2023 • edited

RichiCoder1 commented Mar 8, 2023

mergify bot commented Mar 8, 2023

aws-cdk-automation commented Mar 8, 2023

AWS CodeBuild CI Report

RichiCoder1 commented Mar 8, 2023 • edited

RichiCoder1 commented Mar 8, 2023

RichiCoder1 commented Feb 6, 2023 •

edited

RichiCoder1 commented Feb 17, 2023 •

edited

RichiCoder1 commented Feb 23, 2023 •

edited

MrArnoldPalmer left a comment •

edited

RichiCoder1 commented Mar 8, 2023 •

edited

RichiCoder1 commented Mar 8, 2023 •

edited