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

misleading init templates and sample app #5903

Closed
iliapolo opened this issue Jan 22, 2020 · 7 comments
Closed

misleading init templates and sample app #5903

iliapolo opened this issue Jan 22, 2020 · 7 comments
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-discussion This issue/PR requires more discussion with community. p1 package/tools Related to AWS CDK Tools or CLI

Comments

@iliapolo
Copy link
Contributor

iliapolo commented Jan 22, 2020

❓ General Issue

Our current init templates and sample app don't really encourage the behavior and programming model we want customers to adopt.

The problem is this:

export class %name.PascalCased%Stack extends cdk.Stack {
  ...
  ...
}

The fact we are defaulting to extending cdk.Stack sets users in a trajectory to actually use Stacks as the sharable, reusable, component. In reality, these components should simply be regular constructs.

A Stack might encapsulate deployment context that isn't really sharable or generic:

  • Environment (account, region)
  • Stack name
  • availabilityZones
  • ...

It feels like our core Stack (similar to App) object should be used when defining the actual application that will be deployed (what we currently define in bin), and not the re-usable component (what we currently define in lib).

It follows, that stacks should probably only have one construct.

@eladb Did mention that Stacks can be used inside organizations to enforce specific policies related to deployment, but that probably shouldn't be our main use-case.

As far as templates and examples go, we should have a distinction between customers who are merely construct users, and those who are construct authors.

For example, for users, perhaps a more natural and simplified version of the template could be:

index.ts

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from '@aws-cdk/core';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'MyStack');

// The code that defines your stack goes here

This structure is much simpler for customers who simply want to create a stack and deploy it.

For authors, which are more advanced customers, we can stick to our current template structure, but extending cdk.Construct instead of cdk.Stack.

export class %name.PascalCased%Construct extends cdk.Construct {
  ...
  ...
}

This distinction, between authors and users, is valid and exists in most programming languages and tools, with regards to best practices and such.

We should also probably create a some kind of best practices document, which will also detail when should you actually choose to extend Stack instead of Construct.

The Question

Thoughts?

Environment

  • CDK CLI Version: ALL
  • Module Version: ALL
  • OS: ALL
  • Language: ALL
@iliapolo iliapolo added the needs-triage This issue or PR still needs to be triaged. label Jan 22, 2020
@nija-at
Copy link
Contributor

nija-at commented Jan 22, 2020

I'm not strongly opinionated on this.

FWIW, I like our current structure because it hints at a potential code organization that we should encourage for large CDK appplications, namely, separate file per stack. Keeps the app configuration separate from the stack configuration.

In the world where we're doing CI/CD, all stacks are defined in their own classes and the account to stack mapping (+ configuration) sits alongside the app definition.

@eladb
Copy link
Contributor

eladb commented Jan 22, 2020

How about making this even simpler for the simple use case?

import { SingleStackApp } from '@aws-cdk/core';
import * as s3 from '@aws-cdk/aws-s3';

const app = new SingleStackApp();
new s3.Bucket(app, 'MyBucket');
app.synth();

@hoegertn
Copy link
Contributor

I think the difference between users and authors is already there with the app template and the lib template, isn't it?

I really like the current structure and it works well in several scenarios.

@eladb
Copy link
Contributor

eladb commented Jan 22, 2020

I think the main problem with the current template is that stacks should not live under lib/ because we want to encourage users to reuse through constructs and not through stacks. I also feel bin/ is not the right place for the app's entry point because you don't execute it yourself (the CLI executes it for you).

In the sample-app template, we can keep lib/ but then only put constructs there that are imported from the main index.ts file:

lib/my-construct.ts

export class MyConstruct { ... }

And then:

index.ts

import { MyConstruct } from './lib/my-construct.ts'

const app = new SingleStackApp(app, 'MyStack');
new MyConstruct(stack, '...');

app.synth()

@iliapolo
Copy link
Contributor Author

Now i'm leaning towards simply changing extends cdk.Stack to extends cdk.Construct in the app template and thats it for now. It will also make the app and lib directory differ in just the bin directory, which makes sense.

@SomayaB SomayaB added the package/tools Related to AWS CDK Tools or CLI label Jan 22, 2020
@SomayaB SomayaB added feature-request A feature should be added or improved. needs-discussion This issue/PR requires more discussion with community. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 22, 2020
@richardhboyd
Copy link
Contributor

@eladb I don't agree with the super-simple one-file approach, We had that in the beginning and people made these monstrosities of monofiles that held many levels of abstractions.

@iliapolo what about having the created Construct in sample-app extend cdk.Construct but then contains 2 'cdk.Stacks' to show the preferred organization for an app and also demonstrate passing values between Stacks (by far the most common question asked by new users)

@shivlaks shivlaks added the effort/medium Medium work item – several days of effort label Feb 4, 2020
@shivlaks shivlaks added the p1 label Aug 21, 2020
@NGL321 NGL321 assigned rix0rrr and unassigned shivlaks Jan 25, 2021
@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-discussion This issue/PR requires more discussion with community. p1 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

No branches or pull requests

8 participants