cdk-bundle #1954
cdk-bundle #1954
Conversation
An initial implementation which extracts the build phase from the toolkit into a *jsii* library. The library can read build.json files and execute the tasks defines in it in topological order. Currently supports 'copy', 'zip' and 'docker' tasks. The assets.Artifact class synthesizes a directory structure compatible with this library. Enable using `jest` for testing (simply use `jest` instead of `cdk-test`)
| const sourcePath = path.resolve(props.path); | ||
| validateAssetOnDisk(sourcePath, props.packaging); | ||
|
|
||
| // calculate content hash, which is what we use as the app-level ID of this asset |
There was a problem hiding this comment.
I think the app-level ID should also contain the packaging type at least.
| const artifact = root.node.tryFindChild(id) as AssetArtifact || new AssetArtifact(root, id, { | ||
| fingerprint: fp, | ||
| path: sourcePath, | ||
| ...props |
There was a problem hiding this comment.
Move ...props upfront... Otherwise it could replace fingerprint and path.
| super(scope, id); | ||
|
|
||
| this.fingerprint = props.fingerprint; | ||
| this.artifactId = id; // based on the fingerprint |
There was a problem hiding this comment.
From this scope, you can't know it's based on the fingerprint. Why not make the ID from within the constructor?
| return { | ||
| bucket: stack.node.findChild(bucketParamId) as cdk.Parameter, | ||
| key: stack.node.findChild(keyParamId) as cdk.Parameter, | ||
| sha256: stack.node.findChild(hashParamId) as cdk.Parameter |
There was a problem hiding this comment.
For some reason, I'd prefer to findOrCreate all three parameters independently from each other. Not willing to die on that hill however.
|
|
||
| const bucketParam = new cdk.Parameter(stack, bucketParamId, { | ||
| type: 'String', | ||
| description: `S3 bucket for asset ${this.fingerprint} from ${this.props.path}`, |
There was a problem hiding this comment.
I don't think the path should be there. It'll be absolute and will change on every different machine that synthesizes it. At the very least if there, should ensure it is made relative to the working directory, I guess?
Also - the path could be only an "example" - if the same file is copied in multiple places, the fingerprint would still de-dupe them to the same fingerprint... And this might generate confusion.
| if (code === 0) { | ||
| resolve(Buffer.concat(stdout).toString('utf-8')); | ||
| } else { | ||
| reject(new Error(`${renderCommandLine(command)} exited with error code ${code}`)); |
There was a problem hiding this comment.
If the process ends due to a signal (SIGSEVG, SIGTERM, SIGKILL, ...), the error message won't be very helpful. In such cases, code might be null.
| * App-level context key/value pairs. | ||
| * | ||
| * If the environment variable `CDK_CONTEXT_JSON` is set, it is parsed as a JSON object | ||
| * and merged with this hash to form the application's context. |
There was a problem hiding this comment.
Which has precedence over the other if there are collisions?
|
|
||
| // both are reverse logic | ||
| this.legacyManifest = this.node.getContext(cxapi.DISABLE_LEGACY_MANIFEST_CONTEXT) ? false : true; | ||
| this.runtimeInformation = this.node.getContext(cxapi.DISABLE_VERSION_REPORTING) ? false : true; | ||
| this.outdir = props.outdir || process.env[cxapi.OUTDIR_ENV] || fs.mkdtempSync(os.tmpdir()); |
There was a problem hiding this comment.
For the records, not a huge fan of creating a temporary directory that will never be cleaned up & might be hard to locate by the customer. Also, this will fail because it'll try to create stuff like /tmp<somerandomjunk> -- as mkdtemp will NOT append a / at the end of it's prefix.
| @@ -246,127 +275,23 @@ export class FileSystemStore implements ISessionStore { | |||
| } | |||
|
|
|||
| public list(): string[] { | |||
| return fs.readdirSync(this.outdir).sort(); | |||
| return fs.readdirSync(this.path).sort(); | |||
| } | |||
|
|
|||
| public finalize() { | |||
There was a problem hiding this comment.
Maybe call this close? I don't know if'd feel natural to me. In any case, the error messages everywhere refer to " has already been locked", but the user never does anything called lock, so it's confusing.
| @@ -511,9 +511,9 @@ export class MustUseCDKTest extends ValidationRule { | |||
| if (!shouldUseCDKBuildTools(pkg)) { return; } | |||
| if (!hasTestDirectory(pkg)) { return; } | |||
|
|
|||
| expectJSON(this.name, pkg, 'scripts.test', 'cdk-test'); | |||
| expectJSONOneOf(this.name, pkg, 'scripts.test', [ 'cdk-test', 'jest' ]); | |||
There was a problem hiding this comment.
Not needed after my change that allows cdk-test to run jest if it is configured.
eladb
removed
package/awscl
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.
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
An initial implementation for a new module called
cdk-bundlewhich aims to implement the new CDK bundling system. It reads abundle.jsonfile and executes the commands in that file in topological order. Currently I extracted "copy", "zip" and "docker" from the toolkit. This change also updates how assets are synthesized, so CDK apps now produce an output directory that's compatible with cdk-bundle and the result of building will be a cloud assembly (yey).Currently supports 'copy', 'zip' and 'docker' tasks.
The
assets.Artifactclass synthesizes a directory structure compatiblewith this library.
Enable using
jestfor testing (simply usejestinstead ofcdk-test)The current toolkit implementation conflates "build" and "deploy", so the next step in this journey is to extract the deployment tasks from the toolkit into yet another module called "cdk-deploy" which will read the built assembly and deploy it to the cloud (i.e. implement the various deployment tasks).
Instead of trying to refactor the toolkit from within (only Rico has the audacity to do that safely), I figured that a more healthy approach, would be to simply extract fresh new modules, jsii-compatible, minimal dependencies, and fully covered by unit tests.
The idea is to have three of those modules:
cdk-synth(app => cdk.out): responsible to do all the trickery around context providerscdk-build(cdk.out => assembly): a build system, this is where Sam goes nuts with Lambda builds and shit)cdk-deploy(assembly => cloud): supports various deployment tasks such as CFN, S3, ECR)Our CLI (and future unified CLIs) will thread those together into a cohesive experience, but they can also be used independently either as command line tools or as cross-language libraries (nirvana!).
TODO
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.