[WIP] fix(toolkit) separate aws-cdk into @aws-cdk/cdk-common and @aws-cdk/cdk-deploy by sam-goodwin · Pull Request #2310 · aws/aws-cdk

sam-goodwin · 2019-04-17T05:53:44Z

This is a step towards having separate CLI tools for each step in the deployment process. The deployment and asset packaging logic has been separated out into a new module, @aws-cdk/cdk-deploy, and some fundamental classes have been extracted into @aws-cdk/cdk-common (private module currently) for the upcoming cdk-synth, cdk-resolve and cdk-bundle tools.

TODO:

Implement the cdk-deploy CLI interface (main).
READMEs for cdk-deploy and cdk-common
Run integration tests against the refactored cdk deploy and diff commands.
Refactor package.json for cdk-common and cdk-deploy - just copy-pasted from aws-cdk right now which is probably excessive.

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.

…mands

packages/@aws-cdk/cdk-common/package.json

packages/@aws-cdk/cdk-deploy/package.json

eladb · 2019-04-17T06:23:38Z

packages/@aws-cdk/cdk-deploy/package.json

+    "pkglint": "^0.28.0",
+    "sinon": "^7.2.7"
+  },
+  "dependencies": {


Might be a bit tricky to do at the moment, but one of the goal is to dramatically reduce dependencies for this module given it's sensitivity. For example, I don't think this module needs any fancy CLI/colors/yargs stuff. It should be a low-level utility that is executed by other tools (maybe JSON STDIN, like lambda builders).

Okay if this is too much for the current project, but if there are low-hanging fruit, it would be nice to start cleaning up.

eladb · 2019-04-17T06:30:50Z

packages/aws-cdk/bin/cdk.ts

+      const stacks = await appStacks.selectStacks(
+        stackNames,
+        args.exclusively ? ExtendedStackSelection.None : ExtendedStackSelection.Upstream);
+      return new CDKToolkit({ stacks, provisioner });


I find it weird that all commands must go through this CDKToolkit class which is in cdk-deploy...

Yes, it is a weird setup currently - i did the minimum work to separate them logically. The next step is to wrap cdk-deploy in a CLI interface and call it from aws-cdk instead of routing through this hacky shared dependency.

Did you have thoughts on where the diff command should go? It seems like it has similar security requirements as deploy, so I left it in CDKToolkit class as part of cdk-deploy.

Makes sense. I like it that you are taking the smallest step possible.

I think "diff" needs to go in the main CLI, as well as CDKToolkit. It only needs READ permissions from the account (similar to env context providers), while deploy needs ADMIN WRITE permissions (that's the main motivation for separation).

eladb · 2019-04-17T06:31:54Z

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

@@ -1,11 +1,10 @@
+import { data, deserializeStructure, error, highlight, print, success } from '@aws-cdk/cdk-common';


This class feels like it should go in the common library (or stay in the main toolkit)

eladb · 2019-04-17T06:32:46Z

packages/@aws-cdk/cdk-deploy/bin/cdk-deploy

@@ -0,0 +1,2 @@
+#!/usr/bin/env node
+require('./cdk-deploy.js');


No changes in cdk-deploy.ts, which is weird...

… -> tools/cdk-deploy

eladb

The tools/ directory does not participate in publishing (everything there is considered private), so this tool should go under packages/cdk-deploy (next to packages/decdk).

…app selection into StackSelector class in common.

sam-goodwin · 2019-04-19T03:26:55Z

What to do with renames? Is it the responsibility of cdk-deploy?

eladb · 2019-04-22T15:30:33Z

What to do with renames? Is it the responsibility of cdk-deploy?

Let's take the simplest/pragmatic approach right now so we can land this quickly on master

Formalize the concept of a cloud assembly to allow decoupling "synth" and "deploy". the main motivation is to allow "deploy" to run in a controlled environment without needing to execute the app for security purposes. `cdk synth` now produces a self-contained assembly in the output directory, which we call a "cloud assembly". this directory includes synthesized templates (similar to current behavior) but also a "manifest.json" file and the asset staging directories. `cdk deploy -a assembly-dir` will now skip synthesis and will directly deploy the assembly. To that end, we modified the behavior of asset staging such that all synth output always goes to the output directory, which is by default `cdk.out` (can be set with `--output`). if there's a single template, it is printed to stdout, otherwise the name of output directory is printed. This PR also includes multiple clean ups and deprecations of stale APIs and modules such as: - `cdk.AppProps` includes various new options. - An error is thrown if references between stacks that are not in the same app (mostly in test cases). - Added the token creation stack trace for errors emitted by tokens - Added `ConstructNode.root` which returns the tree root. **TESTING**: verified that all integration tests passed and added a few tests to verify zip and docker assets as well as cloud assemblies. See: https://github.com/awslabs/cdk-ops/pull/364 Closes #1893 Closes #2093 Closes #1954 Closes #2310 Related #2073 Closes #1245 Closes #341 Closes #956 Closes #233 BREAKING CHANGE: *IMPORTANT*: apps created with the CDK version 0.33.0 and above cannot be used with an older CLI version. - `--interactive` has been removed - `--numbered` has been removed - `--staging` is now a boolean flag that indicates whether assets should be copied to the `--output` directory or directly referenced (`--no-staging` is useful for e.g. local debugging with SAM CLI) - Assets (e.g. Lambda code assets) are now referenced relative to the output directory. - `SynthUtils.templateForStackName` has been removed (use `SynthUtils.synthesize(stack).template`). - `cxapi.SynthesizedStack` renamed to `cxapi.CloudFormationStackArtifact` with multiple API changes. - `cdk.App.run()` now returns a `cxapi.CloudAssembly` instead of `cdk.ISynthesisSession`. - `cdk.App.synthesizeStack` and `synthesizeStacks` has been removed. The `cxapi.CloudAssembly` object returned from `app.run()` can be used as an alternative to explore a synthesized assembly programmatically (resolves #2016). - `cdk.CfnElement.creationTimeStamp` may now return `undefined` if there is no stack trace captured. - `cdk.ISynthesizable.synthesize` now accepts a `cxapi.CloudAssemblyBuilder` instead of `ISynthesisSession`. - `cdk`: The concepts of a synthesis session and session stores have been removed (`cdk.FileSystemStore`, cdk.InMemoryStore`, `cdk.SynthesisSession`, `cdk.ISynthesisSession` and `cdk.SynthesisOptions`). - No more support for legacy manifests (`cdk.ManifestOptions` member `legacyManifest` has been removed) - All support for build/bundle manifest have been removed (`cxapi.BuildManifest`, `cxapi.BuildStep`, etc). - Multiple deprecations and breaking changes in the `cxapi` module (`cxapi.AppRuntime` has been renamed to `cxapi.RuntimeInfo`, `cxapi.SynthesizeResponse`, `cxapi.SynthesizedStack` has been removed) - The `@aws-cdk/applet-js` program is no longer available. Use [decdk](https://github.com/awslabs/aws-cdk/tree/master/packages/decdk) as an alternative.

Sam Goodwin added 5 commits April 16, 2019 14:59

Setup @aws-cdk/cdk-common with utilities common between cdk-* CLI com…

e4713fb

…mands

Move classes into cdk-deploy

7820ce8

move tests to cdk-common

b04a821

Move tests into cdk-deploy

311b7cb

Refactor aws-cdk to use @aws-cdk/cdk-common and @aws-cdk/cdk-deploy

0a7075e

sam-goodwin requested a review from a team as a code owner April 17, 2019 05:53

Add back in the stack re-name logic

27fbd0d

eladb reviewed Apr 17, 2019

View reviewed changes

Sam Goodwin added 3 commits April 17, 2019 20:33

@aws-cdk/cdk-common -> tools/toolchain-common and @aws-cdk/cdk-deploy…

905d0b4

… -> tools/cdk-deploy

tools/toolkit-common -> packages/@aws-cdk/toolkit-common

80a206c

Merge branch 'master' into samgood/cdk-deploy

98536e7

eladb reviewed Apr 18, 2019

View reviewed changes

Sam Goodwin added 4 commits April 18, 2019 19:10

Moved more (configuratoon, context, settings) into common. Extracted …

6ecf518

…app selection into StackSelector class in common.

Use StackSelector from within AppStacks

e55d6e4

Have some of cdk-deploy compiling

dbaf11b

move cdk-deploy under packages/

a027bab

eladb mentioned this pull request May 24, 2019

feat: decouple "synth" and "deploy" through cloud assemblies #2636

Merged

6 tasks

eladb closed this in #2636 May 29, 2019

RomainMuller deleted the samgood/cdk-deploy branch August 10, 2019 00:39

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fix(toolkit) separate aws-cdk into @aws-cdk/cdk-common and @aws-cdk/cdk-deploy#2310

[WIP] fix(toolkit) separate aws-cdk into @aws-cdk/cdk-common and @aws-cdk/cdk-deploy#2310
sam-goodwin wants to merge 13 commits intomasterfrom
samgood/cdk-deploy

sam-goodwin commented Apr 17, 2019

Uh oh!

Uh oh!

Uh oh!

eladb Apr 17, 2019

Uh oh!

eladb Apr 17, 2019

Uh oh!

sam-goodwin Apr 17, 2019

Uh oh!

eladb Apr 17, 2019

Uh oh!

eladb Apr 17, 2019

Uh oh!

eladb Apr 17, 2019

Uh oh!

eladb left a comment

Uh oh!

sam-goodwin commented Apr 19, 2019

Uh oh!

eladb commented Apr 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -1,11 +1,10 @@
		import { data, deserializeStructure, error, highlight, print, success } from '@aws-cdk/cdk-common';

		@@ -0,0 +1,2 @@
		#!/usr/bin/env node
		require('./cdk-deploy.js');

Conversation

sam-goodwin commented Apr 17, 2019

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

eladb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

eladb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

sam-goodwin Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

eladb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

eladb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

eladb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

sam-goodwin commented Apr 19, 2019

Uh oh!

eladb commented Apr 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants