Sync env vars (parent env vars) #2120
Conversation
|
WalkthroughThis set of changes introduces support for handling "parent environment" variables across the application's environment variable management, build, and deployment systems. The changes extend environment variable schemas, types, and APIs to allow an optional 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (25)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.import.ts (1)
50-74: Consider clarifying partial success behavior.The current logic returns success if either the child or parent environment variable import succeeds, even if the other fails. This could lead to confusion as users might expect both operations to succeed.
Consider:
- Adding logging to indicate partial success scenarios
- Returning more detailed status information about which operations succeeded/failed
- Documenting the expected behavior when only one environment import succeeds
Example enhancement:
- if (result.success || parentResult.success) { - return json({ success: true }); + if (result.success && parentResult.success) { + return json({ success: true }); + } else if (result.success || parentResult.success) { + return json({ + success: true, + partial: true, + childSuccess: result.success, + parentSuccess: parentResult.success + }); } else {packages/cli-v3/src/build/extensions.ts (1)
163-181: Refactor to reduce code duplication.The new
parentEnvhandling logic correctly follows the same pattern as the existingenvvariable logic. However, there's code duplication in the initialization logic (lines 164-166) that already exists above (lines 143-146).Consider consolidating the initialization logic:
if (layer.deploy?.env) { $manifest.deploy.env ??= {}; $manifest.deploy.sync ??= {}; $manifest.deploy.sync.env ??= {}; $manifest.deploy.sync.parentEnv ??= {}; for (const [key, value] of Object.entries(layer.deploy.env)) { // ... existing logic } } + if (layer.deploy?.env || layer.deploy?.parentEnv) { + $manifest.deploy.env ??= {}; + $manifest.deploy.sync ??= {}; + $manifest.deploy.sync.env ??= {}; + $manifest.deploy.sync.parentEnv ??= {}; + } + + if (layer.deploy?.env) { + for (const [key, value] of Object.entries(layer.deploy.env)) { + // ... existing logic + } + } if (layer.deploy?.parentEnv) { - $manifest.deploy.env ??= {}; - $manifest.deploy.sync ??= {}; - $manifest.deploy.sync.parentEnv ??= {}; for (const [key, value] of Object.entries(layer.deploy.parentEnv)) { // ... rest of logic } }apps/webapp/app/services/upsertBranch.server.ts (1)
149-175: LGTM! Improved branch limit logic with performance consideration.The updated logic correctly excludes the current branch being created/updated from the limit count, preventing false limit violations. The change from a
count()query tofindMany()with filtering is necessary to implement this logic.For large projects with many branches, consider the performance impact of fetching all branch records vs. using a more targeted query:
const usedEnvs = await prisma.runtimeEnvironment.findMany({ where: { projectId, branchName: { not: null, + ...(newBranchName && { not: newBranchName }), }, archivedAt: null, }, + select: { + id: true, + branchName: true, + }, }); - const count = newBranchName - ? usedEnvs.filter((env) => env.branchName !== newBranchName).length - : usedEnvs.length; + const count = usedEnvs.length;This approach reduces memory usage and potentially improves query performance by filtering at the database level and selecting only necessary fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/hello-world/trigger.config.tsis excluded by!references/**
📒 Files selected for processing (14)
apps/webapp/app/presenters/v3/ScheduleListPresenter.server.ts(1 hunks)apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.import.ts(1 hunks)apps/webapp/app/services/upsertBranch.server.ts(3 hunks)apps/webapp/app/v3/services/checkSchedule.server.ts(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(1 hunks)apps/webapp/app/v3/services/createDeploymentBackgroundWorker.server.ts(1 hunks)packages/build/src/extensions/core/syncEnvVars.ts(4 hunks)packages/build/src/extensions/core/vercelSyncEnvVars.ts(3 hunks)packages/cli-v3/src/build/extensions.ts(2 hunks)packages/cli-v3/src/commands/deploy.ts(6 hunks)packages/cli-v3/src/commands/workers/build.ts(2 hunks)packages/core/src/v3/build/extensions.ts(1 hunks)packages/core/src/v3/schemas/api.ts(1 hunks)packages/core/src/v3/schemas/build.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/webapp/app/v3/services/createDeploymentBackgroundWorker.server.ts (1)
internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-67)
apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-67)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (22)
packages/core/src/v3/schemas/api.ts (1)
808-808: LGTM! Well-designed schema extension.The addition of the optional
parentVariablesfield follows the same type pattern as the existingvariablesfield and maintains backward compatibility. This properly supports the parent environment variables feature without introducing breaking changes.apps/webapp/app/presenters/v3/ScheduleListPresenter.server.ts (1)
72-72: LGTM! Logical field addition for archived environment handling.Adding the
archivedAtfield to the environment selection is consistent with the existing pattern and enables proper filtering of archived environments in schedule counting logic.packages/core/src/v3/build/extensions.ts (1)
65-65: LGTM! Consistent interface extension.The addition of the optional
parentEnvfield follows the exact same type pattern as the existingenvfield, maintaining consistency and backward compatibility while supporting the parent environment variables feature.apps/webapp/app/v3/services/createDeploymentBackgroundWorker.server.ts (1)
117-120: LGTM! Excellent error handling improvement.This change properly preserves original
ServiceValidationErrorinstances while maintaining the fallback behavior for other error types. This prevents loss of specific error details from upstream services and follows error handling best practices.packages/core/src/v3/schemas/build.ts (1)
55-55: LGTM! Schema extension follows existing patterns.The addition of the optional
parentEnvproperty follows the same structure and typing as the existingenvproperty, maintaining consistency and backward compatibility.apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
150-152: Excellent error handling improvement!This change preserves the original
ServiceValidationErrordetails when re-throwing, preventing the loss of specific error information that would occur if the error were wrapped in a generic message.apps/webapp/app/v3/services/checkSchedule.server.ts (1)
111-115: LGTM! Proper filtering of archived environments.The updated filtering logic correctly excludes both development and archived environments from schedule counting, ensuring that only active, deployed environments contribute to schedule limits. The type signature update properly reflects the required
archivedAtfield.packages/build/src/extensions/core/vercelSyncEnvVars.ts (3)
4-4: LGTM! Clean type definition.The
EnvVartype with the optionalisParentEnvflag is well-defined and supports the new parent environment variable functionality.
78-91: LGTM! Sound logic for parent environment detection.The logic for determining parent environment variables is well-implemented:
isBranchablecorrectly identifies preview environments- Parent env vars are those without a
gitBranchproperty in branchable environments- The mapping preserves all original properties while adding the new flag
62-62:✅ Verification successful
Verify the URL parameter simplification doesn't break existing functionality.
The URL construction was simplified by removing conditional
gitBranchparameter handling and adding a fixedtargetparameter. Ensure this change is intentional and doesn't break existing Vercel API integrations.
🏁 Script executed:
#!/bin/bash # Search for other usages of gitBranch parameter in Vercel API calls rg -A 5 -B 5 "gitBranch.*vercel|vercel.*gitBranch"Length of output: 52
🏁 Script executed:
#!/bin/bash # Locate the vercelSyncEnvVars file and print context around line 62 file=$(fd vercelSyncEnvVars.ts -t f | head -n1) echo "Inspecting $file (lines 50–80):" sed -n '50,80p' "$file" # Search for any usage of a "branch" parameter in Vercel API calls rg -n 'params\.(set|append)\("branch"' -A3 -B3Length of output: 1360
No ‘gitBranch’ usages found—URL simplification is safe
I searched the entire repo for anybranchorgitBranchquery parameters in Vercel API calls and found no matches. The only supported filters on/v8/projects/:id/envaretargetand (optionally)teamId, so replacing conditional branch logic with a fixedparams.set("target", …)won’t break any existing integrations.packages/cli-v3/src/commands/workers/build.ts (2)
284-286: LGTM! Clean integration of parent environment variables.The function call correctly passes both regular and parent environment variables to the sync function, maintaining the existing pattern while extending functionality.
460-466: LGTM! Backward-compatible function signature update.The
syncEnvVarsWithServerfunction signature is properly updated to accept optionalparentEnvVars, maintaining backward compatibility while extending functionality to support parent environment variable synchronization.apps/webapp/app/services/upsertBranch.server.ts (1)
79-81: LGTM! Proper parameter passing for improved branch limit logic.The updated call correctly passes the sanitized branch name to enable more accurate limit checking that excludes the current branch being created/updated.
packages/build/src/extensions/core/syncEnvVars.ts (4)
3-5: LGTM! Clean type extension for parent environment support.The addition of the optional
isParentEnvflag to the array format provides a clean way to distinguish between regular and parent environment variables.
101-131: Well-structured implementation of parent environment variable support.The changes correctly handle the separation and filtering of regular and parent environment variables. The debug logging at line 117 will be helpful for troubleshooting deployment issues.
135-152: Good refactoring: Extracted filtering logic improves maintainability.Extracting the environment variable filtering logic into a dedicated function improves code organization and makes it reusable for both
envandparentEnv.
160-206: Correct implementation of parent environment variable separation.The function properly handles both array and object return formats from the sync function, correctly separating variables based on the
isParentEnvflag while maintaining backward compatibility.packages/cli-v3/src/commands/deploy.ts (5)
354-397: Comprehensive parent environment variable syncing implementation.The implementation correctly:
- Checks for variables in both
envandparentEnvsets- Safely extracts variables with fallback to empty objects
- Accurately counts the total number of variables
- Passes both sets to the sync function
The logic handles all edge cases properly.
510-526: Improved error handling with detailed deployment error information.Good enhancement that extracts and uses detailed error data from the deployment response when available, providing more informative error messages to users while maintaining a fallback for cases where error data is unavailable.
622-638: Clean extension of sync function to support parent environment variables.The function signature maintains backward compatibility with the optional
parentEnvVarsparameter, and the implementation correctly passes parent variables to the API. The debug logging will help with troubleshooting sync issues.
649-649: Helpful debug logging for deployment failures.The debug logging will assist in troubleshooting deployment failures by capturing the full context.
710-710: Important fix: Display error message instead of error name.Good catch! This ensures users see the actual error message content rather than just the error type name, providing much more useful information.
syncEnvVars (and syncVercelEnvVars) would only sync the environment, not the parent environment's variables.
This is relevant for syncing preview env vars, where you want to set the "Preview" env vars AND the override for the specific environment (if there is an override).