[DO NOT MERGE]: eng 2476 make workspacek8snamespace non nullable part 2#4993
[DO NOT MERGE]: eng 2476 make workspacek8snamespace non nullable part 2#4993
Conversation
this is step 1, next is to backfill all workspaces and then we can change the db schema to make it non-nullable
…rkspacek8snamespace-non-nullable-part-2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request makes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
chronark
changed the base branch from
main
to
eng-2476-make-workspacek8snamespace-non-nullable
chronark
changed the title
chronark
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/db/workspace_insert.sql_generated.go (1)
8-11:⚠️ Potential issue | 🔴 CriticalUnused import
database/sqlcauses build failure.Pipeline reports this import is unused after removing
sql.NullString. Since this is sqlc-generated code, regenerate withsqlc generateto fix.svc/ctrl/worker/deploy/deploy_handler.go (1)
98-103:⚠️ Potential issue | 🟡 MinorEmpty-string won’t persist
SetWorkspaceK8sNamespaceonly updates NULL; if k8s_namespace is"", update no-ops and ws uses a value not stored. Align predicate or handle''in the update.
🤖 Fix all issues with AI agents
In `@web/tools/migrate/backfill_k8s_namespace.ts`:
- Around line 27-33: Wrap the top-level main() invocation in a try-catch and
handle errors: call main() inside an async IIFE or Promise chain and catch any
thrown errors, log them (using console.error or existing logger) and exit with
non-zero status; ensure the catch reports the error from DB operations such as
the db.update on schema.workspaces (and any dns1035 collisions) so failures in
the loop updating k8sNamespace are surfaced and cause process exit instead of
being unhandled.
🧹 Nitpick comments (1)
web/tools/migrate/backfill_k8s_namespace.ts (1)
44-44: Use fault for script errors
main()runs without fault-based handling; failures can surface as unhandled rejections. Wrap per repo fault pattern.As per coding guidelines: Use
faultlibrary for error handling.
| for (const workspace of workspaces) { | ||
| const namespace = dns1035(); | ||
|
|
||
| await db | ||
| .update(schema.workspaces) | ||
| .set({ k8sNamespace: namespace }) | ||
| .where(eq(schema.workspaces.id, workspace.id)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/tools/migrate/backfill_k8s_namespace.tsRepository: unkeyed/unkey
Length of output: 1587
🏁 Script executed:
# Find dns1035 function definition
rg "export.*dns1035|function dns1035" -A 5 --type tsRepository: unkeyed/unkey
Length of output: 462
🏁 Script executed:
# Search for schema.workspaces definition
fd "schema" --type ts | head -20Repository: unkeyed/unkey
Length of output: 226
🏁 Script executed:
# Look for workspaces schema and k8sNamespace field
rg "k8sNamespace" --type ts -B 2 -A 2Repository: unkeyed/unkey
Length of output: 3843
🏁 Script executed:
# Check for fault library usage in migration files
rg "from.*fault|import.*fault" web/tools/migrate --type-add "ts:*.ts" -A 2Repository: unkeyed/unkey
Length of output: 39
🏁 Script executed:
# Look for error handling patterns in other migrations
fd backfill web/tools/migrate --type fRepository: unkeyed/unkey
Length of output: 100
🏁 Script executed:
# Check for retry logic patterns in the codebase
rg "retry|catch.*Error" web/tools/migrate --type-add "ts:*.ts" -A 2 -B 1Repository: unkeyed/unkey
Length of output: 39
🏁 Script executed:
# Check for fault library usage patterns in the broader codebase
rg "from.*fault|import.*fault" web/ --max-count 5 -lRepository: unkeyed/unkey
Length of output: 2901
🏁 Script executed:
# Look at error handling in database operations
rg "\.catch\(|try \{" web/tools/migrate -A 3 -B 1Repository: unkeyed/unkey
Length of output: 1494
🏁 Script executed:
# Check dns1035 collision probability - look at how many values it can generate
cat web/internal/id/src/generate.ts | head -30Repository: unkeyed/unkey
Length of output: 732
Add error handling for main() invocation
The main() function call on line 44 lacks error handling. Add a try-catch block or implement proper error handling per TypeScript guidelines, as shown in other migration scripts (ch_copy.ts, ch_logs.ts).
While theoretically a unique constraint collision on k8sNamespace could occur with dns1035(), the probability is negligible (57-character alphabet, 12+ character names = millions of combinations). The primary concern is unhandled database operation errors.
🤖 Prompt for AI Agents
In `@web/tools/migrate/backfill_k8s_namespace.ts` around lines 27 - 33, Wrap the
top-level main() invocation in a try-catch and handle errors: call main() inside
an async IIFE or Promise chain and catch any thrown errors, log them (using
console.error or existing logger) and exit with non-zero status; ensure the
catch reports the error from DB operations such as the db.update on
schema.workspaces (and any dns1035 collisions) so failures in the loop updating
k8sNamespace are surfaced and cause process exit instead of being unhandled.
2 participants
we need to run seeding and migrations for this PR