feat(stack): GuStack accepts Stack value as prop #243

stephengeller · 2021-02-11T15:41:44Z

What does this change?

As we look to reduce the number of parameters requiring manual updating and maintaining in CFN, this allows library users to hard-code stack and stage variables so we can reason with their values in other parts of the codebase.

If a user would like to avoid having the stack and stage as tokens (which later gets resolved at deployment by writing the values in the CFN UI), you could now simply state these values in the stack declaration:

const stack =  new GuStack(new App(), "Test", { app: "testing", stack: "deploy", stage: "CODE" });

Since we don't often (or ever) write stage-specific CDK declarations, I expect the stage prop will not be used often. However, being able to provide the stack as a hardcoded string I think provides value in a) having more configuration explicit in the repository, b) remove a parameter from CFN UI and c) remove a tiny bit of cognitive load in the process

Does this change require changes to existing projects or CDK CLI?

No, this shouldn't affect existing projects.

How to test

./script/test from the root of the repository.

How can we measure success?

More projects can be explicit about what stack (and perhaps rarely stage) the CDK stacks belong to.

As we look to reduce the number of parameters requiring manual updating and maintaining in CFN, this allows library users to hard-code stack and stage variables so we can reason with their values in other parts of the codebase

akash1810 · 2021-02-11T17:36:06Z

I think we'd always want stage to be a parameter as it can only be defined at runtime - otherwise we'd be using a different template per stage.

I also wonder if it would be simpler to remove the stack parameter entirely too in favour of a prop?

stephengeller · 2021-02-12T09:16:33Z

I think we'd always want stage to be a parameter as it can only be defined at runtime - otherwise we'd be using a different template per stage.

Agreed, but would it not be nice to make it possible to have it defined at declaration? I agree that for the Guardian, we would almost never use this, but I can envision certain circumstances where maybe an external user might want to create very specific stacks per stage, and have them hard-coded in the file.

I also wonder if it would be simpler to remove the stack parameter entirely too in favour of a prop?

Yeah, I think it would be better to remove the parameter altogether. However, would this cause a breaking change for stacks that have it as a parameter currently? If so, I would maybe suggest allowing it for now then maybe removing it later, although it could be better to just make the necessary changes in the stacks and make this our best practice. I think it should be fine to remove parameters, whereas adding one causes issues iirc.

akash1810 · 2021-02-12T10:19:50Z

I agree that for the Guardian, we would almost never use this, but I can envision certain circumstances where maybe an external user might want to create very specific stacks per stage, and have them hard-coded in the file.

Default values can help alleviate this pain, I think?

Whilst we should consider external use, I'm not sure we should be overly concerned with those use cases at the moment. That is, I think we should build for our internal use to make adoption easy and once the library is mature, we can focus on external parties. Maybe we can add a TODO to remind us that this feature might be tricky for external users?

However, would this cause a breaking change for stacks that have it as a parameter currently?

Semver releases solve this 😉. We also don't have too many stacks on CDK so should be easy to manage this change?

I think it should be fine to remove parameters, whereas adding one causes issues iirc.

Great point! Maybe this warrants an ADR?

Since stack is not a sensitive object and would be stable between stages, we can state it explicitly in the CDK declaration BREAKING CHANGE: This would require existing stacks to declare the stack name in their stacks (we need better terms), but I think the removal of a CFN parameter should be safe

stephengeller · 2021-02-12T13:42:55Z

Whilst we should consider external use, I'm not sure we should be overly concerned with those use cases at the moment. That is, I think we should build for our internal use to make adoption easy and once the library is mature, we can focus on external parties. Maybe we can add a TODO to remind us that this feature might be tricky for external users?

You're right, that makes sense. I've made a change for the stage value to be only a parameter, while the stack one should only be passed in. It makes sense, and is in line with our best practices.

I think it should be fine to remove parameters, whereas adding one causes issues iirc.

Great point! Maybe this warrants an ADR?

Sure! How do we go about that again?

jacobwinch · 2021-02-16T12:11:40Z

src/constructs/core/stack.test.ts

-    const stack = simpleGuStackForTesting();
+  it("requires passing the stack value as props", function () {
+    const stack = simpleGuStackForTesting({ stack: "some-stack" });
+    expect(stack.stack).toEqual("some-stack");

    const json = SynthUtils.toCloudFormation(stack) as SynthedStack;

    expect(json.Parameters).toEqual({


I don't think we need the expect(json.Parameters) assertion here, as it's tested as part of "should have a stage parameter"

jacobwinch

This looks good to me. I've made one suggestion which allows us to simplify the tests slightly. Feel free to merge this once that's been tidied up 👍

A CFN parameter for the stack name is not necessary when defining a stack with GuCDK. Indeed since #243, `GuStack` takes a mandatory string for `stack` in it's constructor. Due to the changes in #243, it follows that `GuStackParameter` is never used and is dead code.

Stephen Geller added 2 commits February 11, 2021 15:26

test(stack): add test for only passing one of stack or stage

8a0ee36

stephengeller requested review from akash1810, jacobwinch and a team February 11, 2021 15:41

Stephen Geller added 2 commits February 12, 2021 13:05

Rework to only require stack

6ebe629

stephengeller changed the title ~~feat(stack): allow GuStack to accept Stack and Stage values as props~~ feat(stack): GuStack accepts Stack value as prop Feb 16, 2021

jacobwinch reviewed Feb 16, 2021

View reviewed changes

jacobwinch approved these changes Feb 16, 2021

View reviewed changes

Remove unnecessary extra test expectation

7719c76

stephengeller merged commit c395d80 into main Feb 16, 2021

stephengeller deleted the sg/stack-and-stage-as-props branch February 16, 2021 12:25

akash1810 mentioned this pull request Jun 29, 2021

fix: Remove the GuStackParameter construct #646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stack): GuStack accepts Stack value as prop #243

feat(stack): GuStack accepts Stack value as prop #243

stephengeller commented Feb 11, 2021

akash1810 commented Feb 11, 2021

stephengeller commented Feb 12, 2021

akash1810 commented Feb 12, 2021

stephengeller commented Feb 12, 2021

jacobwinch Feb 16, 2021 •

edited

Loading

jacobwinch left a comment

feat(stack): GuStack accepts Stack value as prop #243

feat(stack): GuStack accepts Stack value as prop #243

Conversation

stephengeller commented Feb 11, 2021

What does this change?

Does this change require changes to existing projects or CDK CLI?

How to test

How can we measure success?

akash1810 commented Feb 11, 2021

stephengeller commented Feb 12, 2021

akash1810 commented Feb 12, 2021

stephengeller commented Feb 12, 2021

jacobwinch Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

jacobwinch left a comment

Choose a reason for hiding this comment

jacobwinch Feb 16, 2021 •

edited

Loading