feat(core): `BundlingDockerImage` now supports `run()` and `cp()` utilities #9728

misterjoshua · 2020-08-14T22:50:15Z

This PR introduces run() and cp() utility methods to BundlingDockerImage. After building their own Docker images, users can more easily run the image or copy files out of the image to create their own assets without using the bundling mechanism.

Possibly closes #9329

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

packages/@aws-cdk/aws-lambda/lib/code.ts

misterjoshua · 2020-08-18T18:52:50Z

New changes to reflect the following:

Changed the bundler option name to copyFromImagePath. The former is a better option name than imageStaticAssetPath because it better conveys what the bundler will do.
Removed lambda.Code.fromDockerBuild as lambda.Code.fromAsset supports the new bundler option out of the box.

nija-at

Since this PR is adding a new feature, mark the title as a 'feat' instead of 'chore'

packages/@aws-cdk/aws-lambda/test/integ.from-docker-build.complex.ts

Pull request has been modified.

eladb · 2020-08-26T20:05:54Z

@jogold could spare some cycles to work with @misterjoshua on this?

@misterjoshua something with the mental model is not intuitive to me at first glance. I am not sure I fully understand the use case and then the terminology of copyFromImagePath doesn't intuitively represent neither intent (the user's final goal) nor mechanism (what's mechanically happening).

misterjoshua · 2020-08-31T05:34:14Z

FYI, I plan to fix the conflict and resume work on this PR as soon as I can, but I'm swamped this week.

eladb · 2020-09-07T09:42:43Z

@jogold can you take a look?

packages/@aws-cdk/aws-s3-assets/README.md

packages/@aws-cdk/aws-s3-assets/lib/asset.ts

eladb · 2020-09-22T19:29:13Z

What's the status here?

misterjoshua · 2020-09-22T23:43:14Z

What's the status here?

@eladb I believe @jogold requested some feedback on whether we should consider implementing a different asset type. Overall, I'm not sure whether supporting my use case is out of scope for CDK bundling or the project as a whole, considering that I can handle this in a stage that happens before CDK runs.

eladb · 2020-09-23T07:55:09Z

What's the status here?

@eladb I believe @jogold requested some feedback on whether we should consider implementing a different asset type. Overall, I'm not sure whether supporting my use case is out of scope for CDK bundling or the project as a whole, considering that I can handle this in a stage that happens before CDK runs.

You can also just write some code in your CDK app that prepares the asset directory (in whatever way) and then defines an asset that refers to this directory (no need to use bundling in this case).

What's the benefit of using the asset bundling mechanism? Is it just to run docker? Can we extract some of that logic into e.g. utility functions maybe?

misterjoshua · 2020-09-23T21:31:37Z

What's the status here?

@eladb I believe @jogold requested some feedback on whether we should consider implementing a different asset type. Overall, I'm not sure whether supporting my use case is out of scope for CDK bundling or the project as a whole, considering that I can handle this in a stage that happens before CDK runs.

You can also just write some code in your CDK app that prepares the asset directory (in whatever way) and then defines an asset that refers to this directory (no need to use bundling in this case).

What's the benefit of using the asset bundling mechanism? Is it just to run docker? Can we extract some of that logic into e.g. utility functions maybe?

@eladb Yes, I'd say the benefit lies in the ease of controlling docker to prepare such a directory. BundlingDockerImage is particularly convenient for building the image. And yes, I suspect that we can extract the docker logic into some utility functions to make it easier.

Does an API like this make sense?

// Build a Docker image
const image = BundlingDockerImage.fromAsset(contextPath, {
  // Added in this PR, lets you override the Dockerfile path - very useful for me
  file: dockerfilePath,
});

// My use case - copy files out of the image from `imagePath` to `localPath` on this computer via `docker cp`
image.copyOut(imagePath, localPath);
// Or alternatively, run the docker image with options.
image.run(dockerRunOptions);

Edit: I've pushed up a POC. It's a much smaller change. :) I'll see about adding an integration test in S3 assets. I ran out of time today.

eladb

I like this direction. See some comments

eladb · 2020-09-24T04:22:38Z

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

@@ -110,6 +110,7 @@ export class BundlingDockerImage {

 const dockerArgs: string[] = [
 'build', '-q',
+ ...(options.file ? ['-f', options.file] : []),


I vaguely recall this is already supported. Did you rebase?

@eladb I wiped out my previous branch and started fresh from the master branch today - the old commits were not practical to revert. I used the force to push it over the branch in this PR.

I didn't see pre-existing support for this in master. However, I've got practically the same code in PR #9582, which I believe you've previously reviewed. Maybe it was seen there? Not sure.

It's currently supported in cdk-assets.

The API of the Docker class in cdk-assets looks pretty handy to use. But, both core and cdk-assets seem to have minimal dependencies. Any ideas what I should do here?

Nothing at this stage, I was just mentioning from where @eladb might have thought that this was already supported.

Not for this PR but at some point it might be interesting to correctly define the "roles" of core vs cdk-assets. I think that this was already discussed in another issue about where zipping should occur.

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

eladb · 2020-09-24T04:26:52Z

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

@@ -110,6 +110,7 @@ export class BundlingDockerImage {



@jogold does it make sense to rename this to DockerImage? What's "bundling" about this?

It's the docker image used when bundling an asset/accepted in the bundling options... but I agree that there's nothing specific to bundling in its implementation.

Let's leave this for a future change

Pull request has been modified.

misterjoshua · 2020-09-24T07:04:10Z

@eladb I've pushed up changes from your review. It's time to sleep, so I'll check in later. :)

eladb

Minor

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

misterjoshua · 2020-09-24T18:15:16Z

@eladb I've made your cp() change and updated from the master branch. Is there anything else you'd like to see added or changed?

misterjoshua · 2020-10-06T15:56:11Z

I see some merge conflicts have arisen. I'll see what's required and push up a commit.

Pull request has been modified.

misterjoshua · 2020-10-06T16:35:55Z

@eladb I resolved a conflict due to the new nodeunitShim, so I think this may need another review. Though the fix was basically a parenthesis.

mergify · 2020-10-11T11:58:22Z

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

aws-cdk-automation · 2020-10-11T12:28:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c35b0f7
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-10-11T12:29:06Z

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

misterjoshua marked this pull request as ready for review August 14, 2020 23:45

misterjoshua marked this pull request as draft August 15, 2020 01:50

misterjoshua changed the title ~~feat(lambda): add lambda.Code.fromDocker()~~ feat(lambda): add lambda.Code.fromDockerBuild() Aug 15, 2020

misterjoshua marked this pull request as ready for review August 16, 2020 17:08

misterjoshua mentioned this pull request Aug 16, 2020

feat(lambda-python): bundle dependencies in a lambda layer #9582

Merged

jogold reviewed Aug 17, 2020

View reviewed changes

packages/@aws-cdk/aws-lambda/lib/code.ts Outdated Show resolved Hide resolved

SomayaB assigned nija-at Aug 17, 2020

misterjoshua changed the title ~~feat(lambda): add lambda.Code.fromDockerBuild()~~ chore(core): add copyFromImagePath to the bundler options Aug 18, 2020

nija-at assigned eladb and unassigned nija-at Aug 21, 2020

nija-at previously requested changes Aug 21, 2020

View reviewed changes

packages/@aws-cdk/aws-lambda/test/integ.from-docker-build.complex.ts Outdated Show resolved Hide resolved

misterjoshua changed the title ~~chore(core): add copyFromImagePath to the bundler options~~ feat(core): add copyFromImagePath to the bundler options Aug 21, 2020

jogold reviewed Sep 7, 2020

View reviewed changes

packages/@aws-cdk/aws-s3-assets/README.md Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-s3-assets/README.md Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-s3-assets/lib/asset.ts Outdated Show resolved Hide resolved

misterjoshua added 3 commits September 23, 2020 16:11

Add dockerfile path support and dockerCopyFromImage

1b2da46

Expose BundlingDockerImage.run

c112486

Add BundlingDockerImage.copyOut

8485802

misterjoshua force-pushed the from-docker branch from 77c5e80 to 8485802 Compare September 23, 2020 22:34

eladb previously requested changes Sep 24, 2020

View reviewed changes

Pull dockerCopyFromImage into BundlingDockerImage.copyOut

e002ef7

misterjoshua changed the title ~~feat(core): add copyFromImagePath to the bundler options~~ feat(core): add run and copyOut methods to BundlingDockerImage Sep 24, 2020

Clean up test names and inline docs

f18d976

eladb changed the title ~~feat(core): add run and copyOut methods to BundlingDockerImage~~ feat(core): BundlingDockerImage now supports run and cp methods Sep 24, 2020

eladb changed the title ~~feat(core): BundlingDockerImage now supports run and cp methods~~ feat(core): BundlingDockerImage now supports run() and cp() utilities Sep 24, 2020

eladb reviewed Sep 24, 2020

View reviewed changes

packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved

misterjoshua added 3 commits September 24, 2020 09:51

Rename copyOut to cp to align with docker

fa539cb

Merge branch 'master' of github.com:aws/aws-cdk into from-docker

c03391a

Re-trigger the build, hoping dockerhub 500s went away

f0224c2

eladb previously approved these changes Oct 4, 2020

View reviewed changes

Merge branch 'master' of github.com:aws/aws-cdk into from-docker

f50ce47

eladb added the pr-linter/exempt-readme The PR linter will not require README changes label Oct 6, 2020

eladb approved these changes Oct 6, 2020

View reviewed changes

Merge branch 'master' into from-docker

c35b0f7

mergify bot merged commit 37fdc94 into aws:master Oct 11, 2020

jogold mentioned this pull request Dec 8, 2020

(core): BundlingDockerImage.cp() needs to be explained more in the README #11914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): `BundlingDockerImage` now supports `run()` and `cp()` utilities #9728

feat(core): `BundlingDockerImage` now supports `run()` and `cp()` utilities #9728

misterjoshua commented Aug 14, 2020 •

edited

Loading

misterjoshua commented Aug 18, 2020

nija-at left a comment

eladb commented Aug 26, 2020

misterjoshua commented Aug 31, 2020

eladb commented Sep 7, 2020

eladb commented Sep 22, 2020

misterjoshua commented Sep 22, 2020 •

edited

Loading

eladb commented Sep 23, 2020

misterjoshua commented Sep 23, 2020 •

edited

Loading

eladb left a comment

eladb Sep 24, 2020

misterjoshua Sep 24, 2020 •

edited

Loading

jogold Sep 24, 2020

misterjoshua Sep 24, 2020

jogold Sep 24, 2020

eladb Sep 24, 2020

jogold Sep 24, 2020

eladb Oct 4, 2020

misterjoshua commented Sep 24, 2020

eladb left a comment

misterjoshua commented Sep 24, 2020 •

edited

Loading

misterjoshua commented Oct 6, 2020

misterjoshua commented Oct 6, 2020

mergify bot commented Oct 11, 2020

aws-cdk-automation commented Oct 11, 2020

mergify bot commented Oct 11, 2020

feat(core): BundlingDockerImage now supports run() and cp() utilities #9728

feat(core): BundlingDockerImage now supports run() and cp() utilities #9728

Conversation

misterjoshua commented Aug 14, 2020 • edited Loading

misterjoshua commented Aug 18, 2020

nija-at left a comment

Choose a reason for hiding this comment

eladb commented Aug 26, 2020

misterjoshua commented Aug 31, 2020

eladb commented Sep 7, 2020

eladb commented Sep 22, 2020

misterjoshua commented Sep 22, 2020 • edited Loading

eladb commented Sep 23, 2020

misterjoshua commented Sep 23, 2020 • edited Loading

eladb left a comment

Choose a reason for hiding this comment

eladb Sep 24, 2020

Choose a reason for hiding this comment

misterjoshua Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

jogold Sep 24, 2020

Choose a reason for hiding this comment

misterjoshua Sep 24, 2020

Choose a reason for hiding this comment

jogold Sep 24, 2020

Choose a reason for hiding this comment

eladb Sep 24, 2020

Choose a reason for hiding this comment

jogold Sep 24, 2020

Choose a reason for hiding this comment

eladb Oct 4, 2020

Choose a reason for hiding this comment

misterjoshua commented Sep 24, 2020

eladb left a comment

Choose a reason for hiding this comment

misterjoshua commented Sep 24, 2020 • edited Loading

misterjoshua commented Oct 6, 2020

misterjoshua commented Oct 6, 2020

mergify bot commented Oct 11, 2020

aws-cdk-automation commented Oct 11, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 11, 2020

feat(core): `BundlingDockerImage` now supports `run()` and `cp()` utilities #9728

feat(core): `BundlingDockerImage` now supports `run()` and `cp()` utilities #9728

misterjoshua commented Aug 14, 2020 •

edited

Loading

misterjoshua commented Sep 22, 2020 •

edited

Loading

misterjoshua commented Sep 23, 2020 •

edited

Loading

misterjoshua Sep 24, 2020 •

edited

Loading

misterjoshua commented Sep 24, 2020 •

edited

Loading