RFC 1: Faster Development Iterations (aka "CDK Watch") #335

offbyone · 2021-06-11T21:28:38Z

Tracking issue: CDK Watch #1
Rendered version

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

eladb

Let's make sure we are aligned on the scope and UX of the feature. Happy to spend some time in a meeting if you need any clarifications.

text/0001-cdk-update.md

Pull request has been modified.

text/0001-cdk-update.md

skinny85

Looks great! My 2 cents.

text/0001-cdk-update.md

skinny85 · 2021-06-16T00:09:37Z

text/0001-cdk-update.md

+```typescript
+const myDevelopmentStack = new ApplicationStack(app, "StackNameForDev", {
+ ...
+ interactiveUpdate: true,


Whether or not we want to make this change, I think the name interactiveUpdate doesn't represent the functionality in this RFC very well.

Maybe something like allowsShortCircuitDeployment?

Let's wordsmith the flag as a last detail, but I think there's room for improvement once we agree on the command name.

skinny85 · 2021-06-16T00:10:37Z

text/0001-cdk-update.md

+of your stacks:
+
+```
+$ cdk update ApplicationStack


I don't like the fact that you have to specify the Stack name on the command line, and mark it as interactiveUpdate: true. Feels like irritating double-booking to me; why do I have to tell you twice that I want to update this stack with cdk update?

I would either have the property on Stack, or specifying the names through CLI arguments. Not both.

The stack is specified because it's possible to have multiple stacks and only want to update one of them, or to watch one, etc. This, and it matches the usage of stacks elsewhere in the CDK cli. I don't like specifying the stacks either, but it is necessary if you have "stages" of stacks.

Then there's no reason for having the interactiveUpdate property at all, and I would remove it.

There is, though! Using an at-work example, for a single application that deploys many stacks, only a very small subset of those stacks should be allowed to be updated using this mechanism, and the others need to be denied by policy somewhere. One way that can be done is to explicitly identify which stacks in an app are suitable for it, another is to assume that the user is going to "know" the ones that are permitted in practice. How, otherwise, would a "production" stack prevent a rapid update, if your app consists of multiple stacks, each corresponding to a deployment stage?

I'm wondering why we are concerned with which ones are permitted, when we are targeting local developer accounts?

skinny85 · 2021-06-16T00:20:23Z

I think this needs to be part of the deploy command and not a separate command. Eventually. this is about deployment of CDK apps. It's not a separate operation.

If I work backwards from the user experience, I don't think users need to "care" if they are short-circuiting or not. They just want to develop their app and have the CLI deploy it continuously and as fast as possible.

So, primarily this feature should be supported via a --watch or (-w) switch to cdk deploy:
cdk deploy --watch
I expect this command to synth & deploy your app and then watch for changes, synthesize your cdk.out directory and perform deployments as needed (with or without short-circuiting). In case the deployment only includes resources that can be short-circuited, it would run the short-circuiting logic. Otherwise, it will perform a normal deployment.

We can add a switch called --fast or --short-circuit (or something else) which will control the behavior of this feature. It has three modes: auto, never and only.

--fast=auto - if the update includes only short-circuitable resources, it will apply them. If the update includes non-short-circuitable resources, it will perform a normal deployment. This is the default if --watch is enabled.

--fast=never - this is the default for cdk deploy - never perform a short-circuit deployment.

--fast=only - only perform short-circuit deployments - fails if there are other updates.

Here are some examples:
cdk deploy --fast=never           # equivalent to "cdk deploy"
cdk deploy --fast=auto            # one-off, allows both normal and short-circuit updates
cdk deploy --fast=only            # one-off, only allows short-cirtcuitable updates
cdk deploy --watch --fast=auto    # continuous, equivalent to "cdk deploy --watch"
cdk deploy --watch --fast=only    # continuous, only allows short-circuitable updates
cdk deploy --watch --fast=never   # continuous, will perform normal CFN updates

