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.

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

cdk-bundle #1954

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
218 changes: 218 additions & 0 deletions packages/@aws-cdk/assets/lib/artifact.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
import cdk = require('@aws-cdk/cdk');
import cxapi = require('@aws-cdk/cx-api');
import fs = require('fs');
import path = require('path');
import { AssetPackaging, GenericAssetProps } from './asset';
import { copyDirectory } from './fs-copy';
import { fingerprint } from './fs-fingerprint';

export interface AssetArtifactProps extends GenericAssetProps {
fingerprint: string;
}

/**
* This is an internal construct (only available within this module) which represents a unique
* asset within the app.
*
* To get an instance, use `AssetArtifact.forAsset`. It will calculate the fingerprint
* of the asset based on content hash and look up if there is already an artifact with this
* fingerprint in the app.
*/
export class AssetArtifact extends cdk.Construct implements cdk.ISynthesizable {

/**
* @returns gets or creates a singletone asset artifact for a specific asset
*/
public static forAsset(scope: cdk.Construct, props: GenericAssetProps): AssetArtifact {
const extra = {
packaging: props.packaging,
extra: props.extra,
};

const sourcePath = path.resolve(props.path);
validateAssetOnDisk(sourcePath, props.packaging);

// calculate content hash, which is what we use as the app-level ID of this asset
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 the app-level ID should also contain the packaging type at least.

const fp = fingerprint(props.path, {
exclude: props.exclude,
follow: props.follow,
extra: JSON.stringify(extra)
});

const root = scope.node.root;

const id = `asset_${fp}`;
const artifact = root.node.tryFindChild(id) as AssetArtifact || new AssetArtifact(root, id, {
fingerprint: fp,
path: sourcePath,
...props
Copy link
Contributor

Choose a reason for hiding this comment

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

Move ...props upfront... Otherwise it could replace fingerprint and path.

});

return artifact;
}

/**
* The unique fingerprint of this asset, based MD5 hash of the content and the
* directory structure.
*/
public readonly fingerprint: string;

private readonly artifactId: string;
private readonly environments = new Set<string>();

private constructor(scope: cdk.Construct, id: string, private readonly props: AssetArtifactProps) {
super(scope, id);

this.fingerprint = props.fingerprint;
this.artifactId = id; // based on the fingerprint
Copy link
Contributor

Choose a reason for hiding this comment

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

From this scope, you can't know it's based on the fingerprint. Why not make the ID from within the constructor?

}

/**
* Creates (or reuses) the set of bucket/key parameters that will be used to wire assets
* into a specific stack. Called by the `Asset` class to "subscribe" the asset to the stack.
*
* @param stack
*/
public wireToStack(stack: cdk.Stack): AssetArtifactParameters {
const bucketParamId = `asset-${this.fingerprint}-S3Bucket`;
const keyParamId = `asset-${this.fingerprint}-S3Key`;
const hashParamId = `asset-${this.fingerprint}-S3ContentHash`;

// if we already have a parameters wired for this asset, just return them (assuming they all exist)
if (stack.node.tryFindChild(keyParamId)) {
return {
bucket: stack.node.findChild(bucketParamId) as cdk.Parameter,
key: stack.node.findChild(keyParamId) as cdk.Parameter,
sha256: stack.node.findChild(hashParamId) as cdk.Parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, I'd prefer to findOrCreate all three parameters independently from each other. Not willing to die on that hill however.

};
}

// okay, "register" this asset with the stack. this includes adding a bunch of parameters
// and a metadata entry that tells the toolkit how to build the asset.

const bucketParam = new cdk.Parameter(stack, bucketParamId, {
type: 'String',
description: `S3 bucket for asset ${this.fingerprint} from ${this.props.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 don't think the path should be there. It'll be absolute and will change on every different machine that synthesizes it. At the very least if there, should ensure it is made relative to the working directory, I guess?

Also - the path could be only an "example" - if the same file is copied in multiple places, the fingerprint would still de-dupe them to the same fingerprint... And this might generate confusion.

});

const keyParam = new cdk.Parameter(stack, keyParamId, {
type: 'String',
description: `S3 key for asset ${this.fingerprint} from ${this.props.path}`
});

const contentHashParam = new cdk.Parameter(stack, hashParamId, {
type: 'String',
description: 'SHA-256 hash of the asset as it was uploaded to S3'
});

stack.setParameterValue(bucketParam, `\${${this.artifactId}}.s3bucket`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads weird here - probably because I don't know what setParameterValue really is for.

stack.setParameterValue(keyParam, `\${${this.artifactId}}.s3key`);
stack.setParameterValue(contentHashParam, `\${${this.artifactId}}.sha256`);

this.environments.add(stack.environment);

return {
bucket: bucketParam,
key: keyParam,
sha256: contentHashParam
};
}

public synthesize(session: cdk.ISynthesisSession) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this blank line is useful. The same applies to the one at the function's end...

switch (this.props.packaging) {
case AssetPackaging.File:
this.synthesizeFile(session);
return;

case AssetPackaging.ZipDirectory:
this.synthesizeZipDirectory(session);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a default case.

You could make this more compact by doing return this.synthesize<whatevs>(session); instead of having the return on a separate line each time.


}

private synthesizeFile(session: cdk.ISynthesisSession) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, you'd want those to be pluggable. So I'd tend to prefer modeling them separately to prepare for that.

const ext = path.extname(this.props.path);
const fileName = `${this.fingerprint}${ext}`;

const stagingFile = path.join(this.fingerprint, path.basename(this.props.path));

fs.mkdirSync(path.join(session.staging.path, this.fingerprint));
fs.copyFileSync(this.props.path, path.join(session.staging.path, stagingFile));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer link if possible (no biggie). Assets can be big and you'll be wearing my SSD off pretty quickly 😅


session.addBuildStep(this.artifactId, {
type: 'CopyFileTask',
parameters: {
src: `staging/${stagingFile}`,
dest: `assembly/${fileName}` // relative to outdir
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use for copying a file to a target outside of the assembly? If there's none, why isn't the dest assembly-relative instead?

}
});

for (const env of this.environments) {
session.addArtifact(this.artifactId, {
type: cxapi.ArtifactType.AwsS3Object,
environment: env,
properties: {
file: fileName // relative to assembly
}
});
}
}

private synthesizeZipDirectory(session: cdk.ISynthesisSession) {
const fileName = `${this.fingerprint}.zip`;

// copy source contents to a staging directory
const stagingDir = this.fingerprint;
fs.mkdirSync(path.join(session.staging.path, stagingDir));
copyDirectory(this.props.path, path.join(session.staging.path, stagingDir));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before - would prefer link if possible.


session.addBuildStep(this.artifactId, {
type: 'ZipDirectoryTask',
parameters: {
src: `staging/${stagingDir}`,
dest: `assembly/${fileName}`, // relative to outdir
}
});

for (const env of this.environments) {
session.addArtifact(this.artifactId, {
type: cxapi.ArtifactType.AwsS3Object,
environment: env,
properties: {
file: fileName // relative to assembly/
}
});
}
}
}

export interface AssetArtifactParameters {
bucket: cdk.Parameter;
key: cdk.Parameter;
sha256: cdk.Parameter;
}

function validateAssetOnDisk(assetPath: string, packaging: AssetPackaging) {
if (!fs.existsSync(assetPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So you're going to use fs.stat, and fs.exists actually makes a fs.stat call. You could instead just make one fs.stat call and store the result in a variable, then re-use it.

Also - as you're using fs.stat and not fs.lstat, you'll be getting the information about the referent of any link being stat'd. I think you should instead use fs.lstat and make link traversal strictly opt-in.

throw new Error(`Cannot find asset at ${assetPath}`);
}

switch (packaging) {
case AssetPackaging.ZipDirectory:
if (!fs.statSync(assetPath).isDirectory()) {
throw new Error(`${assetPath} is expected to be a directory when asset packaging is 'zip'`);
}
break;

case AssetPackaging.File:
if (!fs.statSync(assetPath).isFile()) {
throw new Error(`${assetPath} is expected to be a regular file when asset packaging is 'file'`);
}
break;

default:
throw new Error(`Unsupported asset packaging format: ${packaging}`);
}
}
Loading