-
Notifications
You must be signed in to change notification settings - Fork 69
refactor(toolkit): cxapp to use moden messaging infrastructure #285
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
Conversation
| /** | ||
| * Compute the bootstrap enviornments | ||
| * | ||
| * @internal |
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 was previously missed to mark internal.
| * | ||
| * @internal | ||
| */ | ||
| async getEnvironments(ioHost: IIoHost): Promise<cxapi.Environment[]> { |
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.
Because it's internal, we can change this.
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 turned into a bigger refactor. I realized a lot of these helpers share the same context, so a new abstraction (class) seemed appropriate. No functional changes.
| * fine in certain scenarios. | ||
| */ | ||
| public async defaultEnvVars(): Promise<Env> { | ||
| const debugFn = (msg: string) => this.ioHelper.notify(IO.CDK_ASSEMBLY_I0010.msg(msg)); |
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.
uses a different code than the other messages in this class. probably doesn't need to. We can make a breaking change at some point.
| const assembly = await changeDir(async () => | ||
| withContext(context.all, env, props.synthOptions ?? {}, async (envWithContext, ctx) => | ||
| withEnv(envWithContext, () => { | ||
| const x = new ExecutionEnviornment(services, { outdir: props.outdir }); |
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.
x for eXecution. Open to better suggestions.
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.
Maybe simply execution.
| const outdir = assembly.directory; | ||
| const fileName = assembly.tree()?.file; | ||
| return fileName ? fs.readJSONSync(path.join(outdir, fileName)).tree : {}; | ||
| return fileName ? fs.readJSONSync(path.join(outdir, fileName)).tree : ({} as ConstructTreeNode); |
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.
We always had it like this, but it's an error in my IDE so I thought I might as well cast it.
8719052 to
edd45e2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
=======================================
Coverage 85.33% 85.34%
=======================================
Files 222 222
Lines 36761 36798 +37
Branches 4447 4448 +1
=======================================
+ Hits 31370 31405 +35
+ Misses 5294 5292 -2
- Partials 97 101 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
edd45e2 to
802c5c1
Compare
802c5c1 to
e998ab8
Compare
Signed-off-by: github-actions <[email protected]>
packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/prepare-source.ts
Outdated
Show resolved
Hide resolved
| const assembly = await changeDir(async () => | ||
| withContext(context.all, env, props.synthOptions ?? {}, async (envWithContext, ctx) => | ||
| withEnv(envWithContext, () => { | ||
| const x = new ExecutionEnviornment(services, { outdir: props.outdir }); |
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.
Maybe simply execution.
packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/prepare-source.ts
Outdated
Show resolved
Hide resolved
| () => runCli(args, async (sdk, config) => { | ||
| const env = await prepareDefaultEnvironment(sdk); | ||
| const context = await prepareContext(config.settings, config.context.all, env); | ||
| const env = await prepareDefaultEnvironment(sdk, debugFn); |
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.
Isn't this the default value anyway?
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.
prepareDefaultEnvironment used to have a default for debugFn, but the default was using the old legacy logging system. Removed the default, made the param mandatory. cli-lib-alpha will use legacy logging until further notice, but we plan to deprecate that package anyway soon.
Refactors the
cxappapi code to use modern messaging. This had a larger amount of knock-on effects then other refactors, to the PR is a bit bigger.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license