Repository navigation
feat(toolkit-lib): toolkit.fromCdkAppDirectory - #2062
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## explorer_2_assembly_loading #2062 +/- ##
============================================================
Coverage 92.10% 92.10%
============================================================
Files 82 82
Lines 13189 13189
Branches 1879 1879
============================================================
Hits 12148 12148
Misses 1003 1003
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| */ | ||
| public async fromCdkAppDirectory(appDirectory: string, props: FromCdkAppDirectoryOptions = {}): Promise<ICloudAssemblySource> { | ||
| const cdkJsonPath = path.join(appDirectory, 'cdk.json'); | ||
|
|
There was a problem hiding this comment.
⚪ Optional — The new fromCdkAppDirectory documents synthOptions as overriding named settings the way the CLI command line overrides cdk.json, and the tests confirm this for versionReporting and validation. But AppSynthOptions exposes assetStaging and bundlingForStacks, while the settings-to-context translation reads the keys staging and bundlingStacks. Because settingsFromSynthOptions(..., 'onlyThese') spreads the option keys verbatim into the merged Settings, a caller passing synthOptions: { assetStaging: false } or { bundlingForStacks: ... } writes a key that is never read, so the override silently has no effect for those two settings. The underlying key mismatch is pre-existing and shared with the other builders, so this is a question about whether the new method's advertised override contract is meant to cover those fields, not a defect introduced here.
Suggested change: Confirm whether assetStaging/bundlingForStacks overrides are expected to take effect through synthOptions. If so, translate the AppSynthOptions key names to the settings keys (staging, bundlingStacks) in the shared settingsFromSynthOptions path rather than spreading them verbatim.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
|
||
| let cdkJsonSettings: Settings; | ||
| try { | ||
| const cdkJson = JSON.parse(await fs.promises.readFile(cdkJsonPath, 'utf-8')); | ||
| cdkJsonSettings = new Settings(cdkJson); | ||
| } catch (e: any) { | ||
| if (e.code === 'ENOENT') { | ||
| throw new ToolkitError('CdkJsonMissing', `fromCdkAppDirectory: cdk.json not found at ${cdkJsonPath}`); | ||
| } | ||
| throw e; |
There was a problem hiding this comment.
🟡 Recommended — In fromCdkAppDirectory, the try/catch around reading and parsing cdk.json only special-cases a missing file (e.code === 'ENOENT') and re-throws every other error unchanged. A syntactically invalid cdk.json makes JSON.parse throw a raw SyntaxError (no .code), so a common user mistake such as a trailing comma surfaces as a bare message like Unexpected token } in JSON — not part of the ToolkitError family and without naming the offending file. Since this is a new entry point's primary path, a programmatic consumer cannot catch it as a ToolkitError or tell which file is at fault.
Suggested change: Catch the parse failure explicitly and wrap it in a ToolkitError that names the file, e.g. throw new ToolkitError('CdkJsonInvalid', \fromCdkAppDirectory: could not parse ${cdkJsonPath}`)(optionally preserving the cause), so it is catchable as aToolkitError` and points the user at the file to fix.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
|
||
| const cleanupContextTemp = writeContextToEnv(env, fullContext, 'env-is-complete'); | ||
| using _cleanupEnv = (props.clobberEnv ?? true) ? temporarilyWriteEnv(env) : undefined; | ||
| let assembly; | ||
| try { | ||
| assembly = await builder({ | ||
| outdir: execution.outdir, | ||
| context: fullContext, | ||
| env, | ||
| abort: produceOptions?.abort, | ||
| }); | ||
|
|
||
| produceOptions?.abort?.throwIfAborted(); | ||
| } catch (error: unknown) { | ||
| // re-throw toolkit errors unchanged | ||
| if (ToolkitError.isToolkitError(error)) { | ||
| throw error; | ||
| } | ||
| // otherwise, wrap into an assembly error | ||
| throw AssemblyError.withCause('Assembly builder failed', error); | ||
| } finally { | ||
| await cleanupContextTemp(); | ||
| abort?.throwIfAborted(); | ||
| } catch (error: unknown) { | ||
| // Re-throw abort error unchanged | ||
| if (error instanceof DOMException && error.name === 'AbortError') { |
There was a problem hiding this comment.
🟡 Recommended — The refactor of fromAssemblyBuilder onto the shared synth path adds a new catch branch that re-throws an AbortError (a DOMException with name === 'AbortError') unchanged, before the toolkit-error and assembly-error wrapping. Previously an abort during the in-process builder fell through to AssemblyError.withCause('Assembly builder failed', ...), so a cancelled synth now surfaces to the caller as a raw AbortError rather than an AssemblyError. This is an observable change on a public toolkit-lib entry point, where backwards compatibility is a strong commitment (AGENTS.md#your-role), and it is not called out in the PR description.
Suggested change: Confirm the change is intentional; if so, add a test asserting that aborting a fromAssemblyBuilder produce rejects with an AbortError (not AssemblyError), and note the behavior change in the PR description.
| * | ||
| * @default - settinsgs read from `cdk.json` | ||
| */ | ||
| readonly synthOptions?: AppSynthOptions; | ||
|
|
||
| /** | ||
| * Options to configure loading of the assembly after it has been synthesized | ||
| * | ||
| * @default - default assembly loading options | ||
| */ | ||
| readonly loadAssemblyOptions?: LoadAssemblyOptions; | ||
|
|
||
| /** | ||
| * Resolve the current default environment an provide as environment variables to the app. | ||
| * | ||
| * This will make a (cached) call to STS to resolve the current account using | ||
| * base credentials. The behavior is not always desirable and can add | ||
| * unnecessary delays, e.g. when an app specifies an environment explicitly |
There was a problem hiding this comment.
⚪ Optional — The doc comments on the newly added public FromCdkAppDirectoryOptions contain typos that render into the published API-Extractor documentation: settinsgs read from cdk.json, Resolve the current default environment an provide, and when local actions are be performed. Because this is a brand-new public API surface, the errors are user-visible rather than purely internal.
Suggested change: Fix the typos: settinsgs→settings; environment an provide→environment and provide; are be performed→are performed.
Uses a CDK application directory (with a
cdk.jsonfile) as the sourceof truth for a cloud assembly source. This behaves the same as the CDK CLI.
Closes #373, closes #958.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Stack created with GitHub Stacks CLI • Give Feedback 💬