-
-
Notifications
You must be signed in to change notification settings - Fork 591
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
Fix: vercelSyncEnvVars teamId #1463
Conversation
🦋 Changeset detectedLatest commit: 2b2ed57 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes in this pull request introduce enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
.changeset/rotten-dolphins-punch.md (1)
1-5
: Enhance the changeset description with more details.While the version bump is appropriate for the new feature, the description could be more comprehensive to help users understand the changes and requirements better.
Consider expanding the description like this:
--- "@trigger.dev/build": minor --- -Add teamId option to vercelSyncEnvVars +Add teamId support to vercelSyncEnvVars + +- Add `teamId` option to support Vercel team projects +- Rename function from `syncVercelEnvVars` to `vercelSyncEnvVars` +- Requires `VERCEL_TEAM_ID` environment variable for team projectsdocs/guides/examples/vercel-sync-env-vars.mdx (1)
16-17
: LGTM! Clear documentation of the new team project requirement.The addition of the VERCEL_TEAM_ID requirement for team projects is well-documented and placed appropriately within the existing note block.
Consider adding a link to Vercel's documentation on how to find the team ID, similar to how you've linked to the access token page. This would make it easier for users to locate this information.
- build extension. If you're working with a team project, you'll also need to set `VERCEL_TEAM_ID`. + build extension. If you're working with a team project, you'll also need to set `VERCEL_TEAM_ID` + (found in your [team settings](https://vercel.com/teams/select)).packages/build/src/extensions/core/vercelSyncEnvVars.ts (2)
13-14
: Consider adding validation for vercelTeamIdWhile the fallback chain is well-implemented, consider adding validation for the teamId format when present, similar to how projectId and vercelAccessToken are validated. This would help catch configuration issues early.
const vercelTeamId = options?.vercelTeamId ?? process.env.VERCEL_TEAM_ID ?? ctx.env.VERCEL_TEAM_ID; + if (vercelTeamId && !/^team_[a-zA-Z0-9]+$/.test(vercelTeamId)) { + console.warn("vercelSyncEnvVars: The provided teamId may be invalid. Expected format: team_*"); + }
42-44
: Consider making the Vercel API base URL configurableThe URL construction using URLSearchParams is well-implemented. However, consider making the base URL configurable for testing purposes or future API version changes.
+ const VERCEL_API_BASE = options?.apiBaseUrl ?? "https://api.vercel.com/v8"; const params = new URLSearchParams({ decrypt: "true" }); if (vercelTeamId) params.set("teamId", vercelTeamId); - const vercelApiUrl = `https://api.vercel.com/v8/projects/${projectId}/env?${params}`; + const vercelApiUrl = `${VERCEL_API_BASE}/projects/${projectId}/env?${params}`;Update the options type to include:
type Options = { projectId?: string; vercelAccessToken?: string; vercelTeamId?: string; + apiBaseUrl?: string; }docs/guides/frameworks/nextjs.mdx (1)
254-257
: LGTM with a minor suggestion for clarity.The addition of the
VERCEL_TEAM_ID
requirement for team projects is clear and well-documented. The information is correctly placed within the environment variables section.Consider adding a link to Vercel's documentation about finding the team ID, similar to how you've linked to the access token page. This would make it easier for users to locate their team ID. Here's a suggested revision:
working with a team project, you'll also need to set `VERCEL_TEAM_ID`. You can find / generate - the `VERCEL_ACCESS_TOKEN` in your Vercel [dashboard](https://vercel.com/account/settings/tokens). + the `VERCEL_ACCESS_TOKEN` in your Vercel [dashboard](https://vercel.com/account/settings/tokens) and + find your team ID in the [team settings](https://vercel.com/teams/select). Make sure the scope of the token covers the project you want to sync.docs/config/config-file.mdx (1)
514-517
: Documentation looks good but could be more detailed.The addition of
VERCEL_TEAM_ID
documentation is clear and well-placed. However, consider these enhancements for better clarity:
- Explicitly state whether
VERCEL_TEAM_ID
is mandatory for team projects- Add an example showing how to pass
teamId
explicitly as an argument- Include a link to the Vercel team settings page for finding the team ID
Consider updating the text to include an example:
Make sure the scope of the token covers the project you want to sync. + + For team projects, you can either set the `VERCEL_TEAM_ID` environment variable or pass it explicitly: + + ```ts + vercelSyncEnvVars({ + teamId: "team_yourTeamId" // Optional: Override VERCEL_TEAM_ID environment variable + }) + ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.changeset/rotten-dolphins-punch.md
(1 hunks)docs/config/config-file.mdx
(1 hunks)docs/guides/examples/vercel-sync-env-vars.mdx
(1 hunks)docs/guides/frameworks/nextjs.mdx
(1 hunks)packages/build/src/extensions/core/vercelSyncEnvVars.ts
(2 hunks)
🔇 Additional comments (2)
packages/build/src/extensions/core/vercelSyncEnvVars.ts (2)
Line range hint 4-44
: Verify error handling in team context
The implementation looks solid overall. However, let's ensure proper error handling when using team-specific endpoints.
#!/bin/bash
# Search for any existing error handling patterns with team contexts
rg "teamId.*error|error.*teamId" --type ts
# Look for any existing tests involving team scenarios
rg "test.*team|describe.*team" --type test
4-5
: Function signature change looks good!
The rename from syncVercelEnvVars
to vercelSyncEnvVars
and the addition of the optional vercelTeamId
parameter are well-implemented.
Let's verify if there are any existing imports of the old function name that need updating:
✅ Verification successful
Function rename is safe to proceed
The function rename from syncVercelEnvVars
to vercelSyncEnvVars
is well-implemented. The only references found are:
- The function definition file itself:
packages/build/src/extensions/core/vercelSyncEnvVars.ts
- A barrel file that re-exports it:
packages/build/src/extensions/core.ts
The barrel file is already using the new name in its export statement, indicating the rename was properly implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old function name
rg "syncVercelEnvVars" --type ts
Length of output: 118
Script:
#!/bin/bash
# Search for any references to the function name (both old and new) in TypeScript files
rg "vercelSyncEnvVars|syncVercelEnvVars" --type ts -l
# Search for import statements containing either function name
ast-grep --pattern 'import { $_ } from $_'
Length of output: 194
Script:
#!/bin/bash
# Let's check the content of core.ts to see how the function is used
cat packages/build/src/extensions/core.ts
Length of output: 284
@trigger.dev/build
@trigger.dev/react-hooks
trigger.dev
@trigger.dev/sdk
@trigger.dev/core
commit: |
Closes https://discord.com/channels/1066956501299777596/1303601937282895943/1304512833056411759
✅ Checklist
Testing
Changelog
Screenshots
[Screenshots]
💯
Summary by CodeRabbit
New Features
teamId
option forvercelSyncEnvVars
, enhancing environment variable management for team projects.Documentation
VERCEL_TEAM_ID
for syncing environment variables in team projects.