feat(ecr-assets): custom docker files by iliapolo · Pull Request #5652 · aws/aws-cdk

iliapolo · 2020-01-05T22:07:20Z

Added ability to provide a custom Dockerfile in the relevant asset constructs.

Those are:

Things to note

The file property is assumed to be a relative path inside the asset directory. While its possible to specify a Dockerfile outside the context path, in our case, we want to keep things as self-contained as possible inside the asset directory. If a different need arrises, we will address it then.
If the property is not defined, we use Dockerfile as the default. This happens in the constructor of DockerImageAsset.
Resolving and validating the Dockerfile is done as soon as possible --> in the constructor. I didn't want the resolving logic to appear twice, so the prepareContainerAsset function simply adds the --file option to the docker command as is (if given).
Prior to this change, users had to have a Dockerfile (hardcoded) in their asset directory.

Closes #3829

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

iliapolo · 2020-01-05T22:24:05Z

@eladb Question:

I've changed the way we invoke docker build command. Is there an integration test covering this? How/When are those executed?

aws-cdk-automation · 2020-01-05T22:34:30Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-05T22:39:11Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-05T22:54:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Looks great. Minor comments.

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

packages/@aws-cdk/aws-ecs/lib/images/asset-image.ts

packages/@aws-cdk/cx-api/lib/assets.ts

aws-cdk-automation · 2020-01-06T08:58:23Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-06T08:58:50Z

Thank you for contributing! Your pull request is now being automatically merged.

eladb · 2020-01-15T13:03:18Z

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

      directoryName: staging.stagedPath,
      dockerBuildArgs: props.buildArgs,
      dockerBuildTarget: props.target,
+      dockerFile: file,


This needs to be a relative path since the docker directory is copied into cloud assembly ("staging"). Since we resolve dir above, it results in an absolute path.

I think props.file would just work here

This was actually done intentionally. The idea was to do the validation as soon as possible (like the other validations). To validate - I have to join the directory and the file. If I were to pass only the relative path here, it means the cli would have to join again - resulting in duplicate logic. I think its also inline with the notion of letting the framework do the work and keeping the cli as simple as possible.

I think what I missed though is that the final command would look something like:

docker build --file ${props.directory}/${props.file} /generated/path/to/cloudassemby/....

Which means the command relies on resources outside the cloudassemly. Is this what you meant by:

needs to be a relative path since the docker directory is copied into cloud assembly ?

Ok that the validation is done early, but the value passed to "docker build" needs to be relative to the docker directory.

Actually the value can't be relative because docker will fail - it doesn't assume you pass a relative path to the context when you use --file.

The path passed to docker build --file should be absolute, it just needs to be derived from the asset path inside the cloud assembly. What has to be relative is the property passed to the asset, since otherwise, the manifest in the cloud-assembly will contain an absolute path of the machine the assembly was created on.

eladb · 2020-01-15T13:04:36Z

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

+
+    // THEN
+    const assetMetadata = stack.node.metadata.find(({ type }) => type === ASSET_METADATA);
+    test.deepEqual(assetMetadata && assetMetadata.data.file, path.join(directoryPath, 'Dockerfile.Custom'));


This assertion should have just been:

Suggested change

test.deepEqual(assetMetadata && assetMetadata.data.file, path.join(directoryPath, 'Dockerfile.Custom'));

test.deepEqual(assetMetadata && assetMetadata.data.file, 'Dockerfile.Custom');

nalhabash · 2020-01-21T22:22:54Z

Has anyone been able to use custom dockerfiles since the 1.20 release ?
Trying right now in my stack, with v1.21.1.

const nginxImage = new DockerImageAsset(this, 'NginxDockerImage', {
            directory: __dirname + '/../../app/',
            target: 'some_target',
            repositoryName: `some_repo`,
            file: 'Dockerfile.nginx',
        });

But I am getting this error Error response from daemon: failed to reach build target some_target in Dockerfile.

I have another php image that I build from a Dockerfile file, and it works fine.

iliapolo · 2020-01-22T08:34:27Z

Hi @nalhabash

We indeed had an issue regarding this feature in 1.20.0 and 1.21.0, but version 1.21.1 should work properly. You can have a look here for some more information.

Regarding your specific issue, the error you are getting means that the Dockerfile.nginx file is missing the some_target target. Are you sure you defined it?

If possible, can you share your project structure and the contents of your Dockerfile.nginx file? This will help us figure out this issue quickly.

Thanks

nalhabash · 2020-01-23T15:34:14Z

Hi !
I don't know what is going on, I reverted to reproduce the issue and it looks like it works now. It might be that I somehow forgot to run npm run build or something silly like that, I don't know.
Still thanks for replying @iliapolo !

iliapolo · 2020-01-24T16:05:09Z

Awesome, glad all is well 👍

support custom docker files

30b75f0

iliapolo requested review from RomainMuller, SoManyHs, eladb and rix0rrr as code owners January 5, 2020 22:07

iliapolo added 2 commits January 6, 2020 00:09

Merge branch 'master' into epolon/custom-docker-files

bf319f4

revert tsconfig changes

119458f

eladb reviewed Jan 6, 2020

View reviewed changes

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

packages/@aws-cdk/aws-ecs/lib/images/asset-image.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/cx-api/lib/assets.ts Outdated Show resolved Hide resolved

iliapolo added 2 commits January 6, 2020 10:19

doc strings modification according to conventions

d93c246

Merge branch 'master' into epolon/custom-docker-files

da68fa8

eladb approved these changes Jan 6, 2020

View reviewed changes

mergify bot merged commit 1b25a4b into master Jan 6, 2020

mergify bot deleted the epolon/custom-docker-files branch January 6, 2020 08:58

eladb reviewed Jan 15, 2020

View reviewed changes

eladb mentioned this pull request Jan 15, 2020

docker assets: cannot build docker images outside the source tree (i.e. against a cdk.out directory) #5807

Closed

	test.deepEqual(assetMetadata && assetMetadata.data.file, path.join(directoryPath, 'Dockerfile.Custom'));
	test.deepEqual(assetMetadata && assetMetadata.data.file, 'Dockerfile.Custom');

Conversation

iliapolo commented Jan 5, 2020 • edited by eladb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things to note

Uh oh!

iliapolo commented Jan 5, 2020

Uh oh!

aws-cdk-automation commented Jan 5, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Jan 5, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Jan 5, 2020

AWS CodeBuild CI Report

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 6, 2020

Uh oh!

eladb Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

eladb Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

iliapolo Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

eladb Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

iliapolo Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

eladb Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

nalhabash commented Jan 21, 2020

Uh oh!

iliapolo commented Jan 22, 2020

Uh oh!

nalhabash commented Jan 23, 2020

Uh oh!

iliapolo commented Jan 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iliapolo commented Jan 5, 2020 •

edited by eladb

Loading