Skip to content

Commit

Permalink
chore(lambda-nodejs): prepare code to reduce merge conflicts when dep…
Browse files Browse the repository at this point in the history
…recated APIs are stripped (#13738)

BUNDLING - The `BundlingDockerImage` class is deprecated,
replaced by `DockerImage`. Remove all uses of this class
across modules in a backwards compatible way.

ECR ASSET - Change the way symbols are imported from
'@aws-cdk/assets' so that they can be in-place replaced with
their replacements from '@aws-cdk/core' module.
The `IAsset` interface from the core module introduces the
property `assetHash` instead of `sourceHash`. This change
makes the code forwards compatible so as to be supportive
on v2.

Other small backwards compatible changes.

BREAKING CHANGE: The type of `image` property in the
`Bundling` class is changed from `BundlingDockerImage` to
`DockerImage`.
* **lambda-nodejs**: The type of `dockerImage` property in
`BundlingOptions` is changed from `BundlingDockerImage` to
`DockerImage`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
Niranjan Jayakar authored Mar 24, 2021
1 parent 54dcea2 commit ca391b5
Show file tree
Hide file tree
Showing 13 changed files with 118 additions and 59 deletions.
29 changes: 22 additions & 7 deletions packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
import * as fs from 'fs';
import * as path from 'path';
import * as assets from '@aws-cdk/assets';
import * as ecr from '@aws-cdk/aws-ecr';
import { Annotations, FeatureFlags, IgnoreMode, Stack, Token } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line
import { FingerprintOptions, IAsset, Staging } from '@aws-cdk/assets';
// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct as CoreConstruct } from '@aws-cdk/core';

/**
* Options for DockerImageAsset
*/
export interface DockerImageAssetOptions extends assets.FingerprintOptions {
export interface DockerImageAssetOptions extends FingerprintOptions {
/**
* ECR repository name
*
Expand Down Expand Up @@ -69,7 +71,7 @@ export interface DockerImageAssetProps extends DockerImageAssetOptions {
*
* The image will be created in build time and uploaded to an ECR repository.
*/
export class DockerImageAsset extends CoreConstruct implements assets.IAsset {
export class DockerImageAsset extends CoreConstruct implements IAsset {
/**
* The full URI of the image (including a tag). Use this reference to pull
* the asset.
Expand All @@ -81,8 +83,21 @@ export class DockerImageAsset extends CoreConstruct implements assets.IAsset {
*/
public repository: ecr.IRepository;

/**
* A hash of the source of this asset, which is available at construction time. As this is a plain
* string, it can be used in construct IDs in order to enforce creation of a new resource when
* the content hash has changed.
* @deprecated use assetHash
*/
public readonly sourceHash: string;

/**
* A hash of this asset, which is available at construction time. As this is a plain string, it
* can be used in construct IDs in order to enforce creation of a new resource when the content
* hash has changed.
*/
public readonly assetHash: string;

constructor(scope: Construct, id: string, props: DockerImageAssetProps) {
super(scope, id);

Expand Down Expand Up @@ -141,7 +156,7 @@ export class DockerImageAsset extends CoreConstruct implements assets.IAsset {
// deletion of the ECR repository the app used).
extraHash.version = '1.21.0';

const staging = new assets.Staging(this, 'Staging', {
const staging = new Staging(this, 'Staging', {
...props,
exclude,
ignoreMode,
Expand All @@ -151,16 +166,16 @@ export class DockerImageAsset extends CoreConstruct implements assets.IAsset {
: JSON.stringify(extraHash),
});

this.sourceHash = staging.sourceHash;
this.sourceHash = staging.assetHash;
this.assetHash = staging.assetHash;

const stack = Stack.of(this);
const location = stack.synthesizer.addDockerImageAsset({
directoryName: staging.relativeStagedPath(stack),
dockerBuildArgs: props.buildArgs,
dockerBuildTarget: props.target,
dockerFile: props.file,
repositoryName: props.repositoryName,
sourceHash: staging.sourceHash,
sourceHash: staging.assetHash,
});

this.repository = ecr.Repository.fromRepositoryName(this, 'Repository', location.repositoryName);
Expand Down
62 changes: 31 additions & 31 deletions packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ describe('image asset', () => {

const session = app.synth();

expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'index.py'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'index.py'))).toBeDefined();

});

Expand All @@ -214,16 +214,16 @@ describe('image asset', () => {
const session = app.synth();

// Only the files exempted above should be included.
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, '.dockerignore'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'index.py'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'foobar.txt'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'subdirectory'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'subdirectory', 'baz.txt'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'node_modules'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'node_modules', 'one'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'node_modules', 'some_dep'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'node_modules', 'some_dep', 'file'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, '.dockerignore'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'index.py'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'foobar.txt'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'subdirectory'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'subdirectory', 'baz.txt'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'node_modules'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'node_modules', 'one'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'node_modules', 'some_dep'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'node_modules', 'some_dep', 'file'))).toBeDefined();


});
Expand Down Expand Up @@ -283,13 +283,13 @@ describe('image asset', () => {
const asset6 = new DockerImageAsset(stack, 'Asset6', { directory, extraHash: 'random-extra' });
const asset7 = new DockerImageAsset(stack, 'Asset7', { directory, repositoryName: 'foo' });

expect(asset1.sourceHash).toEqual('ab01ecd4419f59e1ec0ac9e57a60dbb653be68a29af0223fa8cb24b4b747bc73');
expect(asset2.sourceHash).toEqual('7fb12f6148098e3f5c56c788a865d2af689125ead403b795fe6a262ec34384b3');
expect(asset3.sourceHash).toEqual('fc3b6d802ba198ba2ee55079dbef27682bcd1288d5849eb5bbd5cd69038359b3');
expect(asset4.sourceHash).toEqual('30439ea6dfeb4ddfd9175097286895c78393ef52a78c68f92db08abc4513cad6');
expect(asset5.sourceHash).toEqual('5775170880e26ba31799745241b90d4340c674bb3b1c01d758e416ee3f1c386f');
expect(asset6.sourceHash).toEqual('ba82fd351a4d3e3f5c5d948b9948e7e829badc3da90f97e00bb7724afbeacfd4');
expect(asset7.sourceHash).toEqual('26ec194928431cab6ec5af24ea9f01af2cf7b20e361128b07b2a7405d2951f95');
expect(asset1.assetHash).toEqual('ab01ecd4419f59e1ec0ac9e57a60dbb653be68a29af0223fa8cb24b4b747bc73');
expect(asset2.assetHash).toEqual('7fb12f6148098e3f5c56c788a865d2af689125ead403b795fe6a262ec34384b3');
expect(asset3.assetHash).toEqual('fc3b6d802ba198ba2ee55079dbef27682bcd1288d5849eb5bbd5cd69038359b3');
expect(asset4.assetHash).toEqual('30439ea6dfeb4ddfd9175097286895c78393ef52a78c68f92db08abc4513cad6');
expect(asset5.assetHash).toEqual('5775170880e26ba31799745241b90d4340c674bb3b1c01d758e416ee3f1c386f');
expect(asset6.assetHash).toEqual('ba82fd351a4d3e3f5c5d948b9948e7e829badc3da90f97e00bb7724afbeacfd4');
expect(asset7.assetHash).toEqual('26ec194928431cab6ec5af24ea9f01af2cf7b20e361128b07b2a7405d2951f95');

});
});
Expand All @@ -304,12 +304,12 @@ function testDockerDirectoryIsStagedWithoutFilesSpecifiedInDockerignore(app: App
const session = app.synth();

// .dockerignore itself should be included in output to be processed during docker build
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, '.dockerignore'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'index.py'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'foobar.txt'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'subdirectory'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'subdirectory', 'baz.txt'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, '.dockerignore'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'index.py'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'foobar.txt'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'subdirectory'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'subdirectory', 'baz.txt'))).toBeDefined();


}
Expand All @@ -324,12 +324,12 @@ function testDockerDirectoryIsStagedWithoutFilesSpecifiedInExcludeOption(app: Ap

const session = app.synth();

expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, '.dockerignore'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'index.py'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'foobar.txt'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'subdirectory'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.sourceHash}`, 'subdirectory', 'baz.txt'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, '.dockerignore'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'Dockerfile'))).toBeDefined();
expect(fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'index.py'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'foobar.txt'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'subdirectory'))).toBeDefined();
expect(!fs.existsSync(path.join(session.directory, `asset.${image.assetHash}`, 'subdirectory', 'baz.txt'))).toBeDefined();


}
Expand Down
3 changes: 1 addition & 2 deletions packages/@aws-cdk/aws-events-targets/lib/event-bus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@ export class EventBus implements events.IRuleTarget {
this.role = props.role;
}

bind(rule: events.IRule, id?: string): events.RuleTargetConfig {
bind(rule: events.IRule, _id?: string): events.RuleTargetConfig {
if (this.role) {
this.role.addToPrincipalPolicy(this.putEventStatement());
}
const role = this.role ?? singletonEventRole(rule, [this.putEventStatement()]);
return {
id: id ?? '',
arn: this.eventBus.eventBusArn,
role,
};
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class Bundling implements cdk.BundlingOptions {
private static runsLocally?: boolean;

// Core bundling options
public readonly image: cdk.BundlingDockerImage;
public readonly image: cdk.DockerImage;
public readonly command: string[];
public readonly environment?: { [key: string]: string };
public readonly workingDirectory: string;
Expand Down Expand Up @@ -78,14 +78,14 @@ export class Bundling implements cdk.BundlingOptions {
// Docker bundling
const shouldBuildImage = props.forceDockerBundling || !Bundling.runsLocally;
this.image = shouldBuildImage
? props.dockerImage ?? cdk.BundlingDockerImage.fromAsset(path.join(__dirname, '../lib'), {
? props.dockerImage ?? cdk.DockerImage.fromBuild(path.join(__dirname, '../lib'), {
buildArgs: {
...props.buildArgs ?? {},
IMAGE: props.runtime.bundlingDockerImage.image,
ESBUILD_VERSION: props.esbuildVersion ?? ESBUILD_VERSION,
},
})
: cdk.BundlingDockerImage.fromRegistry('dummy'); // Do not build if we don't need to
: cdk.DockerImage.fromRegistry('dummy'); // Do not build if we don't need to

const bundlingCommand = this.createBundlingCommand(cdk.AssetStaging.BUNDLING_INPUT_DIR, cdk.AssetStaging.BUNDLING_OUTPUT_DIR);
this.command = ['bash', '-c', bundlingCommand];
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { BundlingDockerImage } from '@aws-cdk/core';
import { DockerImage } from '@aws-cdk/core';

/**
* Bundling options
Expand Down Expand Up @@ -200,7 +200,7 @@ export interface BundlingOptions {
*
* @default - use the Docker image provided by @aws-cdk/aws-lambda-nodejs
*/
readonly dockerImage?: BundlingDockerImage;
readonly dockerImage?: DockerImage;

/**
* Command hooks
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-lambda-nodejs/test/bundling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as child_process from 'child_process';
import * as os from 'os';
import * as path from 'path';
import { Code, Runtime } from '@aws-cdk/aws-lambda';
import { AssetHashType, BundlingDockerImage } from '@aws-cdk/core';
import { AssetHashType, DockerImage } from '@aws-cdk/core';
import { version as delayVersion } from 'delay/package.json';
import { Bundling } from '../lib/bundling';
import { LogLevel } from '../lib/types';
Expand All @@ -11,7 +11,7 @@ import * as util from '../lib/util';
jest.mock('@aws-cdk/aws-lambda');

// Mock BundlingDockerImage.fromAsset() to avoid building the image
let fromAssetMock = jest.spyOn(BundlingDockerImage, 'fromAsset');
let fromAssetMock = jest.spyOn(DockerImage, 'fromBuild');
let getEsBuildVersionMock = jest.spyOn(util, 'getEsBuildVersion');
beforeEach(() => {
jest.clearAllMocks();
Expand Down Expand Up @@ -290,7 +290,7 @@ test('Custom bundling docker image', () => {
entry,
depsLockFilePath,
runtime: Runtime.NODEJS_12_X,
dockerImage: BundlingDockerImage.fromRegistry('my-custom-image'),
dockerImage: DockerImage.fromRegistry('my-custom-image'),
forceDockerBundling: true,
});

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda-python/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export function bundle(options: BundlingOptions): lambda.Code {
// copy Dockerfile to workdir
fs.copyFileSync(path.join(__dirname, dockerfile), path.join(stagedir, dockerfile));

const image = cdk.BundlingDockerImage.fromAsset(stagedir, {
const image = cdk.DockerImage.fromBuild(stagedir, {
buildArgs: {
IMAGE: runtime.bundlingDockerImage.image,
},
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-lambda/lib/runtime.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { BundlingDockerImage } from '@aws-cdk/core';
import { BundlingDockerImage, DockerImage } from '@aws-cdk/core';

export interface LambdaRuntimeProps {
/**
Expand Down Expand Up @@ -218,7 +218,7 @@ export class Runtime {
this.supportsInlineCode = !!props.supportsInlineCode;
this.family = family;
const imageName = props.bundlingDockerImage ?? `amazon/aws-sam-cli-build-image-${name}`;
this.bundlingDockerImage = BundlingDockerImage.fromRegistry(imageName);
this.bundlingDockerImage = DockerImage.fromRegistry(imageName);
this.supportsCodeGuruProfiling = props.supportsCodeGuruProfiling ?? false;

Runtime.ALL.push(this);
Expand Down
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-s3-assets/lib/asset.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import * as path from 'path';
import * as assets from '@aws-cdk/assets';
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
Expand All @@ -8,11 +7,14 @@ import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { toSymlinkFollow } from './compat';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { CopyOptions } from '@aws-cdk/assets';
// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct as CoreConstruct } from '@aws-cdk/core';

export interface AssetOptions extends assets.CopyOptions, cdk.AssetOptions {
export interface AssetOptions extends CopyOptions, cdk.AssetOptions {
/**
* A list of principals that should be able to read this asset from S3.
* You can use `asset.grantRead(principal)` to grant read permissions later.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as path from 'path';
import * as iam from '@aws-cdk/aws-iam';
import { App, BundlingDockerImage, Stack, StackProps } from '@aws-cdk/core';
import { App, DockerImage, Stack, StackProps } from '@aws-cdk/core';
import { Construct } from 'constructs';
import * as assets from '../lib';

Expand All @@ -12,7 +12,7 @@ class TestStack extends Stack {
const asset = new assets.Asset(this, 'BundledAsset', {
path: path.join(__dirname, 'markdown-asset'), // /asset-input and working directory in the container
bundling: {
image: BundlingDockerImage.fromAsset(path.join(__dirname, 'alpine-markdown')), // Build an image
image: DockerImage.fromBuild(path.join(__dirname, 'alpine-markdown')), // Build an image
command: [
'sh', '-c', `
markdown index.md > /asset-output/index.html
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-s3/lib/bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1829,7 +1829,7 @@ export class Bucket extends BucketBase {
private enableAutoDeleteObjects() {
const provider = CustomResourceProvider.getOrCreateProvider(this, AUTO_DELETE_OBJECTS_RESOURCE_TYPE, {
codeDirectory: path.join(__dirname, 'auto-delete-objects-handler'),
runtime: CustomResourceProviderRuntime.NODEJS_12,
runtime: CustomResourceProviderRuntime.NODEJS_12_X,
description: `Lambda function for auto-deleting objects in ${this.bucketName} S3 bucket.`,
});

Expand Down
45 changes: 44 additions & 1 deletion packages/@aws-cdk/core/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class BundlingDockerImage {
* @param image the image name
*/
public static fromRegistry(image: string) {
return new BundlingDockerImage(image);
return new DockerImage(image);
}

/**
Expand Down Expand Up @@ -278,6 +278,49 @@ export class DockerImage extends BundlingDockerImage {
public static fromBuild(path: string, options: DockerBuildOptions = {}) {
return BundlingDockerImage.fromAsset(path, options);
}

/**
* Reference an image on DockerHub or another online registry.
*
* @param image the image name
*/
public static fromRegistry(image: string) {
return new DockerImage(image);
}

/** @param image The Docker image */
constructor(public readonly image: string, _imageHash?: string) {
super(image, _imageHash);
}

/**
* Provides a stable representation of this image for JSON serialization.
*
* @return The overridden image name if set or image hash name in that order
*/
public toJSON() {
return super.toJSON();
}

/**
* Runs a Docker image
*/
public run(options: DockerRunOptions = {}) {
return super.run(options);
}

/**
* Copies a file or directory out of the Docker image to the local filesystem.
*
* If `outputPath` is omitted the destination path is a temporary directory.
*
* @param imagePath the path in the Docker image
* @param outputPath the destination path for the copy operation
* @returns the destination path
*/
public cp(imagePath: string, outputPath?: string): string {
return super.cp(imagePath, outputPath);
}
}

/**
Expand Down
Loading

0 comments on commit ca391b5

Please sign in to comment.