feat: Add variable change workers#453
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces two new worker functions in the event worker system: one for updating deployment variables and another for updating resource variables. Changes include registering these new workers in the main workers object, updating API routers to enqueue variable change events, and extending event channel definitions and type mappings. The modifications streamline the logic for handling variable updates by simplifying database queries and error handling while integrating a queue-based approach for processing updates. Changes
Sequence Diagram(s)sequenceDiagram
participant API as API Router
participant Queue as updateDeploymentVariableQueue
participant Worker as updateDeploymentVariableWorker
participant DB as Database
participant EvalQ as evaluateReleaseTargetQueue
API->>Queue: Enqueue deployment variable update job
Queue->>Worker: Dispatch job
Worker->>DB: Query variable by ID
DB-->>Worker: Return deployment variable or error
alt Variable Found
Worker->>DB: Query release targets for deployment
DB-->>Worker: Return release targets
Worker->>EvalQ: Add release targets for evaluation
else Variable Not Found
Worker-->>Worker: Throw error
end
sequenceDiagram
participant API as API Router
participant Queue as updateResourceVariableQueue
participant Worker as updateResourceVariableWorker
participant DB as Database
participant EvalQ as evaluateReleaseTargetQueue
API->>Queue: Enqueue resource variable update job
Queue->>Worker: Dispatch job
Worker->>DB: Query deployment IDs via joins using resourceId & key
DB-->>Worker: Return deployment IDs
Worker->>DB: Query release targets using deployment IDs
DB-->>Worker: Return matching release targets
Worker->>EvalQ: Add release targets for processing
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/event-worker/src/workers/update-resource-variable.ts (1)
41-46: Optional safeguard against empty releases.If
releaseTargetsis empty, calling.addBulk([])will effectively enqueue no jobs. While this is not harmful, you may consider short-circuiting to skip unnecessary queue interactions when the array is empty.if (releaseTargets.length > 0) { await getQueue(Channel.EvaluateReleaseTarget).addBulk( releaseTargets.map((rt) => ({ name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, data: rt, })), ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/update-deployment-variable.ts(1 hunks)apps/event-worker/src/workers/update-resource-variable.ts(1 hunks)packages/api/src/router/deployment-variable.ts(9 hunks)packages/api/src/router/resource-variables.ts(4 hunks)packages/events/src/types.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/event-worker/src/workers/update-deployment-variable.tsapps/event-worker/src/workers/index.tspackages/events/src/types.tsapps/event-worker/src/workers/update-resource-variable.tspackages/api/src/router/resource-variables.tspackages/api/src/router/deployment-variable.ts
🧬 Code Definitions (1)
apps/event-worker/src/workers/index.ts (2)
apps/event-worker/src/workers/update-deployment-variable.ts (1)
updateDeploymentVariableWorker(14-35)apps/event-worker/src/workers/update-resource-variable.ts (1)
updateResourceVariableWorker(6-48)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (10)
apps/event-worker/src/workers/index.ts (1)
11-12: Worker registration looks good!The new workers for handling deployment and resource variable updates are properly imported and registered in the workers object.
Also applies to: 23-24
packages/events/src/types.ts (2)
20-22: Channel enum values added appropriately.The new enum values for variable updates are well-placed and follow the existing naming conventions.
35-37: ChannelMap type extensions look good.The type mappings correctly associate the new channels with their respective schema types.
apps/event-worker/src/workers/update-deployment-variable.ts (2)
6-13: Good documentation!The comments clearly explain the worker's purpose and assumptions, which helps with maintainability.
14-35: Worker implementation looks correct.The worker follows the established pattern using createWorker, has appropriate error handling for missing variables, and properly queues release targets for evaluation.
packages/api/src/router/resource-variables.ts (4)
5-5: Queue initialization looks good.Proper imports and initialization of the resource variable update queue.
Also applies to: 11-11
28-35: Create mutation update looks good.The implementation correctly awaits the database operation, captures the result, adds it to the queue, and returns the variable.
67-71: Update mutation change looks good.The chained promise approach correctly captures the updated variable, queues it for processing, and returns it.
93-99: Delete mutation update looks good.The implementation now includes returning the deleted record for queuing and properly notifies the worker system about the deletion.
packages/api/src/router/deployment-variable.ts (1)
27-34: Queue initialization looks good.Using
Channel.UpdateDeploymentVariableviagetQueue()is aligned with the new worker-based architecture. The approach is clean and maintains consistency with the rest of the codebase.
| const variable = await tx | ||
| .select() | ||
| .from(deploymentVariable) | ||
| .where(eq(deploymentVariable.id, input.variableId)) | ||
| .then(takeFirst); | ||
| await updateDeploymentVariableQueue.add(variable.id, variable); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Streamline repeated enqueue logic for better maintainability.
Across create, update, and delete operations, the code repeatedly fetches a variable and enqueues it with identical statements (updateDeploymentVariableQueue.add(variable.id, variable)). Consider creating a shared helper function, such as enqueueVariableUpdate(variable), to reduce duplication. This also ensures consistent error handling or logging for queue interactions.
Additionally, note that after deleting a variable (lines 358-368), the worker might receive a “null” or “missing” entry. Confirm whether the worker can gracefully handle or ignore events for non-existent data.
- await updateDeploymentVariableQueue.add(variable.id, variable);
// Proposed helper usage
+ async function enqueueVariableUpdate(variable: DeploymentVariable) {
+ await updateDeploymentVariableQueue.add(variable.id, variable);
+ }
// Then in create, update, delete:
+ await enqueueVariableUpdate(variable);Also applies to: 96-96, 136-136, 160-172, 336-346, 358-368
Please run the above script to confirm that the enqueue logic is indeed duplicated across various operations (e.g., in create, update, and delete flows). Once verified, consider refactoring by extracting a helper function (like enqueueVariableUpdate(variable)). Also, double-check that the worker processing these queue events can gracefully handle cases where a variable is null or missing, especially in the deletion scenario.
Action: Refactor Repeated Enqueue Logic for Deployment Variable Updates
It appears that the enqueue operation (updateDeploymentVariableQueue.add(variable.id, variable)) is repeated identically across multiple code sections. To improve maintainability and ensure consistent error handling/logging:
- Extract a helper function: Create a dedicated function (e.g.,
enqueueVariableUpdate(variable)) to encapsulate the enqueue logic. - Verify worker safety: Confirm that the worker processing the update queue can gracefully ignore or handle events when the corresponding variable is null/missing (notably after deletions).
Proposed refactor diff:
- await updateDeploymentVariableQueue.add(variable.id, variable);
+ async function enqueueVariableUpdate(variable: DeploymentVariable) {
+ await updateDeploymentVariableQueue.add(variable.id, variable);
+ }
+
+ // In create, update, and delete operations, replace with:
+ await enqueueVariableUpdate(variable);This change is relevant in the following areas:
- Lines 64-69 (current snippet)
- Lines 96-96, 136-136, 160-172, 336-346, and 358-368
Please verify the duplicated enqueue point using the above script and review the worker’s logic regarding null or missing variables.
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of updateDeploymentVariableQueue.add in the repository to verify repeated enqueue usage.
rg "updateDeploymentVariableQueue.add\(" -n .Length of output: 798
Refactor Enqueue Logic in Deployment Variable Router
The enqueue operation (updateDeploymentVariableQueue.add(variable.id, variable)) has been confirmed to be duplicated in multiple locations within the file (found at lines 69, 136, 172, 324, 344, and 365 in packages/api/src/router/deployment-variable.ts). To improve maintainability and ensure consistent error handling/logging, please extract a helper function (e.g., enqueueVariableUpdate(variable)) to encapsulate this repeated logic.
Additionally, ensure that the worker processing the update queue can gracefully handle cases where the variable is null or missing (for instance, after a deletion). This may involve adding checks before processing the queued event or updating the worker’s error handling to ignore or safely log such events.
Proposed refactor diff:
- await updateDeploymentVariableQueue.add(variable.id, variable);
+ async function enqueueVariableUpdate(variable: DeploymentVariable) {
+ await updateDeploymentVariableQueue.add(variable.id, variable);
+ }
+
+ // Then in create, update, and delete operations:
+ await enqueueVariableUpdate(variable);Action Items:
- Refactor the duplicate enqueue calls at lines 69, 136, 172, 324, 344, and 365.
- Review the worker handling the update queue and ensure it safely manages null/missing variable entries.
Summary by CodeRabbit
New Features
Refactor