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

fix(docker): cannot use cdk docker assets as base image #6471

Merged
merged 2 commits into from
Feb 27, 2020
Merged

fix(docker): cannot use cdk docker assets as base image #6471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6ae88f
fix(docker): cannot use cdk docker assets as base image
jogold Feb 26, 2020
495c711
Merge branch 'master' into docker-login-before-build
mergify[bot] Feb 27, 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
6 changes: 3 additions & 3 deletions packages/aws-cdk/lib/docker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,17 +172,17 @@ export async function prepareContainerAssetNew(assemblyDir: string,

buildCommand.push(contextPath);

// Login to ECR before build to allow building images that reference other docker assets
try {
await shell(buildCommand);
await dockerLogin(toolkitInfo);
} catch (e) {
if (e.code === 'ENOENT') {
throw new Error('Unable to execute "docker" in order to build a container asset. Please install "docker" and try again.');
}
throw e;
}

// login to ECR
await dockerLogin(toolkitInfo);
await shell(buildCommand);

// There's no way to make this quiet, so we can't use a PleaseHold. Print a header message.
print(` ⌛ Pushing Docker image for ${contextPath}; this may take a while.`);
Expand Down
88 changes: 84 additions & 4 deletions packages/aws-cdk/test/docker-new.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@ import { prepareContainerAsset } from '../lib/docker';
import * as os from '../lib/os';
import { MockSDK } from './util/mock-sdk';

const shellFake = async (args: string[]) => {
if (args[1] === 'login') {
return 'LOGIN';
}

if (args[1] === 'build') {
throw new Error('STOPTEST');
}

throw new Error('UNEXPECTED');
};

test('fails if "repositoryName" and "imageTag" are not specified', async () => {
// GIVEN
const toolkit = newMockToolkitInfo();
Expand Down Expand Up @@ -118,13 +130,62 @@ test('configures image scanning', async () => {
});
});

test('calls docker login with the corrects args and before docker build', async () => {
// GIVEN
const toolkit = newMockToolkitInfo();

const prepareEcrRepositoryStub = sinon.stub(toolkit, 'prepareEcrRepository').resolves({ repositoryUri: 'uri' });
const checkEcrImageStub = sinon.stub(toolkit, 'checkEcrImage').resolves(false);
const shellStub = sinon.stub(os, 'shell').callsFake(shellFake);
const getEcrCredentialsStub = sinon.stub(toolkit, 'getEcrCredentials').resolves({
username: 'username',
password: 'password',
endpoint: 'endpoint'
});

// WHEN
const asset: cxapi.ContainerImageAssetMetadataEntry = {
id: 'assetId',
packaging: 'container-image',
path: '/foo',
sourceHash: '1234567890abcdef',
repositoryName: 'some-name',
imageTag: 'some-tag',
buildArgs: {
a: 'b',
c: 'd'
},
target: 'a-target',
};

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

// THEN
const command = ['docker', 'login', '--username', 'username', '--password', 'password', 'endpoint'];
sinon.assert.calledWith(shellStub.firstCall, command);

prepareEcrRepositoryStub.restore();
shellStub.restore();
checkEcrImageStub.restore();
getEcrCredentialsStub.restore();
});

test('passes the correct target to docker build', async () => {
// GIVEN
const toolkit = newMockToolkitInfo();

const prepareEcrRepositoryStub = sinon.stub(toolkit, 'prepareEcrRepository').resolves({ repositoryUri: 'uri' });
const checkEcrImageStub = sinon.stub(toolkit, 'checkEcrImage').resolves(false);
const shellStub = sinon.stub(os, 'shell').rejects('STOPTEST');
const shellStub = sinon.stub(os, 'shell').callsFake(shellFake);
const getEcrCredentialsStub = sinon.stub(toolkit, 'getEcrCredentials').resolves({
username: 'username',
password: 'password',
endpoint: 'endpoint'
});

// WHEN
const asset: cxapi.ContainerImageAssetMetadataEntry = {
Expand Down Expand Up @@ -154,6 +215,7 @@ test('passes the correct target to docker build', async () => {
prepareEcrRepositoryStub.restore();
shellStub.restore();
checkEcrImageStub.restore();
getEcrCredentialsStub.restore();
});

test('passes the correct args to docker build', async () => {
Expand All @@ -162,7 +224,12 @@ test('passes the correct args to docker build', async () => {

const prepareEcrRepositoryStub = sinon.stub(toolkit, 'prepareEcrRepository').resolves({ repositoryUri: 'uri' });
const checkEcrImageStub = sinon.stub(toolkit, 'checkEcrImage').resolves(false);
const shellStub = sinon.stub(os, 'shell').rejects('STOPTEST');
const shellStub = sinon.stub(os, 'shell').callsFake(shellFake);
const getEcrCredentialsStub = sinon.stub(toolkit, 'getEcrCredentials').resolves({
username: 'username',
password: 'password',
endpoint: 'endpoint'
});

// WHEN
const asset: cxapi.ContainerImageAssetMetadataEntry = {
Expand Down Expand Up @@ -191,13 +258,19 @@ test('passes the correct args to docker build', async () => {
prepareEcrRepositoryStub.restore();
checkEcrImageStub.restore();
shellStub.restore();
getEcrCredentialsStub.restore();
});

test('passes the correct docker file name if specified', async () => {
const toolkit = newMockToolkitInfo();
const prepareEcrRepositoryStub = sinon.stub(toolkit, 'prepareEcrRepository').resolves({ repositoryUri: 'uri' });
const checkEcrImageStub = sinon.stub(toolkit, 'checkEcrImage').resolves(false);
const shellStub = sinon.stub(os, 'shell').rejects('STOPTEST');
const shellStub = sinon.stub(os, 'shell').callsFake(shellFake);
const getEcrCredentialsStub = sinon.stub(toolkit, 'getEcrCredentials').resolves({
username: 'username',
password: 'password',
endpoint: 'endpoint'
});

// WHEN
const asset: cxapi.ContainerImageAssetMetadataEntry = {
Expand Down Expand Up @@ -227,14 +300,20 @@ test('passes the correct docker file name if specified', async () => {
prepareEcrRepositoryStub.restore();
checkEcrImageStub.restore();
shellStub.restore();
getEcrCredentialsStub.restore();
});

test('relative path', async () => {
// GIVEN
const toolkit = newMockToolkitInfo();
const prepareEcrRepositoryStub = sinon.stub(toolkit, 'prepareEcrRepository').resolves({ repositoryUri: 'uri' });
const checkEcrImageStub = sinon.stub(toolkit, 'checkEcrImage').resolves(false);
const shellStub = sinon.stub(os, 'shell').rejects('STOPTEST');
const shellStub = sinon.stub(os, 'shell').callsFake(shellFake);
const getEcrCredentialsStub = sinon.stub(toolkit, 'getEcrCredentials').resolves({
username: 'username',
password: 'password',
endpoint: 'endpoint'
});

// WHEN
const asset: cxapi.ContainerImageAssetMetadataEntry = {
Expand Down Expand Up @@ -263,6 +342,7 @@ test('relative path', async () => {
prepareEcrRepositoryStub.restore();
checkEcrImageStub.restore();
shellStub.restore();
getEcrCredentialsStub.restore();
});

test('skips build & push if image already exists in the ECR repo', async () => {
Expand Down