From a735b52e4a33803b9ce1911bc0e2cc7b78ef581a Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Fri, 19 Feb 2021 10:17:07 +0200 Subject: [PATCH] fix(core): ENOTDIR invalid cwd on "cdk deploy" (#13145) This commit reverts two recent changes to the asset system (#12258 and ##13076) which introduced a regression in 1.90.0. Fixes #13131 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- allowed-breaking-changes.txt | 7 ++ packages/@aws-cdk/aws-s3-assets/README.md | 21 ---- packages/@aws-cdk/aws-s3-assets/lib/asset.ts | 30 ++++- packages/@aws-cdk/core/lib/asset-staging.ts | 90 +------------ packages/@aws-cdk/core/lib/bundling.ts | 35 ------ .../@aws-cdk/core/test/archive/archive.zip | 0 packages/@aws-cdk/core/test/docker-stub.sh | 15 +-- packages/@aws-cdk/core/test/staging.test.ts | 118 +----------------- 8 files changed, 42 insertions(+), 274 deletions(-) delete mode 100644 packages/@aws-cdk/core/test/archive/archive.zip diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index 2ca2ca5b6067f..964bb4d5c7712 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -56,3 +56,10 @@ incompatible-argument:@aws-cdk/aws-ecs.TaskDefinition.addVolume # We made properties optional and it's really fine but our differ doesn't think so. weakened:@aws-cdk/cloud-assembly-schema.DockerImageSource weakened:@aws-cdk/cloud-assembly-schema.FileSource + +# https://github.com/aws/aws-cdk/pull/13145 +removed:@aws-cdk/core.AssetStaging.isArchive +removed:@aws-cdk/core.AssetStaging.packaging +removed:@aws-cdk/core.BundlingOutput +removed:@aws-cdk/core.BundlingOptions.outputType + diff --git a/packages/@aws-cdk/aws-s3-assets/README.md b/packages/@aws-cdk/aws-s3-assets/README.md index f2583b7c10a24..aab4c46d9c44d 100644 --- a/packages/@aws-cdk/aws-s3-assets/README.md +++ b/packages/@aws-cdk/aws-s3-assets/README.md @@ -124,27 +124,6 @@ new assets.Asset(this, 'BundledAsset', { Although optional, it's recommended to provide a local bundling method which can greatly improve performance. -If the bundling output contains a single archive file (zip or jar) it will be -uploaded to S3 as-is and will not be zipped. Otherwise the contents of the -output directory will be zipped and the zip file will be uploaded to S3. This -is the default behavior for `bundling.outputType` (`BundlingOutput.AUTO_DISCOVER`). - -Use `BundlingOutput.NOT_ARCHIVED` if the bundling output must always be zipped: - -```ts -const asset = new assets.Asset(this, 'BundledAsset', { - path: '/path/to/asset', - bundling: { - image: BundlingDockerImage.fromRegistry('alpine'), - command: ['command-that-produces-an-archive.sh'], - outputType: BundlingOutput.NOT_ARCHIVED, // Bundling output will be zipped even though it produces a single archive file. - }, -}); -``` - -Use `BundlingOutput.ARCHIVED` if the bundling output contains a single archive file and -you don't want it to be zippped. - ## CloudFormation Resource Metadata > NOTE: This section is relevant for authors of AWS Resource Constructs. diff --git a/packages/@aws-cdk/aws-s3-assets/lib/asset.ts b/packages/@aws-cdk/aws-s3-assets/lib/asset.ts index 510834a61c634..938778d1381f4 100644 --- a/packages/@aws-cdk/aws-s3-assets/lib/asset.ts +++ b/packages/@aws-cdk/aws-s3-assets/lib/asset.ts @@ -1,3 +1,4 @@ +import * as fs from 'fs'; import * as path from 'path'; import * as assets from '@aws-cdk/assets'; import * as iam from '@aws-cdk/aws-iam'; @@ -12,6 +13,8 @@ import { toSymlinkFollow } from './compat'; // eslint-disable-next-line no-duplicate-imports, import/order import { Construct as CoreConstruct } from '@aws-cdk/core'; +const ARCHIVE_EXTENSIONS = ['.zip', '.jar']; + export interface AssetOptions extends assets.CopyOptions, cdk.AssetOptions { /** * A list of principals that should be able to read this asset from S3. @@ -136,12 +139,17 @@ export class Asset extends CoreConstruct implements cdk.IAsset { this.assetPath = staging.relativeStagedPath(stack); - this.isFile = staging.packaging === cdk.FileAssetPackaging.FILE; + const packaging = determinePackaging(staging.sourcePath); + + this.isFile = packaging === cdk.FileAssetPackaging.FILE; - this.isZipArchive = staging.isArchive; + // sets isZipArchive based on the type of packaging and file extension + this.isZipArchive = packaging === cdk.FileAssetPackaging.ZIP_DIRECTORY + ? true + : ARCHIVE_EXTENSIONS.some(ext => staging.sourcePath.toLowerCase().endsWith(ext)); const location = stack.synthesizer.addFileAsset({ - packaging: staging.packaging, + packaging, sourceHash: this.sourceHash, fileName: this.assetPath, }); @@ -202,3 +210,19 @@ export class Asset extends CoreConstruct implements cdk.IAsset { this.bucket.grantRead(grantee); } } + +function determinePackaging(assetPath: string): cdk.FileAssetPackaging { + if (!fs.existsSync(assetPath)) { + throw new Error(`Cannot find asset at ${assetPath}`); + } + + if (fs.statSync(assetPath).isDirectory()) { + return cdk.FileAssetPackaging.ZIP_DIRECTORY; + } + + if (fs.statSync(assetPath).isFile()) { + return cdk.FileAssetPackaging.FILE; + } + + throw new Error(`Asset ${assetPath} is expected to be either a directory or a regular file`); +} diff --git a/packages/@aws-cdk/core/lib/asset-staging.ts b/packages/@aws-cdk/core/lib/asset-staging.ts index 17fd89781004a..66c65e3d14864 100644 --- a/packages/@aws-cdk/core/lib/asset-staging.ts +++ b/packages/@aws-cdk/core/lib/asset-staging.ts @@ -5,8 +5,8 @@ import * as cxapi from '@aws-cdk/cx-api'; import { Construct } from 'constructs'; import * as fs from 'fs-extra'; import * as minimatch from 'minimatch'; -import { AssetHashType, AssetOptions, FileAssetPackaging } from './assets'; -import { BundlingOptions, BundlingOutput } from './bundling'; +import { AssetHashType, AssetOptions } from './assets'; +import { BundlingOptions } from './bundling'; import { FileSystem, FingerprintOptions } from './fs'; import { Names } from './names'; import { Cache } from './private/cache'; @@ -17,8 +17,6 @@ import { Stage } from './stage'; // eslint-disable-next-line import { Construct as CoreConstruct } from './construct-compat'; -const ARCHIVE_EXTENSIONS = ['.zip', '.jar']; - /** * A previously staged asset */ @@ -140,9 +138,6 @@ export class AssetStaging extends CoreConstruct { private readonly cacheKey: string; - private _packaging = FileAssetPackaging.ZIP_DIRECTORY; - private _isArchive = true; - constructor(scope: Construct, id: string, props: AssetStagingProps) { super(scope, id); @@ -208,20 +203,6 @@ export class AssetStaging extends CoreConstruct { return this.assetHash; } - /** - * How this asset should be packaged. - */ - public get packaging(): FileAssetPackaging { - return this._packaging; - } - - /** - * Whether this asset is an archive (zip or jar). - */ - public get isArchive(): boolean { - return this._isArchive; - } - /** * Return the path to the staged asset, relative to the Cloud Assembly (manifest) directory of the given stack * @@ -300,16 +281,11 @@ export class AssetStaging extends CoreConstruct { const bundleDir = this.determineBundleDir(this.assetOutdir, assetHash); this.bundle(bundling, bundleDir); - // Check bundling output content and determine if we will need to archive - const bundlingOutputType = bundling.outputType ?? BundlingOutput.AUTO_DISCOVER; - const bundledAsset = determineBundledAsset(bundleDir, bundlingOutputType); - this._packaging = bundledAsset.packaging; - // Calculate assetHash afterwards if we still must - assetHash = assetHash ?? this.calculateHash(this.hashType, bundling, bundledAsset.path); - const stagedPath = path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension)); + assetHash = assetHash ?? this.calculateHash(this.hashType, bundling, bundleDir); + const stagedPath = path.resolve(this.assetOutdir, renderAssetFilename(assetHash)); - this.stageAsset(bundledAsset.path, stagedPath, 'move'); + this.stageAsset(bundleDir, stagedPath, 'move'); return { assetHash, stagedPath }; } @@ -347,8 +323,6 @@ export class AssetStaging extends CoreConstruct { const stat = fs.statSync(sourcePath); if (stat.isFile()) { fs.copyFileSync(sourcePath, targetPath); - this._packaging = FileAssetPackaging.FILE; - this._isArchive = ARCHIVE_EXTENSIONS.includes(path.extname(sourcePath).toLowerCase()); } else if (stat.isDirectory()) { fs.mkdirSync(targetPath); FileSystem.copyDirectory(sourcePath, targetPath, this.fingerprintOptions); @@ -528,57 +502,3 @@ function sortObject(object: { [key: string]: any }): { [key: string]: any } { } return ret; } - -/** - * Returns the single archive file of a directory or undefined - */ -function singleArchiveFile(directory: string): string | undefined { - if (!fs.existsSync(directory)) { - throw new Error(`Directory ${directory} does not exist.`); - } - - if (!fs.statSync(directory).isDirectory()) { - throw new Error(`${directory} is not a directory.`); - } - - const content = fs.readdirSync(directory); - if (content.length === 1) { - const file = path.join(directory, content[0]); - const extension = path.extname(content[0]).toLowerCase(); - if (fs.statSync(file).isFile() && ARCHIVE_EXTENSIONS.includes(extension)) { - return file; - } - } - - return undefined; -} - -interface BundledAsset { - path: string, - packaging: FileAssetPackaging, - extension?: string -} - -/** - * Returns the bundled asset to use based on the content of the bundle directory - * and the type of output. - */ -function determineBundledAsset(bundleDir: string, outputType: BundlingOutput): BundledAsset { - const archiveFile = singleArchiveFile(bundleDir); - - // auto-discover means that if there is an archive file, we take it as the - // bundle, otherwise, we will archive here. - if (outputType === BundlingOutput.AUTO_DISCOVER) { - outputType = archiveFile ? BundlingOutput.ARCHIVED : BundlingOutput.NOT_ARCHIVED; - } - - switch (outputType) { - case BundlingOutput.NOT_ARCHIVED: - return { path: bundleDir, packaging: FileAssetPackaging.ZIP_DIRECTORY }; - case BundlingOutput.ARCHIVED: - if (!archiveFile) { - throw new Error('Bundling output directory is expected to include only a single .zip or .jar file when `output` is set to `ARCHIVED`'); - } - return { path: archiveFile, packaging: FileAssetPackaging.FILE, extension: path.extname(archiveFile) }; - } -} diff --git a/packages/@aws-cdk/core/lib/bundling.ts b/packages/@aws-cdk/core/lib/bundling.ts index 57885ceeca05c..b1247fd913ea0 100644 --- a/packages/@aws-cdk/core/lib/bundling.ts +++ b/packages/@aws-cdk/core/lib/bundling.ts @@ -79,41 +79,6 @@ export interface BundlingOptions { * @experimental */ readonly local?: ILocalBundling; - - /** - * The type of output that this bundling operation is producing. - * - * @default BundlingOutput.AUTO_DISCOVER - * - * @experimental - */ - readonly outputType?: BundlingOutput; -} - -/** - * The type of output that a bundling operation is producing. - * - * @experimental - */ -export enum BundlingOutput { - /** - * The bundling output directory includes a single .zip or .jar file which - * will be used as the final bundle. If the output directory does not - * include exactly a single archive, bundling will fail. - */ - ARCHIVED = 'archived', - - /** - * The bundling output directory contains one or more files which will be - * archived and uploaded as a .zip file to S3. - */ - NOT_ARCHIVED = 'not-archived', - - /** - * If the bundling output directory contains a single archive file (zip or jar) - * it will not be zipped. Otherwise the bundling output will be zipped. - */ - AUTO_DISCOVER = 'auto-discover', } /** diff --git a/packages/@aws-cdk/core/test/archive/archive.zip b/packages/@aws-cdk/core/test/archive/archive.zip deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/packages/@aws-cdk/core/test/docker-stub.sh b/packages/@aws-cdk/core/test/docker-stub.sh index 94f806f69a120..fe48e93d4a207 100755 --- a/packages/@aws-cdk/core/test/docker-stub.sh +++ b/packages/@aws-cdk/core/test/docker-stub.sh @@ -24,18 +24,5 @@ if echo "$@" | grep "DOCKER_STUB_SUCCESS"; then exit 0 fi -if echo "$@" | grep "DOCKER_STUB_MULTIPLE_FILES"; then - outdir=$(echo "$@" | xargs -n1 | grep "/asset-output" | head -n1 | cut -d":" -f1) - touch ${outdir}/test1.txt - touch ${outdir}/test2.txt - exit 0 -fi - -if echo "$@" | grep "DOCKER_STUB_SINGLE_ARCHIVE"; then - outdir=$(echo "$@" | xargs -n1 | grep "/asset-output" | head -n1 | cut -d":" -f1) - touch ${outdir}/test.zip - exit 0 -fi - -echo "Docker mock only supports one of the following commands: DOCKER_STUB_SUCCESS_NO_OUTPUT,DOCKER_STUB_FAIL,DOCKER_STUB_SUCCESS,DOCKER_STUB_MULTIPLE_FILES,DOCKER_SINGLE_ARCHIVE" +echo "Docker mock only supports one of the following commands: DOCKER_STUB_SUCCESS_NO_OUTPUT,DOCKER_STUB_FAIL,DOCKER_STUB_SUCCESS" exit 1 diff --git a/packages/@aws-cdk/core/test/staging.test.ts b/packages/@aws-cdk/core/test/staging.test.ts index 76bf74f9cffd7..347c5fcea3b63 100644 --- a/packages/@aws-cdk/core/test/staging.test.ts +++ b/packages/@aws-cdk/core/test/staging.test.ts @@ -1,11 +1,10 @@ import * as os from 'os'; import * as path from 'path'; -import { FileAssetPackaging } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; import * as fs from 'fs-extra'; import { nodeunitShim, Test } from 'nodeunit-shim'; import * as sinon from 'sinon'; -import { App, AssetHashType, AssetStaging, BundlingDockerImage, BundlingOptions, BundlingOutput, FileSystem, Stack, Stage } from '../lib'; +import { App, AssetHashType, AssetStaging, BundlingDockerImage, BundlingOptions, FileSystem, Stack, Stage } from '../lib'; const STUB_INPUT_FILE = '/tmp/docker-stub.input'; const STUB_INPUT_CONCAT_FILE = '/tmp/docker-stub.input.concat'; @@ -13,9 +12,7 @@ const STUB_INPUT_CONCAT_FILE = '/tmp/docker-stub.input.concat'; enum DockerStubCommand { SUCCESS = 'DOCKER_STUB_SUCCESS', FAIL = 'DOCKER_STUB_FAIL', - SUCCESS_NO_OUTPUT = 'DOCKER_STUB_SUCCESS_NO_OUTPUT', - MULTIPLE_FILES = 'DOCKER_STUB_MULTIPLE_FILES', - SINGLE_ARCHIVE = 'DOCKER_STUB_SINGLE_ARCHIVE', + SUCCESS_NO_OUTPUT = 'DOCKER_STUB_SUCCESS_NO_OUTPUT' } const FIXTURE_TEST1_DIR = path.join(__dirname, 'fs', 'fixtures', 'test1'); @@ -53,34 +50,6 @@ nodeunitShim({ test.deepEqual(staging.sourcePath, sourcePath); test.deepEqual(path.basename(staging.stagedPath), 'asset.2f37f937c51e2c191af66acf9b09f548926008ec68c575bd2ee54b6e997c0e00'); test.deepEqual(path.basename(staging.relativeStagedPath(stack)), 'asset.2f37f937c51e2c191af66acf9b09f548926008ec68c575bd2ee54b6e997c0e00'); - test.deepEqual(staging.packaging, FileAssetPackaging.ZIP_DIRECTORY); - test.deepEqual(staging.isArchive, true); - test.done(); - }, - - 'staging of an archive file correctly sets packaging and isArchive'(test: Test) { - // GIVEN - const stack = new Stack(); - const sourcePath = path.join(__dirname, 'archive', 'archive.zip'); - - // WHEN - const staging = new AssetStaging(stack, 's1', { sourcePath }); - - test.deepEqual(staging.packaging, FileAssetPackaging.FILE); - test.deepEqual(staging.isArchive, true); - test.done(); - }, - - 'staging of a non-archive file correctly sets packaging and isArchive'(test: Test) { - // GIVEN - const stack = new Stack(); - const sourcePath = __filename; - - // WHEN - const staging = new AssetStaging(stack, 's1', { sourcePath }); - - test.deepEqual(staging.packaging, FileAssetPackaging.FILE); - test.deepEqual(staging.isArchive, false); test.done(); }, @@ -816,89 +785,6 @@ nodeunitShim({ ); test.equal(asset.assetHash, '33cbf2cae5432438e0f046bc45ba8c3cef7b6afcf47b59d1c183775c1918fb1f'); // hash of MyStack/Asset - test.done(); - }, - - 'bundling that produces a single archive file is autodiscovered'(test: Test) { - // GIVEN - const app = new App(); - const stack = new Stack(app, 'stack'); - const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); - - // WHEN - const staging = new AssetStaging(stack, 'Asset', { - sourcePath: directory, - bundling: { - image: BundlingDockerImage.fromRegistry('alpine'), - command: [DockerStubCommand.SINGLE_ARCHIVE], - }, - }); - - // THEN - const assembly = app.synth(); - test.deepEqual(fs.readdirSync(assembly.directory), [ - 'asset.f43148c61174f444925231b5849b468f21e93b5d1469cd07c53625ffd039ef48', // this is the bundle dir but it's empty - 'asset.f43148c61174f444925231b5849b468f21e93b5d1469cd07c53625ffd039ef48.zip', - 'cdk.out', - 'manifest.json', - 'stack.template.json', - 'tree.json', - ]); - test.equal(fs.readdirSync(path.join(assembly.directory, 'asset.f43148c61174f444925231b5849b468f21e93b5d1469cd07c53625ffd039ef48')).length, 0); // empty bundle dir - test.deepEqual(staging.packaging, FileAssetPackaging.FILE); - test.deepEqual(staging.isArchive, true); - - test.done(); - }, - - 'bundling that produces a single archive file with NOT_ARCHIVED'(test: Test) { - // GIVEN - const app = new App(); - const stack = new Stack(app, 'stack'); - const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); - - // WHEN - const staging = new AssetStaging(stack, 'Asset', { - sourcePath: directory, - bundling: { - image: BundlingDockerImage.fromRegistry('alpine'), - command: [DockerStubCommand.SINGLE_ARCHIVE], - outputType: BundlingOutput.NOT_ARCHIVED, - }, - }); - - // THEN - const assembly = app.synth(); - test.deepEqual(fs.readdirSync(assembly.directory), [ - 'asset.86ec07746e1d859290cfd8b9c648e581555649c75f51f741f11e22cab6775abc', - 'cdk.out', - 'manifest.json', - 'stack.template.json', - 'tree.json', - ]); - test.deepEqual(staging.packaging, FileAssetPackaging.ZIP_DIRECTORY); - test.deepEqual(staging.isArchive, true); - - test.done(); - }, - - 'throws with ARCHIVED and bundling that does not produce a single archive file'(test: Test) { - // GIVEN - const app = new App(); - const stack = new Stack(app, 'stack'); - const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); - - // WHEN - test.throws(() => new AssetStaging(stack, 'Asset', { - sourcePath: directory, - bundling: { - image: BundlingDockerImage.fromRegistry('alpine'), - command: [DockerStubCommand.MULTIPLE_FILES], - outputType: BundlingOutput.ARCHIVED, - }, - }), /Bundling output directory is expected to include only a single .zip or .jar file when `output` is set to `ARCHIVED`/); - - test.done(); }, });