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

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

Merged
merged 5 commits into from
Feb 28, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Feb 25, 2019

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.
@rix0rrr rix0rrr requested a review from a team as a code owner February 25, 2019 14:27
packages/aws-cdk/bin/cdk.ts Outdated Show resolved Hide resolved
/**
* Interface for provisioners
*
* Provisioners apply templates to the cloud infrastructure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's aspirational and a guide to future maintainers.

packages/aws-cdk/lib/cdk-toolkit.ts Show resolved Hide resolved
packages/aws-cdk/lib/cdk-toolkit.ts Show resolved Hide resolved
packages/aws-cdk/lib/cdk-toolkit.ts Show resolved Hide resolved
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -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' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"only diff requested stacks"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* Provisioners apply templates to the cloud infrastructure.
*/
export interface IProvisioner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that Task is not good. Maybe DeploymentProvider?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also related to #395

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* The provisioning engine used to apply changes to the cloud
*/
provisioner: IProvisioner;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@RomainMuller RomainMuller merged commit 72d2535 into master Feb 28, 2019
@RomainMuller RomainMuller deleted the huijbers/multi-stack-diff branch February 28, 2019 09:03
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants