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

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

Closed
wants to merge 13 commits into from

Conversation

sam-goodwin
Copy link
Contributor

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.

@sam-goodwin sam-goodwin requested a review from a team as a code owner April 17, 2019 05:53
packages/@aws-cdk/cdk-common/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/cdk-deploy/package.json Outdated Show resolved Hide resolved
"pkglint": "^0.28.0",
"sinon": "^7.2.7"
},
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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.

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

@sam-goodwin
Copy link
Contributor Author

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

@eladb
Copy link
Contributor

eladb commented Apr 22, 2019

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

@eladb eladb closed this in #2636 May 29, 2019
eladb pushed a commit that referenced this pull request May 29, 2019
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.
@RomainMuller RomainMuller deleted the samgood/cdk-deploy branch August 10, 2019 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants