-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
"pkglint": "^0.28.0", | ||
"sinon": "^7.2.7" | ||
}, | ||
"dependencies": { |
There was a problem hiding this comment.
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 }); |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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...
There was a problem hiding this 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
).
…app selection into StackSelector class in common.
What to do with renames? Is it the responsibility of |
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.
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 upcomingcdk-synth
,cdk-resolve
andcdk-bundle
tools.TODO:
cdk-deploy
CLI interface (main).cdk-deploy
andcdk-common
package.json
forcdk-common
andcdk-deploy
- just copy-pasted fromaws-cdk
right now which is probably excessive.Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.