feat: create exit hooks from deployment endpoint #577
Conversation
WalkthroughThis change introduces support for "exit hooks" in deployment creation and update workflows. It adds an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant UpsertExitHook
Client->>API: POST/PATCH /v1/deployments (with exitHooks)
API->>DB: Create/Update deployment (transaction)
alt exitHooks present
API->>UpsertExitHook: For each exitHook in exitHooks
UpsertExitHook->>DB: Upsert hook, runbook, runhook, variables (transactional)
end
API-->>Client: Response
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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 (5)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
openapi.v1.json (2)
679-684: MakeexitHooksnullable ifnullshould be allowed
To explicitly allownullvalues for theexitHooksarray—aligning with other nullable properties in the schema—add"nullable": trueto this property.
4926-4954: EnhanceExitHookschema clarity
Consider adding an object-level"description"and setting"additionalProperties": falsefor theExitHookschema to clarify the contract and prevent unexpected fields.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts(1 hunks)apps/webservice/src/app/api/v1/deployments/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/deployments/route.ts(4 hunks)openapi.v1.json(3 hunks)packages/db/drizzle/0107_small_deathbird.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment.ts(1 hunks)packages/db/src/schema/event.ts(1 hunks)packages/db/src/schema/resource-deployment-rule.ts(1 hunks)packages/db/src/schema/runbook.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.tsapps/webservice/src/app/api/v1/deployments/route.tspackages/db/src/schema/resource-deployment-rule.tspackages/db/src/schema/event.tsapps/webservice/src/app/api/v1/deployments/[deploymentId]/openapi.tspackages/db/src/schema/deployment.tsapps/webservice/src/app/api/v1/deployments/openapi.tspackages/db/src/schema/runbook.tsapps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts
🧬 Code Graph Analysis (4)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts (2)
apps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts (1)
upsertExitHook(80-90)packages/db/src/client.ts (1)
db(15-15)
apps/webservice/src/app/api/v1/deployments/route.ts (1)
apps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts (1)
upsertExitHook(80-90)
packages/db/src/schema/runbook.ts (1)
packages/db/src/schema/job-agent.ts (1)
jobAgent(10-22)
apps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts (4)
packages/db/src/common.ts (3)
Tx(22-22)buildConflictUpdateColumns(30-46)takeFirst(9-13)packages/db/src/schema/deployment.ts (2)
deployment(69-93)Deployment(117-117)packages/db/src/schema/event.ts (1)
hook(30-46)packages/db/src/schema/runbook.ts (1)
runbook(20-38)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (17)
packages/db/drizzle/meta/_journal.json (1)
754-760: LGTM! Standard migration journal entry.The new journal entry follows the correct Drizzle migration format and properly tracks the database schema changes for the exit hooks feature.
packages/db/src/schema/deployment.ts (1)
99-111: Well-structured schema extension for exit hooks.The
exitHooksarray addition is properly implemented with:
- Appropriate validation for each exit hook object
- UUID validation for
jobAgentId- Flexible configuration through
z.record(z.any())- Optional field that maintains backward compatibility
The schema design aligns well with the existing deployment structure.
packages/db/src/schema/resource-deployment-rule.ts (1)
1-210:❓ Verification inconclusive
Verify impact of disabling resource deployment rule functionality.
The entire resource deployment rule schema and functionality has been commented out, which could be a breaking change if this code is referenced elsewhere in the codebase.
Please verify that:
- No existing code imports or uses these commented schemas
- This change aligns with the migration to exit hooks functionality
- Any dependent functionality has been properly updated or removed
🏁 Script executed:
#!/bin/bash # Description: Search for usage of resource deployment rule imports and types # Search for imports of this module rg -A 3 "from.*resource-deployment-rule" --type ts # Search for usage of exported types and schemas rg -A 3 "ResourceDeploymentRule|resourceDeploymentRule|ResourceDeploymentRelationshipType" --type ts # Search for any database queries using these tables rg -A 3 "resource_deployment_relationship_rule|resource_deployment_rule_metadata" --type tsLength of output: 8698
No references to resource deployment rule found – please confirm intent
We’ve searched for imports of
resource-deployment-rule, usages ofResourceDeploymentRule,resourceDeploymentRule,ResourceDeploymentRelationshipType, and the table names; nothing outside this file is referencing them. Commenting out this module will not break any existing code.Please confirm that:
- This change is intentional as part of the exit‐hooks migration.
- Any logic, tests, documentation or CI workflows that depended on resource deployment rules have also been updated or removed.
apps/webservice/src/app/api/v1/deployments/route.ts (3)
14-14: LGTM! Proper import for exit hook utility.The import is correctly added for the new exit hook functionality.
39-39: Good practice: Extracting exitHooks early.Extracting the
exitHooksfrom the validated request body early in the function improves readability and reusability.
55-60:❌ Incorrect review comment
Review transaction handling in upsertExitHook.
The exit hook processing logic is correctly implemented with
Promise.allfor concurrent execution. However, verify that theupsertExitHookfunction properly handles nested transactions since it's called within the existing database context.From the relevant code snippets, I can see that
upsertExitHookcreates its own transaction withdb.transaction(). This could lead to nested transactions which may cause issues depending on the database driver.Also applies to: 72-75
🏁 Script executed:
#!/bin/bash # Description: Check how the database driver handles nested transactions # Search for transaction handling patterns in the codebase rg -A 5 -B 5 "db\.transaction|tx\.transaction" --type ts # Look for any existing nested transaction patterns rg -A 10 "transaction.*transaction" --type tsLength of output: 44891
upsertExitHook creates independent transactions
In this route handler,ctx.dbis the root database client (not already inside a transaction), so each call toupsertExitHook(ctx.db, …)invokes a top-leveldb.transaction(). There is no nested transaction in this context, and the existing implementation is correct.Likely an incorrect or invalid review comment.
packages/db/src/schema/event.ts (1)
30-46: LGTM! Well-designed unique constraint for hook data integrity.The unique index on
(name, scopeType, scopeId)properly enforces that hook names must be unique within each scope, which is essential for the exit hooks upsert functionality. This prevents duplicate hooks and ensures data consistency.packages/db/drizzle/0107_small_deathbird.sql (1)
1-2: LGTM! Migration script correctly implements the required unique indexes.The SQL statements properly create the unique indexes that correspond to the schema changes in the TypeScript files. The btree indexes are appropriate for enforcing uniqueness constraints on the specified column combinations.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/openapi.ts (1)
129-132: LGTM! OpenAPI schema correctly documents the exit hooks functionality.The
exitHooksproperty is properly defined as an array referencing theExitHookschema component. This aligns well with the API implementation and provides clear documentation for consumers of the API.apps/webservice/src/app/api/v1/deployments/openapi.ts (1)
11-33: LGTM! Well-defined ExitHook schema with proper structure.The ExitHook schema is well-structured with appropriate types, descriptions, and examples. The required fields make sense for exit hook functionality.
packages/db/src/schema/runbook.ts (2)
3-10: LGTM! Proper import additions for unique index functionality.The added
uniqueIndeximport is correctly included to support the new index definition.
20-38: LGTM! Proper database schema refactoring with unique index.The refactoring from object literal to three-argument
pgTableform is the correct approach for adding indexes in Drizzle. The unique index on(name, systemId)ensures runbook name uniqueness within each system, which is a sensible business constraint for the exit hooks functionality.apps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts (5)
1-11: LGTM! Well-structured imports and type definition.The imports are appropriate and the
ExitHookInserttype clearly defines the expected structure for exit hook data.
13-27: LGTM! Proper hook upsert implementation.The function correctly handles the unique constraint on
(name, scopeType, scopeId)and appropriately updates the action on conflicts.
46-75: LGTM! Well-defined standard variables for exit hooks.The predefined variables (
resourceIdanddeploymentId) are appropriate for exit hook functionality. TheonConflictDoNothingstrategy is correct to preserve existing variable configurations.
77-78: LGTM! Simple and correct hook-runbook association.The function properly handles the relationship between hooks and runbooks with appropriate conflict resolution.
80-90:❌ Incorrect review comment
Verify transaction nesting behavior.
The function accepts a
Txparameter but then creates a new transaction withdb.transaction(). This could cause issues if theTxis already part of an ongoing transaction.Please verify how this function is intended to be used. If it should work within existing transactions, consider using the passed
dbparameter directly. If it needs its own transaction, consider accepting the raw database client instead.
🏁 Script executed:
#!/bin/bash # Description: Check how upsertExitHook is called to understand transaction context # Expected: Find usage patterns to determine if nested transactions are intended rg -A 5 -B 5 "upsertExitHook"Length of output: 5961
🏁 Script executed:
#!/bin/bash echo "=== upsertExitHook file content (first 50 lines) ===" sed -n '1,50p' apps/webservice/src/app/api/v1/deployments/_utils/upsertExitHook.ts || echo "File not found" echo echo "=== Searching for Tx alias definitions ===" rg -n "type Tx" -n . echo echo "=== Searching for Kysely instantiation ===" rg -n "new Kysely" -n . echo echo "=== Searching for ctx.db assignment in middleware ===" rg -n "ctx\\.db" -n apps/webservice/src/app/api/v1Length of output: 6046
🏁 Script executed:
#!/bin/bash echo "=== Inspecting middleware.ts ===" sed -n '1,200p' apps/webservice/src/app/api/v1/middleware.ts || echo "File not found" echo echo "=== Inspecting db common.ts (Tx and transaction) ===" sed -n '1,200p' packages/db/src/common.ts || echo "File not found"Length of output: 3551
No nested‐transaction issue – upsertExitHook always uses the root db
The
upsertExitHookhelper is only ever invoked withctx.db(the top-level client), not atxfrom another transaction. Each call correctly starts its own transaction for atomicity, so there’s no accidental nesting in existing code.Likely an incorrect or invalid review comment.
| const { exitHooks } = body; | ||
| if (exitHooks != null) | ||
| await Promise.all( | ||
| exitHooks.map((eh) => upsertExitHook(db, updatedDeployment, eh)), | ||
| ); |
There was a problem hiding this comment.
Fix transaction handling and improve error handling.
There are several concerns with the exit hooks processing:
-
Nested transaction issue: The
upsertExitHookfunction creates its own transaction (as seen in the relevant code snippet), but it's being called withdbwhich is already a transaction context. This could lead to nested transaction issues. -
Error handling: If any exit hook processing fails, the deployment update has already been committed to the database, potentially leaving the system in an inconsistent state.
-
Transaction isolation: Using
Promise.allfor concurrent processing might not provide proper transaction isolation.
Consider this approach to fix the issues:
- const { exitHooks } = body;
- if (exitHooks != null)
- await Promise.all(
- exitHooks.map((eh) => upsertExitHook(db, updatedDeployment, eh)),
- );
+ const { exitHooks, ...deploymentData } = body;
+
+ const updatedDeployment = await db.transaction(async (tx) => {
+ const deployment = await tx
+ .update(SCHEMA.deployment)
+ .set(deploymentData)
+ .where(eq(SCHEMA.deployment.id, deploymentId))
+ .returning()
+ .then(takeFirst);
+
+ if (exitHooks != null) {
+ for (const exitHook of exitHooks) {
+ await upsertExitHook(tx, deployment, exitHook);
+ }
+ }
+
+ return deployment;
+ });This ensures all operations are within a single transaction and processes exit hooks sequentially to avoid concurrency issues.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/webservice/src/app/api/v1/deployments/[deploymentId]/route.ts around
lines 121 to 125, the current code calls upsertExitHook with a transaction
context that itself starts a transaction, causing nested transactions, and uses
Promise.all which can lead to concurrency and error handling issues. To fix
this, refactor the code so that all exit hook operations run within the same
outer transaction without starting new transactions inside upsertExitHook, and
process the exit hooks sequentially (e.g., with a for loop and await) instead of
concurrently. This ensures proper transaction handling, isolation, and
consistent error management.
| }, | ||
| "exitHooks": { | ||
| "type": "array" | ||
| } |
There was a problem hiding this comment.
Add items definition for exitHooks in create schema
The exitHooks array in the create deployment request schema lacks an "items" definition, so its element type is unconstrained. This will break validation.
Include a reference to the ExitHook schema, for example:
-"exitHooks": {
- "type": "array"
-},
+"exitHooks": {
+ "type": "array",
+ "items": {
+ "$ref": "#/components/schemas/ExitHook"
+ },
+ "nullable": true
+},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, | |
| "exitHooks": { | |
| "type": "array" | |
| } | |
| }, | |
| "exitHooks": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/ExitHook" | |
| }, | |
| "nullable": true | |
| }, |
🤖 Prompt for AI Agents
In openapi.v1.json around lines 1135 to 1138, the "exitHooks" array in the
create deployment request schema is missing an "items" definition, causing its
element type to be unconstrained and breaking validation. Fix this by adding an
"items" property that references the "ExitHook" schema to specify the type of
elements allowed in the array.
Summary by CodeRabbit
New Features
Bug Fixes
Chores