-
Notifications
You must be signed in to change notification settings - Fork 25
fix(toolkit-lib)!: fromAssemblyBuilder
mutates globally shared process.env
#528
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
base: main
Are you sure you want to change the base?
Conversation
…uilder` The Toolkit communicates with the CDK app passing environment variables (containing context, and others). If the CDK app is executed as a subprocess it is trivial to set those environment variables when executing the subprocess; but for an in-memory synthesis as allowed by `fromAssemblyBuilder` mutating `process.env` is not the correct approach: it is clobbering global state, and multiple async assembly builders might fight with each other over the single shared global object. In `fromAssemblyBuilder()`, we now pass an additional `env` prop containing the environment variables we didn't apply. There were already parametes for `outdir` and `context`; those *must* now be passed to an `App` instantiated inside the builder (whereas previously the `App` would pick up on the globally mutated `process.env` changes). The rest of this PR is reorganizing the code to make the code that touches the environment return values instead of directly applying them.
fromAssemblyBuilder
fromAssemblyBuilder
mutates globally shared process.env
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #528 +/- ##
==========================================
+ Coverage 78.92% 79.20% +0.27%
==========================================
Files 46 46
Lines 6980 6957 -23
Branches 777 777
==========================================
+ Hits 5509 5510 +1
+ Misses 1452 1427 -25
- Partials 19 20 +1
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:
|
TODO: this pessimizes the default API for an edge case. Retain all the code changes, but make it an option; leave the legacy behavior by default. This will require capturing the OUTDIR and CONTEXT envvars in the |
The Toolkit communicates with the CDK app passing environment variables (containing context, and others).
If the CDK app is executed as a subprocess it is trivial to set those environment variables when executing the subprocess; but for an in-memory synthesis as allowed by
fromAssemblyBuilder
mutatingprocess.env
is not the correct approach: it is clobbering global state, and multiple async assembly builders might fight with each other over the single shared global object.In
fromAssemblyBuilder()
, we now pass an additionalenv
prop containing the environment variables we didn't apply. There were already parametes foroutdir
andcontext
; those must now be passed to anApp
instantiated inside the builder (whereas previously theApp
would pick up on the globally mutatedprocess.env
changes).The rest of this PR is reorganizing the code to make the code that touches the environment return values instead of directly applying them.
withEnv()
andwithContext()
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license