feat: add deploy-specific environment variables to dev#7947
feat: add deploy-specific environment variables to dev#7947
Conversation
fdb2610 to
020c389
Compare
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughSummary by CodeRabbit
WalkthroughThreads a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
221f837 to
6adbe65
Compare
6adbe65 to
852cd8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/edge-functions/registry.ts (1)
368-388:⚠️ Potential issue | 🟠 MajorPreserve higher‑precedence env values when merging deployEnvironment.
Avoid overwriting keys already set from higher‑precedence sources in the env map.Based on learnings: Environment variables must be loaded with specific precedence: Process environment > .env files > Netlify site settings > Addon-provided variables > Build-time configuration.🔧 Suggested fix
- for (const { key, value } of deployEnvironment) { - env[key] = value - } + for (const { key, value } of deployEnvironment) { + if (!(key in env)) { + env[key] = value + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/edge-functions/registry.ts` around lines 368 - 388, The merge currently lets deployEnvironment values overwrite env entries from higher‑precedence sources; update getEnvironmentVariables so when iterating deployEnvironment (the array of { key, value, isSecret }), only assign env[key] = value if the key is not already present (e.g., check env[key] === undefined or !(key in env)) to preserve higher precedence values from envConfig (.env, ui, account, addons, internal) and avoid clobbering them.src/lib/functions/netlify-function.ts (1)
272-289:⚠️ Potential issue | 🟠 MajorAvoid deployEnvironment overriding higher‑precedence values.
Guard deployEnvironment entries so they don’t clobber keys already set by higher‑precedence sources (e.g., process env).Based on learnings: Environment variables must be loaded with specific precedence: Process environment > .env files > Netlify site settings > Addon-provided variables > Build-time configuration.🔧 Suggested fix
- const environment = { - // Include function-specific environment variables from config - ...configEnvVars, - ...Object.fromEntries(this.deployEnvironment.map(({ key, value }) => [key, value])), - } + const deployEnvVars = Object.fromEntries( + this.deployEnvironment + .filter(({ key }) => !(key in process.env)) + .map(({ key, value }) => [key, value]), + ) + const environment = { + // Include function-specific environment variables from config + ...configEnvVars, + ...deployEnvVars, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/functions/netlify-function.ts` around lines 272 - 289, The current environment construction allows this.deployEnvironment entries to overwrite higher-precedence variables; update the build in src/lib/functions/netlify-function.ts so deployEnvironment does not clobber existing keys by either (a) filtering this.deployEnvironment to only include entries whose keys are not already present in process.env or configEnvVars, or (b) changing the spread order so higher-precedence sources (process.env and configEnvVars) come after deployEnvironment; locate the relevant symbols configEnvVars, this.deployEnvironment and the environment object and implement the filter or adjust the spread order to preserve the precedence: Process env > .env/site/addon > build config.src/utils/run-build.ts (1)
192-205:⚠️ Potential issue | 🟡 MinorBoth
'build'and'dev'timelines castdeployEnvironmentto a type missing thescopesfield declared in the overloads.The function overloads (lines 59–63, 64–68) declare
deployEnvironmentwithscopes: string[], but lines 172 and 203 cast to{ key: string; value: string; isSecret: boolean }[], dropping thescopesfield. The downstreamproxy.tsexpects the full type withscopes. Verify whether@netlify/build'sstartDevandbuildSiteactually returnscopes, then either remove the casts or update the overload signatures accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/run-build.ts` around lines 192 - 205, The deployEnvironment returned from startDev / buildSite is being cast to { key: string; value: string; isSecret: boolean }[] (see deployEnvironment usage in run-build.ts) which drops the scopes field declared in the overloads; inspect the actual return shape of `@netlify/build`'s startDev and buildSite (confirm whether they include scopes:string[]), then either remove the unsafe casts in run-build.ts (so the value retains scopes) or update the overload signatures to match the true return type; ensure any downstream consumer (e.g., proxy.ts) receives the full type including scopes and update the type assertions or interfaces accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/run-build.ts`:
- Around line 170-174: The return value of the build path violates the overload
signature by returning deployEnvironment items without the required scopes
field; update the returned object in runBuild (the block returning configPath:
tempConfigPath, deployEnvironment: [] as { key: string; value: string; isSecret:
boolean }[], generatedFunctions) so its deployEnvironment is typed to include
scopes (e.g. { key: string; value: string; isSecret: boolean; scopes: string[]
}[]) or construct the empty array with the correct shape (empty items with
scopes: string[]), ensuring the return matches the declared overload type for
deployEnvironment.
In `@tests/integration/commands/dev/dev.test.ts`:
- Around line 837-931: The test hard-codes port 4445 which causes collisions
under describe.concurrent; change dev.targetPort to 0 in withNetlifyToml and
make the generated dev server (index.mjs created by withContentFile) listen on
the assigned port from the environment (e.g., server.listen(process.env.PORT) or
process.env.PORT || 0) so the test harness can bind a random free port; keep the
fetches using server.url (withDevServer) unchanged. Update references in the
diff: modify the dev.targetPort in withNetlifyToml and replace
server.listen(4445) in the index.mjs content so the runtime uses
process.env.PORT (or equivalent) provided by the test harness.
---
Outside diff comments:
In `@src/lib/edge-functions/registry.ts`:
- Around line 368-388: The merge currently lets deployEnvironment values
overwrite env entries from higher‑precedence sources; update
getEnvironmentVariables so when iterating deployEnvironment (the array of { key,
value, isSecret }), only assign env[key] = value if the key is not already
present (e.g., check env[key] === undefined or !(key in env)) to preserve higher
precedence values from envConfig (.env, ui, account, addons, internal) and avoid
clobbering them.
In `@src/lib/functions/netlify-function.ts`:
- Around line 272-289: The current environment construction allows
this.deployEnvironment entries to overwrite higher-precedence variables; update
the build in src/lib/functions/netlify-function.ts so deployEnvironment does not
clobber existing keys by either (a) filtering this.deployEnvironment to only
include entries whose keys are not already present in process.env or
configEnvVars, or (b) changing the spread order so higher-precedence sources
(process.env and configEnvVars) come after deployEnvironment; locate the
relevant symbols configEnvVars, this.deployEnvironment and the environment
object and implement the filter or adjust the spread order to preserve the
precedence: Process env > .env/site/addon > build config.
In `@src/utils/run-build.ts`:
- Around line 192-205: The deployEnvironment returned from startDev / buildSite
is being cast to { key: string; value: string; isSecret: boolean }[] (see
deployEnvironment usage in run-build.ts) which drops the scopes field declared
in the overloads; inspect the actual return shape of `@netlify/build`'s startDev
and buildSite (confirm whether they include scopes:string[]), then either remove
the unsafe casts in run-build.ts (so the value retains scopes) or update the
overload signatures to match the true return type; ensure any downstream
consumer (e.g., proxy.ts) receives the full type including scopes and update the
type assertions or interfaces accordingly.
netlify dev8e467a6 to
d0ef90d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/conventional-commit.yml:
- Around line 19-20: Remove the dead "Install Dependencies" step that runs `npm
install `@commitlint/config-conventional`` from the workflow; the
`amannn/action-semantic-pull-request` action uses its own `with:` inputs (like
`types` and `scopes`) so the commitlint package installation is
unnecessary—delete the step named "Install Dependencies" (the run: npm install
`@commitlint/config-conventional`) and keep the existing
`amannn/action-semantic-pull-request` configuration intact.
d0ef90d to
4f3cde9
Compare
No description provided.