My reasoning for having it as a separate command (I'm not married to the update name, BTW) is to make sure this is not used for production deployments. I think that, if this is a separate command, it will be easier to explain in our docs this should not be used in production, and we can also print a warning in the CLI each time the update command is executed.

In your example, I don't like the complexity that --fast changes its default based on whether --watch was passed or not. I think that will be difficult to explain, remember, and might be error-prone for customers.

I also don't see the value of --fast=only. I agree with your initial statement:

I don't think users need to "care" if they are short-circuiting or not. They just want to develop their app and have the CLI deploy it continuously and as fast as possible.

I think giving customers the --fast switch where they can select --fast=only goes against this statement.

Still open: naming and command UX

Pull request has been modified.

- rename the command to `cdk deploy --accelerated` - identify the options for selecting deployable stacks

text/0001-cdk-update.md

eladb

Another review iteration

The RFC needs to dive deeper into the architecture and design (totally fine to do that as we implement/prototype).

For example, I expected to find some information on how changes are identified and associated with a specific update type and what it means to add new resource types.

text/0001-cdk-update.md

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

Pull request has been modified.

- added "help" section for CLI help - clarified that other things than assets are in scope - put down an outline of how the extension model might work

text/0001-cdk-update.md

eladb

There are still quite a lot of unresolved conversations. Please make sure to either address them or add a respond and resolve all conversations before merging.

text/0001-cdk-update.md

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

Pull request has been modified.

- re-ordered the two flags to make `--watch` the primary - re-ordered the usage to center `--watch` - made interactive use optional, default to non-interactive - move step functions workflows into the primary implementation - remove references to the "toolkit"

skinny85

LGTM!

@eladb do you have any more comments?

eladb

Lgtm

offbyone force-pushed the 0001-cdk-update branch from 77ed849 to f349f90 Compare June 11, 2021 21:31

CDK RFC for update

dc74db4

offbyone force-pushed the 0001-cdk-update branch from f349f90 to dc74db4 Compare June 11, 2021 21:32

eladb previously requested changes Jun 13, 2021

View reviewed changes

offbyone requested a review from eladb June 14, 2021 19:56

ericzbeard reviewed Jun 15, 2021

View reviewed changes

text/0001-cdk-update.md Outdated Show resolved Hide resolved

offbyone force-pushed the 0001-cdk-update branch from 2cd68e1 to c657427 Compare June 15, 2021 21:42

skinny85 previously requested changes Jun 16, 2021

View reviewed changes

Address eladb's comments on scope.

8f0d98c

Still open: naming and command UX

offbyone force-pushed the 0001-cdk-update branch from c657427 to 8f0d98c Compare June 16, 2021 22:24

eladb changed the title ~~CDK Update~~ RFC 1: Faster Development Iterations (aka "CDK Watch") Jun 22, 2021

Chris Rose added 2 commits June 22, 2021 16:29

Update post-sync w/ Elad

eb0bb40

- rename the command to `cdk deploy --accelerated` - identify the options for selecting deployable stacks

faq: address deployment safety

8e01a68

ericzbeard reviewed Jun 23, 2021

View reviewed changes

text/0001-cdk-update.md Show resolved Hide resolved

corymhall reviewed Jun 23, 2021

View reviewed changes

text/0001-cdk-update.md Show resolved Hide resolved

eladb previously requested changes Jun 24, 2021

View reviewed changes

Typo fix.

2cf7639

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

Updates for PR review

db10d3d

- added "help" section for CLI help - clarified that other things than assets are in scope - put down an outline of how the extension model might work

wong-a reviewed Jun 30, 2021

View reviewed changes

text/0001-cdk-update.md Show resolved Hide resolved

wong-a reviewed Jun 30, 2021

View reviewed changes

text/0001-cdk-update.md Outdated Show resolved Hide resolved

rix0rrr reviewed Jul 5, 2021

View reviewed changes

text/0001-cdk-update.md Outdated Show resolved Hide resolved

rix0rrr reviewed Jul 5, 2021

View reviewed changes

text/0001-cdk-update.md Show resolved Hide resolved

Expanded the implementation plan

736a7a9

eladb previously requested changes Jul 7, 2021

View reviewed changes

text/0001-cdk-update.md Outdated Show resolved Hide resolved

text/0001-cdk-update.md Outdated Show resolved Hide resolved

Apply suggestions from code review

86ac9f8

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

offbyone requested review from skinny85 and eladb July 13, 2021 14:54

skinny85 approved these changes Jul 13, 2021

View reviewed changes

skinny85 added the pr/do-not-merge Let mergify know not to auto merge label Jul 13, 2021

eladb approved these changes Jul 14, 2021

View reviewed changes

eladb added the status/api-approved API Bar Raiser signed-off the API of this RFC label Jul 14, 2021

skinny85 removed the pr/do-not-merge Let mergify know not to auto merge label Jul 15, 2021

skinny85 merged commit 7c9bcab into aws:master Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC 1: Faster Development Iterations (aka "CDK Watch") #335

RFC 1: Faster Development Iterations (aka "CDK Watch") #335

offbyone commented Jun 11, 2021 •

edited by skinny85

Loading

eladb left a comment

skinny85 left a comment

skinny85 Jun 16, 2021

offbyone Jun 16, 2021

skinny85 Jun 16, 2021

offbyone Jun 16, 2021

skinny85 Jun 16, 2021 •

edited

Loading

offbyone Jun 17, 2021

ericzbeard Jun 17, 2021

skinny85 commented Jun 16, 2021

eladb left a comment

eladb left a comment

skinny85 left a comment

eladb left a comment

RFC 1: Faster Development Iterations (aka "CDK Watch") #335

RFC 1: Faster Development Iterations (aka "CDK Watch") #335

Conversation

offbyone commented Jun 11, 2021 • edited by skinny85 Loading

eladb left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Jun 16, 2021

Choose a reason for hiding this comment

offbyone Jun 16, 2021

Choose a reason for hiding this comment

skinny85 Jun 16, 2021

Choose a reason for hiding this comment

offbyone Jun 16, 2021

Choose a reason for hiding this comment

skinny85 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

offbyone Jun 17, 2021

Choose a reason for hiding this comment

ericzbeard Jun 17, 2021

Choose a reason for hiding this comment

skinny85 commented Jun 16, 2021

eladb left a comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

offbyone commented Jun 11, 2021 •

edited by skinny85

Loading

skinny85 Jun 16, 2021 •

edited

Loading