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.

Sign up for GitHub

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

feat(ecr-assets): custom docker files #5652

Merged
merged 5 commits into from
Jan 6, 2020
Merged

feat(ecr-assets): custom docker files #5652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30b75f0
support custom docker files
iliapolo Jan 5, 2020
bf319f4
Merge branch 'master' into epolon/custom-docker-files
iliapolo Jan 5, 2020
119458f
revert tsconfig changes
iliapolo Jan 5, 2020
d93c246
doc strings modification according to conventions
iliapolo Jan 6, 2020
da68fa8
Merge branch 'master' into epolon/custom-docker-files
iliapolo Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ export interface DockerImageAssetProps extends assets.CopyOptions {
* @default - no target
*/
readonly target?: string;

/**
* Path to the Dockerfile (relative to the directory).
*
* @default 'Dockerfile'
*/
readonly file?: string;
}

/**
Expand Down Expand Up @@ -71,8 +78,11 @@ export class DockerImageAsset extends Construct implements assets.IAsset {
if (!fs.existsSync(dir)) {
throw new Error(`Cannot find image directory at ${dir}`);
}
if (!fs.existsSync(path.join(dir, 'Dockerfile'))) {
throw new Error(`No 'Dockerfile' found in ${dir}`);

// validate the docker file exists
const file = path.join(dir, props.file || 'Dockerfile');
if (!fs.existsSync(file)) {
throw new Error(`Cannot find file at ${file}`);
}

let exclude: string[] = props.exclude || [];
Expand All @@ -96,6 +106,7 @@ export class DockerImageAsset extends Construct implements assets.IAsset {
directoryName: staging.stagedPath,
dockerBuildArgs: props.buildArgs,
dockerBuildTarget: props.target,
dockerFile: file,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think props.file would just work here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

repositoryName: props.repositoryName || `cdk/${this.node.uniqueId.replace(/[:/]/g, '-').toLowerCase()}`,
sourceHash: staging.sourceHash
});
Expand Down
5 changes: 5 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/test/demo-image-custom-docker-file/Dockerfile.Custom
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM python:3.6
EXPOSE 8000
WORKDIR /src
ADD . /src
CMD python3 index.py
33 changes: 33 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/test/demo-image-custom-docker-file/index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#!/usr/bin/python
import sys
import textwrap
import http.server
import socketserver

PORT = 8000


class Handler(http.server.SimpleHTTPRequestHandler):
def do_GET(self):
self.send_response(200)
self.send_header('Content-Type', 'text/html')
self.end_headers()
self.wfile.write(textwrap.dedent('''\
<!doctype html>
<html><head><title>It works</title></head>
<body>
<h1>Hello from the integ test container</h1>
<p>This container got built and started as part of the integ test.</p>
<img src="https://media.giphy.com/media/nFjDu1LjEADh6/giphy.gif">
</body>
''').encode('utf-8'))


def main():
httpd = http.server.HTTPServer(("", PORT), Handler)
print("serving at port", PORT)
httpd.serve_forever()


if __name__ == '__main__':
main()
33 changes: 32 additions & 1 deletion packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@ export = {
test.done();
},

'with file'(test: Test) {
// GIVEN
const stack = new Stack();

const directoryPath = path.join(__dirname, 'demo-image-custom-docker-file');
// WHEN
new DockerImageAsset(stack, 'Image', {
directory: directoryPath,
file: 'Dockerfile.Custom'
});

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

Choose a reason for hiding this comment

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

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');

test.done();
},

'asset.repository.grantPull can be used to grant a principal permissions to use the image'(test: Test) {
// GIVEN
const stack = new Stack();
Expand Down Expand Up @@ -211,7 +228,21 @@ export = {
new DockerImageAsset(stack, 'Asset', {
directory: __dirname
});
}, /No 'Dockerfile' found in/);
}, /Cannot find file at/);
test.done();
},

'fails if the file does not exist'(test: Test) {
// GIVEN
const stack = new Stack();

// THEN
test.throws(() => {
new DockerImageAsset(stack, 'Asset', {
directory: __dirname,
file: 'doesnt-exist'
});
}, /Cannot find file at/);
test.done();
},

Expand Down
9 changes: 9 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/images/asset-image.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ export interface AssetImageProps {
* @default none
*/
readonly target?: string;

