feat(core): allow multiple Apps #1582

rix0rrr · 2019-01-20T21:08:47Z

Change the meaning of App to represent the application the user
is defining (as opposed to the internals of the CDK program). The
names of Apps (if given) will be incorporated into the Stack name,
so that it becomes possible to instantiate your Stacks multiple
times just like other constructs (without having to make the
id parameter unique). A new stack property stackName exists
to force the stack to a different name.

If a single App is instantiated without parameters, the observable
behavior of your CDK app won't change. If you insantiate more than
one App, all of them must receive ids.

To represent the CDK internals, a new class called Program exists,
which the user doesn't need to interact with.

app.run() is no longer necessary, and is deprecated. It still
exists to avoid breaking existing programs, but will be removed
in a future release.

Fixes #1479.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Change the meaning of `App` to represent the application the user is defining (as opposed to the internals of the CDK program). The names of Apps (if given) will be incorporated into the Stack name, so that it becomes possible to instantiate your Stacks multiple times just like other constructs (without having to make the `id` parameter unique). A new stack property `stackName` exists to force the stack to a different name. If a single `App` is instantiated without parameters, the observable behavior of your CDK app won't change. If you insantiate more than one `App`, all of them must receive ids. To represent the CDK internals, a new class called `Program` exists, which the user doesn't need to interact with. `app.run()` is no longer necessary, and is deprecated. It still exists to avoid breaking existing programs, but will be removed in a future release.

BDQ · 2019-01-21T12:25:39Z

From a functional perspective this looks great, and totally solves my request in #1479.

The only thing I'd note is Program seems a little generic for the name of that class, would Manager or Executor be a better?

rix0rrr · 2019-01-21T17:35:39Z

Just saw in Gitter that someone is relying on app.synthesizeStack() to do validation work for them.

Better retain that behavior then...

sam-goodwin · 2019-01-22T21:35:42Z

packages/@aws-cdk/cdk/lib/app.ts

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


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

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?

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' }

Oh I see, as keys.

That will not across JSII, for sure.

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.

+1 for symbols

sam-goodwin · 2019-01-22T21:46:33Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

+    return this.node.rootPath()
+        .filter(c => !App.isDefaultApp(c))
+        .map(c => c.node.id)
+        .join('-');


Am I reading this wrong? rootPath removes the first element so it won't include the App? Your tests pass so I assume so :)

public rootPath(): IConstruct[] { const ancestors = this.ancestors(); ancestors.shift(); return ancestors; }

Program is the actual root of the construct tree, so that's the one that gets cut.

eladb

I rather we opt in for auto-run through the existence of CDK_OUT (happy to rename) than opt out with a new environment variable. The current approach will no doubt trip people off in very weird ways, especially in Jsii languages.

if you are inclined (but ok if we punt), would like to discuss a way to generalize synthesis and move to Construct

eladb · 2019-01-23T20:58:15Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

+ *
+ *     MyStack/Vpc/Subnet, for production stacks
+ *
+ * We do a slightly complicated calculation here because we want to


Is that necessary?

I don't want to have to rerun all integration tests that use EC2 capacity in all projects. Takes forever.

eladb · 2019-01-24T14:26:06Z

packages/@aws-cdk/cdk/lib/app.ts

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


+1 for symbols

eladb · 2019-01-24T14:29:01Z

packages/@aws-cdk/cdk/lib/app.ts

