Local Build env vars should NOT be set on the global process#10538
Local Build env vars should NOT be set on the global process#10538falahat wants to merge 4 commits into
Conversation
…dentally inject raw secret values into the terminal.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors environment variable handling during local builds by passing them explicitly through function arguments instead of mutating the global process.env. Feedback focuses on improving type safety by using NodeJS.ProcessEnv instead of generic records and replacing any types in test stubs with proper TypeScript definitions to align with the repository's style guide.
| export async function runUniversalMaker(projectRoot: string): Promise<AppHostingBuildOutput> { | ||
| export async function runUniversalMaker( | ||
| projectRoot: string, | ||
| addedEnv: Record<string, string> = {}, |
There was a problem hiding this comment.
Using NodeJS.ProcessEnv is more idiomatic for environment variables in Node.js and aligns with the return type of toProcessEnv. This also helps avoid unnecessary type assertions at the call site.
| addedEnv: Record<string, string> = {}, | |
| addedEnv: NodeJS.ProcessEnv = {}, |
| function executeUniversalMakerBinary( | ||
| universalMakerBinary: string, | ||
| projectRoot: string, | ||
| addedEnv: Record<string, string> = {}, |
There was a problem hiding this comment.
Update the type of addedEnv to NodeJS.ProcessEnv to maintain consistency with runUniversalMaker and the standard Node.js environment variable types.
| addedEnv: Record<string, string> = {}, | |
| addedEnv: NodeJS.ProcessEnv = {}, |
| } | ||
| const apphostingBuildOutput = await runUniversalMaker( | ||
| projectRoot, | ||
| addedEnv as Record<string, string>, |
There was a problem hiding this comment.
With the updated signature of runUniversalMaker to accept NodeJS.ProcessEnv, this type assertion is no longer necessary.
| addedEnv as Record<string, string>, | |
| addedEnv, |
| expect(process.env.MY_BUILD_SECRET).to.equal("secret-value"); | ||
| expect(process.env.MY_RUNTIME_SECRET).to.be.undefined; | ||
| expect(process.env.MY_PLAIN_VAR).to.equal("plain-value"); | ||
| sinon.stub(childProcess, "spawnSync").callsFake((command: any, args: any, options: any) => { |
There was a problem hiding this comment.
Avoid using any for function parameters. Use proper types to ensure type safety and adhere to the repository style guide.
| sinon.stub(childProcess, "spawnSync").callsFake((command: any, args: any, options: any) => { | |
| sinon.stub(childProcess, "spawnSync").callsFake((command: string, args: string[], options: childProcess.SpawnSyncOptions) => { |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| sinon.stub(childProcess, "spawnSync").callsFake(() => { | ||
| expect(process.env.MY_PLAIN_VAR).to.equal("plain-value"); | ||
| expect(process.env.ANOTHER_VAR).to.equal("another-value"); | ||
| sinon.stub(childProcess, "spawnSync").callsFake((command: any, args: any, options: any) => { |
There was a problem hiding this comment.
Avoid using any for function parameters. Use proper types to ensure type safety and adhere to the repository style guide.
| sinon.stub(childProcess, "spawnSync").callsFake((command: any, args: any, options: any) => { | |
| sinon.stub(childProcess, "spawnSync").callsFake((command: string, args: string[], options: childProcess.SpawnSyncOptions) => { |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
2 participants
Description
Instead of modifying process.env, this sets env vars on the child process that runs universal maker.
Scenarios Tested
Performed a local build