Skip to content

Commit

Permalink
fix(cdk-assets): context path not honored by Docker asset build (#6957)
Browse files Browse the repository at this point in the history
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as `.`. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: #6954 #6814
  • Loading branch information
MrArnoldPalmer committed Mar 24, 2020
1 parent 73899a9 commit 1edd507
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 8 deletions.
6 changes: 3 additions & 3 deletions packages/cdk-assets/lib/private/docker.ts
Expand Up @@ -37,9 +37,9 @@ export class Docker {
'--tag', options.tag,
...options.target ? ['--target', options.target] : [],
...options.file ? ['--file', options.file] : [],
options.directory,
'.'
];
await this.execute(buildCommand);
await this.execute(buildCommand, { cwd: options.directory });
}

/**
Expand Down Expand Up @@ -100,4 +100,4 @@ async function obtainEcrCredentials(ecr: AWS.ECR, logger?: Logger) {

function flatten(x: string[][]) {
return Array.prototype.concat([], ...x);
}
}
7 changes: 4 additions & 3 deletions packages/cdk-assets/test/docker-images.test.ts
Expand Up @@ -7,6 +7,7 @@ import { mockAws, mockedApiFailure, mockedApiResult } from './mock-aws';
import { mockSpawn } from './mock-child_process';

let aws: ReturnType<typeof mockAws>;
const absoluteDockerPath = '/simple/cdk.out/dockerdir';
beforeEach(() => {
mockfs({
'/simple/cdk.out/assets.json': JSON.stringify({
Expand All @@ -33,7 +34,7 @@ beforeEach(() => {
dockerImages: {
theAsset: {
source: {
directory: '/simple/cdk.out/dockerdir'
directory: absoluteDockerPath
},
destinations: {
theDestination: {
Expand Down Expand Up @@ -112,7 +113,7 @@ describe('with a complete manifest', () => {
mockSpawn(
{ commandLine: ['docker', 'login', '--username', 'user', '--password-stdin', 'https://proxy.com/'] },
{ commandLine: ['docker', 'inspect', 'cdkasset-theasset'], exitCode: 1 },
{ commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset', '/simple/cdk.out/dockerdir'] },
{ commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset', '.'], cwd: absoluteDockerPath },
{ commandLine: ['docker', 'tag', 'cdkasset-theasset', '12345.amazonaws.com/repo:abcdef'] },
{ commandLine: ['docker', 'push', '12345.amazonaws.com/repo:abcdef'] },
);
Expand All @@ -135,7 +136,7 @@ test('correctly identify Docker directory if path is absolute', async () => {
// Only care about the 'build' command line
{ commandLine: ['docker', 'login'], prefix: true, },
{ commandLine: ['docker', 'inspect'], exitCode: 1, prefix: true },
{ commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset', '/simple/cdk.out/dockerdir'] },
{ commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset', '.'], cwd: absoluteDockerPath },
{ commandLine: ['docker', 'tag'], prefix: true },
{ commandLine: ['docker', 'push'], prefix: true },
);
Expand Down
9 changes: 7 additions & 2 deletions packages/cdk-assets/test/mock-child_process.ts
Expand Up @@ -7,6 +7,7 @@ if (!(child_process as any).spawn.mockImplementationOnce) {

export interface Invocation {
commandLine: string[];
cwd?: string;
exitCode?: number;
stdout?: string;

Expand All @@ -20,7 +21,7 @@ export function mockSpawn(...invocations: Invocation[]) {
let mock = (child_process.spawn as any);
for (const _invocation of invocations) {
const invocation = _invocation; // Mirror into variable for closure
mock = mock.mockImplementationOnce((binary: string, args: string[], _options: any) => {
mock = mock.mockImplementationOnce((binary: string, args: string[], options: child_process.SpawnOptions) => {
if (invocation.prefix) {
// Match command line prefix
expect([binary, ...args].slice(0, invocation.commandLine.length)).toEqual(invocation.commandLine);
Expand All @@ -29,6 +30,10 @@ export function mockSpawn(...invocations: Invocation[]) {
expect([binary, ...args]).toEqual(invocation.commandLine);
}

if (invocation.cwd != null) {
expect(options.cwd).toBe(invocation.cwd);
}

const child: any = new events.EventEmitter();
child.stdin = new events.EventEmitter();
child.stdin.write = jest.fn();
Expand Down Expand Up @@ -57,4 +62,4 @@ function mockEmit(emitter: events.EventEmitter, event: string, data: any) {
setImmediate(() => {
emitter.emit(event, data);
});
}
}

0 comments on commit 1edd507

Please sign in to comment.