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

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

Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. docs/inline Related to inline documentation of the API Reference documentation This is a problem with documentation. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@dmlook-pillpack
Copy link

When using lambda.Code.fromAsset and cdk.BundlingDockerImage.fromAsset together, synth fails to find anything in \asset-output

Reproduction Steps

  1. Create a Dockerfile that compiles and copies files to an /asset-output directory
FROM python:3.7-slim
COPY . /asset-input
COPY . /asset-output
WORKDIR /asset-input
RUN apt-get update && apt-get -y install curl make automake gcc g++ subversion python3-dev
RUN curl -sSL https://raw.githubusercontent.com/python-poetry/poetry/master/get-poetry.py | python3 -
ENV PATH "/root/.poetry/bin:/opt/venv/bin:${PATH}"
RUN poetry export -f requirements.txt -o requirements.txt
RUN pip3 install -r requirements.txt -t /asset-output
  1. Use the following snippet when creating the lambda using cdk:
code: lambda.Code.fromAsset(PROJECT_DIR, {
        bundling: {
          image: cdk.BundlingDockerImage.fromAsset(PROJECT_DIR)
        }
      }),
  1. Run tsc && cdk synth -o cdk.out

What did you expect to happen?

Docker should find the compiled assets in /asset-output

What actually happened?

Error: Bundling did not produce any output. Check that content is written to /asset-output.

Environment

  • CDK CLI Version : 1.75.0
  • Framework Version: 1.75.0
  • Node.js Version: v15.3.0
  • OS : Mac Catalina 10.15.7
  • Language (Version): TypeScript 4.1.2

Other

If I use an implementation of ILocalBundling that is mostly copied from asset-staging.ts but calls both run and cp the synth works but I don't believe that should be necessary:

class LocalBundling implements ILocalBundling {
  tryBundle(outputDir: string, options: BundlingOptions): boolean {

    let user: string;
    if (options.user) {
      user = options.user;
    } else {
      // Default to current user
      const userInfo = os.userInfo();
      user =
        userInfo.uid !== -1 // uid is -1 on Windows
          ? `${userInfo.uid}:${userInfo.gid}`
          : "1000:1000";
    }

    // Always mount input and output dir
    const volumes = [
      {
        hostPath: PROJECT_DIR, // this.sourcePath
        containerPath: AssetStaging.BUNDLING_INPUT_DIR,
      },
      {
        hostPath: outputDir, // bundleDir
        containerPath: AssetStaging.BUNDLING_OUTPUT_DIR ?? outputDir,
      },
      ...(options.volumes ?? []),
    ];

    options.image.run({
      command: options.command,
      user,
      volumes,
      environment: options.environment,
      workingDirectory:
        options.workingDirectory ?? AssetStaging.BUNDLING_INPUT_DIR,
    });

    options.image.cp(AssetStaging.BUNDLING_OUTPUT_DIR ?? outputDir, outputDir);

    return true;
  }
}

This is 🐛 Bug Report

@dmlook-pillpack dmlook-pillpack added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 7, 2020
@SomayaB SomayaB changed the title (aws-cdk/core): BundlingDockerImage.fromAsset not finding assets (core): BundlingDockerImage.fromAsset not finding assets Dec 8, 2020
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Dec 8, 2020
@jogold
Copy link
Contributor

jogold commented Dec 8, 2020

