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

feat(core): allow multiple Apps #1582

Closed
wants to merge 11 commits into from
Closed
24 changes: 23 additions & 1 deletion packages/@aws-cdk/assert/lib/inspector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,33 @@ export class StackPathInspector extends Inspector {
}

public get value(): { [key: string]: any } | undefined {
const md = this.stack.metadata[`/${this.stack.name}${this.path}`];
const md = this.getPathMetadata();
if (md === undefined) { return undefined; }
const resourceMd = md.find(entry => entry.type === 'aws:cdk:logicalId');
if (resourceMd === undefined) { return undefined; }
const logicalId = resourceMd.data;
return this.stack.template.Resources[logicalId];
}

/**
* Return the metadata entry for the given stack and path
*
* Complicated slightly by the fact that the stack may or may not
* have an "App" parent, whose id will appear in the metadata key
* but there's no way to get the app's id otherwise.
*/
private getPathMetadata() {
for (const [mdKey, mdValue] of Object.entries(this.stack.metadata)) {
// Looks like either:
// "/App/Stack/Resource/Path"
// "/Stack/Resource/Path"
const parts = mdKey.split('/');

if ((parts[1] === this.stack.name && ('/' + parts.slice(2).join('/')) === this.path)
|| (parts[2] === this.stack.name && ('/' + parts.slice(3).join('/')) === this.path)) {
return mdValue;
}
}
return undefined;
}
}
253 changes: 130 additions & 123 deletions packages/@aws-cdk/cdk/lib/app.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,96 @@
import cxapi = require('@aws-cdk/cx-api');
import fs = require('fs');
import path = require('path');
import { Stack } from './cloudformation/stack';
import { IConstruct, MetadataEntry, PATH_SEP, Root } from './core/construct';
import { Construct } from './core/construct';
import { IConstruct, MetadataEntry, PATH_SEP } from './core/construct';
import { Environment } from './environment';
import { Program } from './program';
import { validateAndThrow } from './util/validation';

/**
* Represents a CDK program.
* Properties for an App
*/
export class App extends Root {
export interface AppProps {
/**
* The AWS environment (account/region) where stacks in this app will be deployed.
*
* If not supplied, the `default-account` and `default-region` context parameters will be
* used. If they are undefined, it will not be possible to deploy the stack.
*
* @default Automatically determined
*/
env?: Environment;
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're breaking stuff, can we rename to environment? This is infrequently used and the abbreviation doesn't add that much...


/**
* The CDK Program instance in which this app will be defined.
*
* You don't have to pass this, it exists for testing purposes. Only
* supply this parameter if you know what you are doing.
*
* @default Automatically created
*/
program?: Program;
}

/**
* Used as construct name if no ID is supplied for the App
*/
const DEFAULT_APP_NAME = 'App';

/**
* An instance of your App
*/
export class App extends Construct {
/**
* True if the given construct is a default-named App
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get consistent about having:

  • @param for each parameter
  • @returns for the return value

Missing those, documentation will be very poor, especially for other languages such as Java.

*/
public static isDefaultApp(construct: IConstruct) {
return App.isApp(construct) && construct.defaultAppName;
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't return true(per the documentation), but it'll return truthy. I'd coerce to a boolean context.

}

/**
* True if the given construct is an App object
*/
public static isApp(construct: IConstruct): construct is App {
return (construct as any).defaultAppName !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to use Symbols, would that mess with JSII? It would be a safer way to make this determination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure.

Also, what would the advantage of a symbol be over any instance of an object? Effectively,

const sym1 = Symbol();
const sym2 = {};

sym1 and sym2 would behave the same when compared for equality, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Symbols are unique, therefore there is no way to accidentally add add a 'defaultAppName' property to your construct. They can also be used to compensate for instanceOf shortcomings:

const isApp = Symbol.for('aws-cdk:isApp'); // creates or gets a universal named symbol from the registry

class App {
  [isApp]: true; // will not collide with an ordinary property like 'isApp'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, as keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will not across JSII, for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need to? We only need this deep in the internals to tag App, Stack and Construct.. we can just generate an ordinary property in the JSII facades. It eliminates instanceOf and name-collisions when checking properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for symbols

}

/**
* The environment in which this stack is deployed.
*/
public readonly env: Environment;

/**
* Whether the App id was supplied
*/
private defaultAppName: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation comment confuses.


/**
* Whether validate() was already run or not
*/
private validated: boolean;

/**
* Whether prepare() was already run or not
*/
private prepared: boolean;

/**
* Initializes a CDK application.
*
* @param request Optional toolkit request (e.g. for tests)
*/
constructor() {
super();
this.loadContext();
constructor(id?: string, props: AppProps = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are breaking the construct signature anyway here, why not make ID and optional prop instead of an optional argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cf:

const app = new App('MyApp');

const app = new App({ id: 'MyApp' });

Even though we don't have the full construct signature, at least in this it's somewhat obvious that the ID passed is a regular construct ID, which will contribute to a construct path (a stack would have path /MyApp/MyStack). I don't find that as obvious in the second case. Plus, the second one reads more noisily. Just generally don't care for it much.

// For tests, we use a fresh Program every time
const program = props.program || (process.env.CDK_TEST_MODE === '1'
? new Program()
: Program.defaultInstance());

super(program, id || DEFAULT_APP_NAME);

this.env = props.env || {};
this.defaultAppName = id === undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not !id?

this.validated = false;
this.prepared = false;
}

private get stacks() {
Expand All @@ -30,42 +106,27 @@ export class App extends Root {
}

/**
* Runs the program. Output is written to output directory as specified in the request.
* @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

@deprecated should mention why & what users should do instead (even if that is "nothing").

*/
public run(): void {
const outdir = process.env[cxapi.OUTDIR_ENV];
if (!outdir) {
process.stderr.write(`ERROR: The environment variable "${cxapi.OUTDIR_ENV}" is not defined\n`);
process.stderr.write('AWS CDK Toolkit (>= 0.11.0) is required in order to interact with this program.\n');
process.exit(1);
return;
}

const result: cxapi.SynthesizeResponse = {
version: cxapi.PROTO_RESPONSE_VERSION,
stacks: this.synthesizeStacks(Object.keys(this.stacks)),
runtime: this.collectRuntimeInformation()
};

const outfile = path.join(outdir, cxapi.OUTFILE_NAME);
fs.writeFileSync(outfile, JSON.stringify(result, undefined, 2));
public run() {
this.node.addWarning(`It's not necessary to call app.run() anymore`);
}

/**
* Synthesize and validate a single stack
* @param stackName The name of the stack to synthesize
*/
public synthesizeStack(stackName: string): cxapi.SynthesizedStack {
const stack = this.getStack(stackName);

this.node.prepareTree();

// first, validate this stack and stop if there are errors.
const errors = stack.node.validateTree();
if (errors.length > 0) {
const errorList = errors.map(e => `[${e.source.node.path}] ${e.message}`).join('\n ');
throw new Error(`Stack validation failed with the following errors:\n ${errorList}`);
// Maintain the existing contract that `synthesizeStack` can be called
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still flag as @deprecated, but maybe that requires we devise an other mechanism for customers to get the "prepare + validate" behavior. I feel like it's wrong for user land to call synthesizeStack directly.

// by consumers, and that it will prepare and validate the construct tree.
if (!this.prepared) {
this.node.prepareTree();
}
if (!this.validated) {
validateAndThrow(this);
}

const stack = this.getStack(stackName);

const account = stack.env.account || 'unknown-account';
const region = stack.env.region || 'unknown-region';
Expand All @@ -78,12 +139,12 @@ export class App extends Root {

const missing = Object.keys(stack.missingContext).length ? stack.missingContext : undefined;
return {
name: stack.node.id,
name: stack.name,
environment,
missing,
template: stack.toCloudFormation(),
metadata: this.collectMetadata(stack),
dependsOn: noEmptyArray(stack.dependencies().map(s => s.node.id)),
dependsOn: noEmptyArray(stack.dependencies().map(s => s.name)),
};
}

Expand All @@ -106,16 +167,16 @@ export class App extends Root {

visit(stack);

// add app-level metadata under "."
// add app-level metadata as well
if (this.node.metadata.length > 0) {
output[PATH_SEP] = this.node.metadata;
output[PATH_SEP + this.node.path] = this.node.metadata;
}

return output;

function visit(node: IConstruct) {
if (node.node.metadata.length > 0) {
// Make the path absolute
// Make the path absolute.
output[PATH_SEP + node.node.path] = node.node.metadata.map(md => node.node.resolve(md) as MetadataEntry);
}

Expand All @@ -125,27 +186,25 @@ export class App extends Root {
}
}

private collectRuntimeInformation(): cxapi.AppRuntime {
const libraries: { [name: string]: string } = {};
protected prepare() {
this.prepared = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not call this.node.prepareTree()?

}

for (const fileName of Object.keys(require.cache)) {
const pkg = findNpmPackage(fileName);
if (pkg && !pkg.private) {
libraries[pkg.name] = pkg.version;
}
protected validate(): string[] {
this.validated = true;
if (numberOfAppsInProgram(this) > 1 && this.node.id === DEFAULT_APP_NAME) {
Copy link
Contributor

Choose a reason for hiding this comment

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

But what if the id I passed is App (which happens to be the default)? Shouldn't this refer to the defaultAppName property?

return ['When constructing more than one App, all of them must have ids'];
}
return [];
}

// include only libraries that are in the @aws-cdk npm scope
for (const name of Object.keys(libraries)) {
if (!name.startsWith('@aws-cdk/')) {
delete libraries[name];
}
}

// add jsii runtime version
libraries['jsii-runtime'] = getJsiiAgentVersion();

return { libraries };
/**
* Synthesize the App
*/
protected synthesize(): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should skip the : any here. I reckon the inferred type will still be better info than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not good enough. jsii needs an unchanging type and it can't be an anonymous one.

return {
stacks: this.synthesizeStacks(Object.keys(this.stacks)),
};
}

private getStack(stackname: string) {
Expand All @@ -159,77 +218,25 @@ export class App extends Root {
}
return stack;
}
}

private loadContext() {
const contextJson = process.env[cxapi.CONTEXT_ENV];
const context = !contextJson ? { } : JSON.parse(contextJson);
for (const key of Object.keys(context)) {
this.node.setContext(key, context[key]);
}
}
function noEmptyArray<T>(xs: T[]): T[] | undefined {
return xs.length > 0 ? xs : undefined;
}

/**
* Determines which NPM module a given loaded javascript file is from.
*
* The only infromation that is available locally is a list of Javascript files,
* and every source file is associated with a search path to resolve the further
* ``require`` calls made from there, which includes its own directory on disk,
* and parent directories - for example:
*
* [ '...repo/packages/aws-cdk-resources/lib/cfn/node_modules',
* '...repo/packages/aws-cdk-resources/lib/node_modules',
* '...repo/packages/aws-cdk-resources/node_modules',
* '...repo/packages/node_modules',
* // etc...
* ]
*
* We are looking for ``package.json`` that is anywhere in the tree, except it's
* in the parent directory, not in the ``node_modules`` directory. For this
* reason, we strip the ``/node_modules`` suffix off each path and use regular
* module resolution to obtain a reference to ``package.json``.
*
* @param fileName a javascript file name.
* @returns the NPM module infos (aka ``package.json`` contents), or
* ``undefined`` if the lookup was unsuccessful.
* Cound the Apps in the construct tree
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Cound/Count/

*/
function findNpmPackage(fileName: string): { name: string, version: string, private?: boolean } | undefined {
const mod = require.cache[fileName];
const paths = mod.paths.map(stripNodeModules);

try {
const packagePath = require.resolve('package.json', { paths });
return require(packagePath);
} catch (e) {
return undefined;
}

/**
* @param s a path.
* @returns ``s`` with any terminating ``/node_modules``
* (or ``\\node_modules``) stripped off.)
*/
function stripNodeModules(s: string): string {
if (s.endsWith('/node_modules') || s.endsWith('\\node_modules')) {
// /node_modules is 13 characters
return s.substr(0, s.length - 13);
}
return s;
}
function numberOfAppsInProgram(app: IConstruct): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this be an attribute of the Program class?

return findRoot(app).node.findAll().filter(App.isApp).length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don’t we have node.root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't but it's easy enough to make.

}

function getJsiiAgentVersion() {
let jsiiAgent = process.env.JSII_AGENT;

// if JSII_AGENT is not specified, we will assume this is a node.js runtime
// and plug in our node.js version
if (!jsiiAgent) {
jsiiAgent = `node.js/${process.version}`;
/**
* Return the root of the construct tree
*/
function findRoot(current: IConstruct): IConstruct {
while (current.node.scope !== undefined) {
current = current.node.scope;
}

return jsiiAgent;
}

function noEmptyArray<T>(xs: T[]): T[] | undefined {
return xs.length > 0 ? xs : undefined;
return current;
}
Loading