-  constructor() {
-    super();
-    this.loadContext();
+  constructor(id?: string, props: AppProps = {}) {


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

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.

eladb · 2019-01-24T14:30:11Z

packages/@aws-cdk/cdk/lib/app.ts

-    return s;
-  }
+function numberOfAppsInProgram(app: IConstruct): number {
+  return findRoot(app).node.findAll().filter(App.isApp).length;


Don’t we have node.root?

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

eladb · 2019-01-24T14:31:23Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

@@ -179,6 +193,16 @@ export class Stack extends Construct {
    }
  }

+  /**
+   * The environment in which this stack is deployed.
+   */


Why couldn’t me make this eager?

eladb · 2019-01-24T14:34:30Z

packages/@aws-cdk/cdk/lib/program.ts

+  /**
+   * Initialize the default Program instance
+   */
+  private initializeDefaultProgram() {


I’d give this method a more specific name: “autoRunOnExit” or something

I'd suggest simply runBeforeExit.

eladb · 2019-01-24T14:35:48Z

packages/@aws-cdk/cdk/lib/program.ts

+  }
+}
+
+interface ISynthesizable extends IConstruct {


I’ve been wanting to add this at the construct level for ages. Would you be open to discussing?

eladb · 2019-01-24T14:37:56Z

tools/cdk-build-tools/bin/cdk-test.ts

-    await shell(testCommand, timers);
+    await shell(testCommand, {
+      timers,
+      env: { CDK_TEST_MODE: '1' }


This concerns me. I actually rather we opt in from the toolkit to auto run the app on exit instead of opt out, or perhaps base it on the existence of CDK_OUT?

RomainMuller · 2019-01-28T10:53:46Z

packages/@aws-cdk/applet-js/bin/cdk-applet-js.ts

@@ -37,12 +37,13 @@ async function main() {
  const searchDir = path.dirname(appletFile);
  const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'cdkapplet'));
  try {
-    const app = new cdk.App();
+    const program = new cdk.Program();


Could be a cdk.Host, too, for example... (Chipping in on the naming stuff is hard :D)

RomainMuller · 2019-01-28T10:54:52Z

packages/@aws-cdk/applet-js/bin/cdk-applet-js.ts


    for (const [name, definition] of Object.entries(appletMap)) {
      await constructStack(app, searchDir, tempDir, name, definition);
    }
-    app.run();
+    program.run();


Are we making sure you cannot add new app's on there after run was called? (Mentioning here because I see it now)

RomainMuller · 2019-01-28T10:56:56Z

packages/@aws-cdk/cdk/lib/app.ts

+   *
+   * @default Automatically determined
+   */
+  env?: Environment;


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

RomainMuller · 2019-01-28T10:58:38Z

packages/@aws-cdk/cdk/lib/app.ts

+   * True if the given construct is a default-named App
+   */
+  public static isDefaultApp(construct: IConstruct) {
+    return App.isApp(construct) && construct.defaultAppName;


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

RomainMuller · 2019-01-28T10:59:15Z

packages/@aws-cdk/cdk/lib/app.ts

+ */
+export class App extends Construct {
+  /**
+   * True if the given construct is a default-named App


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.

RomainMuller · 2019-01-28T11:10:21Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts


    this.logicalIds = new LogicalIDs(props && props.namingScheme ? props.namingScheme : new HashedAddressingScheme());
-    this.name = this.node.id;
+    this.name = props.stackName !== undefined ? props.stackName : this.calculateStackName();


props.stackName || this.calculateStackName()?

RomainMuller · 2019-01-28T11:11:33Z

packages/@aws-cdk/cdk/lib/program.ts

+import { validateAndThrow } from './util/validation';
+
+/**
+ * Represents a CDK program.


Program and app feel too interchangeable. Would rather call this Host or something more distinctive. No hard feelings though.

RomainMuller · 2019-01-28T11:14:12Z

packages/@aws-cdk/cdk/lib/program.ts

+  /**
+   * Initialize the default Program instance
+   */
+  private initializeDefaultProgram() {


I'd suggest simply runBeforeExit.

RomainMuller · 2019-01-28T11:16:36Z

packages/@aws-cdk/cdk/lib/util/validation.ts

+export function validateAndThrow(construct: IConstruct) {
+  const errors = construct.node.validateTree();
+  if (errors.length > 0) {
+    const errorList = errors.map(e => `[${e.source.node.path}] ${e.message}`).join('\n  ');


I like using \t for indents in messages because it gives users more control over how it renders (it's more accessible).

RomainMuller · 2019-01-28T11:18:46Z

tools/cdk-build-tools/lib/os.ts


  await makeShellScriptExecutable(command[0]);

  const child = child_process.spawn(command[0], command.slice(1), {
    // Need this for Windows where we want .cmd and .bat to be found as well.
    shell: true,
-    stdio: [ 'ignore', 'pipe', 'inherit' ]
+    stdio: [ 'ignore', 'pipe', 'inherit' ],
+    env: { ...process.env, ...(options.env || {}) }


Doc should mention that options.env is additive on top of process.env.
I don't believe this is the usual behavior (typically I'd expect it uses process.env if I don't pass anything, and whatever I passed otherwise).

eladb · 2019-02-04T08:17:19Z

@rix0rrr any updates on this?

rix0rrr requested a review from a team as a code owner January 20, 2019 21:08

Rico Huijbers added 3 commits January 21, 2019 17:02

Restored contract of synthesizeStacks()

022ed01

Make linter more explicit about actionable errors, ignore API violations

22f818a

Fix stack inspector

f765650

sam-goodwin reviewed Jan 22, 2019

View reviewed changes

Rico Huijbers added 5 commits January 22, 2019 15:24

Update applet expectations

6cc27c5

Only set test mode for unit tests

d06e02b

Mention we should do this better once using Jest

0778bd7

Make VPC tag names consistent

6c7888b

Fix metadata changes

ac93c06

eladb suggested changes Jan 24, 2019

View reviewed changes

Use other environment variable, fix some tests

f4201f2

RomainMuller reviewed Jan 28, 2019

View reviewed changes

Use a symbol for isApp

9943293

rix0rrr self-assigned this Feb 4, 2019

eladb mentioned this pull request Feb 28, 2019

feat(core): democratize synthesis and introduce artifacts #1889

Merged

4 tasks

rix0rrr mentioned this pull request Mar 4, 2019

feat(core): can use Constructs to model collections of Stacks #1940

Merged

4 tasks

rix0rrr closed this Mar 4, 2019

rix0rrr deleted the huijbers/app-is-app branch April 23, 2019 07:12

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

feat(core): allow multiple Apps #1582

feat(core): allow multiple Apps #1582

Conversation

rix0rrr commented Jan 20, 2019

Pull Request Checklist

BDQ commented Jan 21, 2019 • edited Loading

rix0rrr commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Feb 4, 2019

BDQ commented Jan 21, 2019 •

edited

Loading