I think that what you are looking for is BundlingDockerImage.cp() (introduced in #9728).

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.

/**
* Copies a file or directory out of the Docker image to the local filesystem
*/
public cp(imagePath: string, outputPath: string) {

/asset-input and /asset-output (with mounted volumes) works when running the container not when building it.

@rix0rrr rix0rrr changed the title (core): BundlingDockerImage.fromAsset not finding assets (core): BundlingDockerImage.cp() needs to be explained more in the README Dec 8, 2020
@rix0rrr rix0rrr added docs/inline Related to inline documentation of the API Reference documentation This is a problem with documentation. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1 labels Dec 8, 2020
@dmlook-pillpack
Copy link
Author

So what is the appropriate use of the default bundling behavior? I would assume the snippet in step 2 above would work without needing to implement ILocalBundling to use the cp method.

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Dec 8, 2020
@jogold
Copy link
Contributor

jogold commented Dec 9, 2020

So what is the appropriate use of the default bundling behavior?

The default behavior is to run a command in a container where the source path is mounted at /asset-input and during the execution it should put content at /asset-output. You can see examples here:

@dmlook-pillpack
Copy link
Author

Gotcha, so anything put in /asset-output by the Dockerfile will be cleared out and it is assumed that the command will be the one to transfer the required bundling files?

@jogold
Copy link
Contributor

jogold commented Dec 10, 2020

Gotcha, so anything put in /asset-output by the Dockerfile will be cleared out

anything put in /asset-output when the container runs will be used as the final CDK asset.

and it is assumed that the command will be the one to transfer the required bundling files?

yes, you can do whatever you want as long as you put content in /asset-output at some point, you have access to the original asset in /asset-input. A trivial example would be cp -R /asset-input/* /asset-output.

@eladb
Copy link
Contributor

eladb commented Dec 10, 2020

I also ran into a situation where I just wanted to use some content from the built image as the asset output. I think our APIs can probably offer a better experience for this.

  1. In this case the asset input is meaningless.
  2. Ideally docker cp will be much faster to extract files from the built image as oppose to running a command inside the image.

@jogold what do you think?

@jogold
Copy link
Contributor

jogold commented Dec 10, 2020

You can already do this:

const assetPath = '/path/to/my/asset';

const image = cdk.BundlingDockerImage.fromAsset('/path/to/docker');

image.cp('/path/in/the/image', assetPath);

new lambda.Function(this, 'Fn', {
  code: lambda.Code.fromAsset(assetPath),
  runtime: lambda.Runtime.NODEJS_12_X,
  handler: 'index.handler',
});

Is it this that you want to improve?

docker cp is of course faster but has different use cases.

@dmlook-pillpack
Copy link
Author

My issue was that the Dockerfile put everything needed in /asset-output but when the container ran that folder was empty. I am now putting everything in a folder named /asset-stage and passing cp -r ../asset-stage ../asset-output to copy everything from stage to output.

What I would like to see improved is either better documentation around the behavior of the asset-output folder or just simply taking what's already in there instead of wiping it before bundling.

@eladb
Copy link
Contributor

eladb commented Dec 28, 2020

const image = BundlingDockerImage.fromAsset('/path/to/docker');

new lambda.Function(this, 'Fn', {
  code: lambda.Code.fromAsset(image.fetch('/path/in/the/image')),
  runtime: lambda.Runtime.NODEJS_12_X,
  handler: 'index.handler',
});

I think the API for BundlingDockerImage can be improved:

const image = Docker.build('/path/to/docker');
const tmpdir = image.cp('/path/in/the/image');
// alternatively, users can specify the destination for "cp"
image.cp('/path/in/the/image', tmpdir);

And then, we can also add something like:

new lambda.Function(this, 'Fn', {
  code: lambda.Code.fromDockerBuildAsset('/path/in/the/image'),
  runtime: lambda.Runtime.NODEJS_12_X,
  handler: 'index.handler',
});

@jogold
Copy link
Contributor

jogold commented Dec 28, 2020

@eladb

error JSII5016: Members cannot be named "build" as it conflicts with synthetic declarations in some languages.

@eladb
Copy link
Contributor

eladb commented Dec 28, 2020

Haha... so perhaps Docker.fromBuild()?

@jogold
Copy link
Contributor

jogold commented Dec 28, 2020

Haha... so perhaps Docker.fromBuild()?

yes

@jogold
Copy link
Contributor

jogold commented Dec 28, 2020

And then, we can also add something like:

new lambda.Function(this, 'Fn', {
  code: lambda.Code.fromDockerBuildAsset('/path/in/the/image'),
  runtime: lambda.Runtime.NODEJS_12_X,
  handler: 'index.handler',
});

shouldn't this be lambda.Code.fromDockerBuildAsset('/path/to/docker', buildOptions) and the asset is supposed to be located at /asset in the image?

/**
 * Loads the function code from an asset created by a Docker build.
 *
 * The asset is expected to be located at `/asset` in the image.
 *
 * @param path The path to the directory containing the Docker file
 * @param options Docker build options
 */
public static fromDockerBuildAsset(path: string, options: cdk.DockerBuildOptions = {}): AssetCode {
  const assetPath = cdk.DockerImage.fromBuild(path, options).cp('/asset');
  return new AssetCode(assetPath);
}

@mergify mergify bot closed this as completed in #12258 Feb 18, 2021
mergify bot pushed a commit that referenced this issue Feb 18, 2021
Use the result of a Docker build as code. The runtime code is expected to be
located at `/asset` in the image.

Also deprecate `BundlingDockerImage` in favor of `DockerImage`.

Closes #11914

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

NovakGu pushed a commit to NovakGu/aws-cdk that referenced this issue Feb 18, 2021
Use the result of a Docker build as code. The runtime code is expected to be
located at `/asset` in the image.

Also deprecate `BundlingDockerImage` in favor of `DockerImage`.

Closes aws#11914

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This was referenced Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment