fix(toolkit): support diff on multiple stacks #1855

rix0rrr · 2019-02-25T14:27:14Z

If there are stack dependencies, 'diff' would fail because it does not
know how to diff multiple stacks. Make diff support the same stack
selection mechanisms as 'cdk deploy'.

Move 'stack rename' facilities into the class that deals with the
CDK app, which is the source of thruth for stacks. This way, all
downstream code doesn't have to deal with the renames every time.

Start factoring out toolkit code into logical layers. Introducing
the class CdkToolkit, which represents the toolkit logic and
forms the bridge between AppStacks which deals with the CDK
model source (probably needs to be renamed to something better)
and CfnProvisioner, which deals with the deployed stacks.

N.B.: The indirection to a provisioner class with an interface is
because the interface is going to be complex (therefore, interface
over a set of functions that take callbacks) and we want to depend
just on the interface so it's easy to stub out for testing.

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- 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.

If there are stack dependencies, 'diff' would fail because it does not know how to diff multiple stacks. Make diff support the same stack selection mechanisms as 'cdk deploy'. Move 'stack rename' facilities into the class that deals with the CDK app, which is the source of thruth for stacks. This way, all downstream code doesn't have to deal with the renames every time. Start factoring out toolkit code into logical layers. Introducing the class `CdkToolkit`, which represents the toolkit logic and forms the bridge between `AppStacks` which deals with the CDK model source (probably needs to be renamed to something better) and `CfnProvisioner`, which deals with the deployed stacks. N.B.: The indirection to a provisioner class with an interface is because the interface is going to be complex (therefore, interface over a set of functions that take callbacks) and we want to depend just on the interface so it's easy to stub out for testing.

packages/aws-cdk/bin/cdk.ts

RomainMuller · 2019-02-27T14:21:24Z

packages/aws-cdk/lib/api/provisioner.ts

+/**
+ * Interface for provisioners
+ *
+ * Provisioners apply templates to the cloud infrastructure.


Currently this is more of an inspection than an application...

It's aspirational and a guide to future maintainers.

packages/aws-cdk/lib/cdk-toolkit.ts

eladb

I am okay merging this and moving forward, but bear in mind that this is going to quite dramatically change as we generalize artifacts (see #1893) and formalize the build step (#1435).

eladb · 2019-02-25T14:58:56Z

packages/aws-cdk/bin/cdk.ts

@@ -66,7 +67,8 @@ async function parseCommandLineArguments() {
 .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs
 .option('exclusively', { type: 'boolean', alias: 'x', desc: 'only deploy requested stacks, don\'t include dependees' })
 .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' }))
- .command('diff [STACK]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', yargs => yargs
+ .command('diff [STACKS..]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', yargs => yargs
+ .option('exclusively', { type: 'boolean', alias: 'e', desc: 'only deploy requested stacks, don\'t include dependencies' })


"only diff requested stacks"

eladb · 2019-02-27T14:14:17Z

packages/aws-cdk/lib/api/provisioner.ts

+/**
+ * Default provisioner (applies to CloudFormation).
+ */
+export class CfnProvisioner implements IProvisioner {


May I ask that we use CloudFormation as a prefix (I prefer we keep "Cfn" for L1 classes).

eladb · 2019-02-27T14:15:33Z

packages/aws-cdk/lib/api/provisioner.ts

+ *
+ * Provisioners apply templates to the cloud infrastructure.
+ */
+export interface IProvisioner {


Can we use DeploymentTask to align with the terminology from #1893?

Yes on Deployment, not sure Task is the right word. DeploymentTarget?

I agree that Task is not good. Maybe DeploymentProvider?

eladb · 2019-02-27T14:20:12Z

packages/aws-cdk/lib/api/provisioner.ts

+ * Provisioners apply templates to the cloud infrastructure.
+ */
+export interface IProvisioner {
+ readCurrentTemplate(stack: cxapi.SynthesizedStack): Promise<Template>;


If this is a general purpose provisioner, that this should be something like readCurrentState: Promise<any> because I would also like us to view uploading to S3 and pushing to ECR as "provisioning" (or deployment tasks).

Also related to #395

At some point we should have generalized templates, but somewhere in the stack we should also specialize for clarity (otherwise we might as well pass anys around everywhere).

It's not entirely clear to me how this would generalize either. For example, does readCurrentState() for an asset deployer return the payload (wasteful) or the metadata (inconsistent with the template).

So I'd rather avoid premature generalization for now.

For example, this could be the layer under the layer that deals with artifacts, in which case readCurrentTemplate() is perfectly appropriate.

ok, let's leave for now, and we'll tackle soon

For clarity: I'm only introducing the interface so that I can provide a stub for testability. The IProvisioner or IDeploymentTarget or whatever can go in a future refactor far as I'm concerned.

The germane things about the refactoring are separating out the CloudFormationProvider and adding dependency injection for testing.

eladb · 2019-02-27T14:22:24Z

packages/aws-cdk/lib/cdk-toolkit.ts

+ /**
+ * The provisioning engine used to apply changes to the cloud
+ */
+ provisioner: IProvisioner;


We may have a different provisioner per artifact type. At the moment we only model "cloudformation stacks" as artifacts but we are on the process of generalizing this and then each artifact type could have a different provisioner, so I can envision this to be coupled with AppStacks (which will also be generalized).

…k-diff

rix0rrr requested review from RomainMuller, eladb and sam-goodwin February 25, 2019 14:27

rix0rrr requested a review from a team as a code owner February 25, 2019 14:27

Rico Huijbers and others added 2 commits February 26, 2019 16:04

Be compatible with version of Writable on build server

4277db6

Change type the other way

7f31bf4

RomainMuller approved these changes Feb 27, 2019

View reviewed changes

eladb approved these changes Feb 27, 2019

View reviewed changes

rix0rrr and others added 2 commits February 27, 2019 15:54

Review tweaks

a0edb71

Merge remote-tracking branch 'origin/master' into huijbers/multi-stac…

5910f54

…k-diff

RomainMuller merged commit 72d2535 into master Feb 28, 2019

RomainMuller deleted the huijbers/multi-stack-diff branch February 28, 2019 09:03

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(toolkit): support diff on multiple stacks #1855

fix(toolkit): support diff on multiple stacks #1855

rix0rrr commented Feb 25, 2019

RomainMuller Feb 27, 2019

rix0rrr Feb 27, 2019

eladb left a comment

eladb Feb 25, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

rix0rrr Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

rix0rrr Feb 27, 2019

rix0rrr Feb 27, 2019

eladb Feb 27, 2019

rix0rrr Feb 27, 2019

eladb Feb 27, 2019

fix(toolkit): support diff on multiple stacks #1855

fix(toolkit): support diff on multiple stacks #1855

Conversation

rix0rrr commented Feb 25, 2019

Pull Request Checklist

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