/**
* Path to the Dockerfile (relative to the directory).
*
* @default 'Dockerfile'
*/
readonly file?: string;

}

/**
Expand All @@ -40,6 +48,7 @@ export class AssetImage extends ContainerImage {
directory: this.directory,
buildArgs: this.props.buildArgs,
target: this.props.target,
file: this.props.file,
});
asset.repository.grantPull(containerDefinition.taskDefinition.obtainExecutionRole());

Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/core/lib/assets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ export interface DockerImageAssetSource {
*/
readonly dockerBuildTarget?: string;

/**
* Path to the Dockerfile (relative to the directory).
*
* @default - no file
*/
readonly dockerFile?: string;

/**
* ECR repository name
*
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/core/lib/stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,8 @@ export class Stack extends Construct implements ITaggable {
imageNameParameter: params.imageNameParameter.logicalId,
repositoryName: asset.repositoryName,
buildArgs: asset.dockerBuildArgs,
target: asset.dockerBuildTarget
target: asset.dockerBuildTarget,
file: asset.dockerFile,
};

this.node.addMetadata(cxapi.ASSET_METADATA, metadata);
Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/cx-api/lib/assets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ export interface ContainerImageAssetMetadataEntry extends BaseAssetMetadataEntry
* @default no build target
*/
readonly target?: string;

/**
* Path to the Dockerfile (relative to the directory).
*
* @default - no file is passed
*/
readonly file?: string;

}

export type AssetMetadataEntry = FileAssetMetadataEntry | ContainerImageAssetMetadataEntry;
4 changes: 4 additions & 0 deletions packages/aws-cdk/lib/docker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ export async function prepareContainerAsset(assemblyDir: string,
baseCommand.push('--target', asset.target);
}

if (asset.file) {
baseCommand.push('--file', asset.file);
}

const command = ci
? [...baseCommand, '--cache-from', latest] // This does not fail if latest is not available
: baseCommand;
Expand Down
43 changes: 43 additions & 0 deletions packages/aws-cdk/test/test.docker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,49 @@ export = {
test.done();
},

async 'passes the correct file to docker build'(test: Test) {
// GIVEN
const toolkit = new ToolkitInfo({
sdk: new MockSDK(),
bucketName: 'BUCKET_NAME',
bucketEndpoint: 'BUCKET_ENDPOINT',
environment: { name: 'env', account: '1234', region: 'abc' }
});

const prepareEcrRepositoryStub = sinon.stub(toolkit, 'prepareEcrRepository').resolves({
repositoryUri: 'uri',
repositoryName: 'name'
});

const shellStub = sinon.stub(os, 'shell').rejects('STOPTEST');

// WHEN
const asset: cxapi.ContainerImageAssetMetadataEntry = {
id: 'assetId',
imageNameParameter: 'MyParameter',
packaging: 'container-image',
path: '/foo',
sourceHash: '1234567890abcdef',
repositoryName: 'some-name',
file: 'some-file'
};

try {
await prepareContainerAsset('.', asset, toolkit, false);
} catch (e) {
if (!/STOPTEST/.test(e.toString())) { throw e; }
}

// THEN
const command = ['docker', 'build', '--tag', `uri:latest`, '/foo', '--file', 'some-file'];

test.ok(shellStub.calledWith(command));

prepareEcrRepositoryStub.restore();
shellStub.restore();
test.done();
},

async 'relative path'(test: Test) {
// GIVEN
const toolkit = new ToolkitInfo({
Expand Down