feat: Env selector update worker #449
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new worker that processes environment selector update events. The changes add a dedicated worker implementation to handle resource matching, system retrieval, and the creation of release targets when environment selectors are modified. Additionally, both the API route and environment router now enqueue update jobs when a resource selector changes. A new channel type is added to standardize event handling, and the worker mapping is updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as API Handler (POST /environments)
participant Q as Queue System
participant W as EnvSelectorUpdate Worker
participant DB as Database
C->>API: Send POST request with environment data
API->>DB: Query for existing environment record
DB-->>API: Return existing environment data
API->>API: Compare resource selectors
API->>Q: Enqueue job if selector change detected
Q->>W: Dispatch EnvironmentSelectorUpdate job
W->>DB: Query for system & newly matched resources
DB-->>W: Return system details and resource list
W->>DB: Insert new release targets (if applicable)
W-->>W: Log errors if system is not found
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 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
|
![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
🧹 Nitpick comments (2)
apps/event-worker/src/workers/env-selector-update.ts (2)
14-45: Consider optimizing large-scale resource comparisons.
If the number of resources is large, repeatedly fetching and comparing entire arrays could become expensive. You might consider incremental or more selective approaches (e.g., limiting pagination, using IDs only, leveraging intersections in the database) to avoid potential performance bottlenecks.
47-59: Enhance documentation clarity.
The docstring references “environment data with old and new selectors” but doesn’t explicitly mention theoldSelectorparameter in the@paramsection. Including it helps clarify the difference betweenoldSelectorand theenvironment.resourceSelectorfor future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/workers/env-selector-update.ts(1 hunks)apps/event-worker/src/workers/index.ts(2 hunks)apps/webservice/src/app/api/v1/environments/route.ts(2 hunks)packages/api/src/router/environment.ts(2 hunks)packages/events/src/types.ts(3 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.
packages/events/src/types.tsapps/event-worker/src/workers/index.tsapps/webservice/src/app/api/v1/environments/route.tspackages/api/src/router/environment.tsapps/event-worker/src/workers/env-selector-update.ts
🧬 Code Definitions (2)
apps/event-worker/src/workers/index.ts (1)
apps/event-worker/src/workers/env-selector-update.ts (1)
envSelectorUpdateWorker(60-96)
apps/webservice/src/app/api/v1/environments/route.ts (2)
packages/db/src/schema/environment.ts (1)
environment(57-82)packages/db/src/upsert-env.ts (1)
upsertEnv(34-80)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (11)
apps/event-worker/src/workers/index.ts (2)
6-6: LGTM: Import for the new worker added correctly.The new import for the environment selector update worker is correctly added and follows the naming and file path conventions of other workers in the file.
20-20: LGTM: Worker properly registered for the new channel.The new environment selector update worker is correctly registered in the workers map, mapping it to the
Channel.EnvironmentSelectorUpdateenum value. This ensures the worker will process the appropriate channel events.packages/events/src/types.ts (3)
2-2: LGTM: Import for ResourceCondition type added correctly.The ResourceCondition type is imported from the appropriate module for use in the ChannelMap type definition.
16-16: LGTM: New channel type added to enum.The
EnvironmentSelectorUpdateenum value is correctly added to the Channel enum with an appropriate string identifier that follows the naming pattern of other channels.
31-33: LGTM: ChannelMap type extended correctly for new channel.The ChannelMap type is properly extended to include the data structure for the environment selector update channel. The type definition correctly includes the environment object along with the oldSelector property to track changes.
packages/api/src/router/environment.ts (2)
32-32: LGTM: New imports added correctly.The
ChannelandgetQueueimports are correctly added from the@ctrlplane/eventsmodule for handling environment selector update events.
315-319: LGTM: Environment selector update event properly enqueued.The code correctly enqueues an environment selector update event when a resource selector changes. The job includes the updated environment information and the old selector value, providing all necessary context for the worker to process the changes.
apps/webservice/src/app/api/v1/environments/route.ts (3)
7-9: LGTM: New imports added correctly.The imports are correctly updated to include
takeFirstOrNull,Channel, andgetQueue, which are needed for the new functionality that checks existing environments and enqueues update events.
53-58: LGTM: Query for existing environment added correctly.The code properly queries for an existing environment with the same name to determine if selector changes need to be processed. The use of
takeFirstOrNullis appropriate to handle cases where no environment exists.
64-72: LGTM: Environment selector update event properly enqueued.The code correctly checks if an existing environment exists and if its resource selector has changed. When both conditions are met, it properly enqueues an environment selector update event with the updated environment and the old selector value.
apps/event-worker/src/workers/env-selector-update.ts (1)
1-2: Imports look good.
No issues with these imports; they correctly reference types and maintain clarity.
| export const envSelectorUpdateWorker = createWorker( | ||
| Channel.EnvironmentSelectorUpdate, | ||
| async (job) => { | ||
| const { oldSelector, ...environment } = job.data; | ||
| const system = await db.query.environment | ||
| .findFirst({ | ||
| where: eq(schema.environment.id, environment.id), | ||
| with: { system: { with: { deployments: true } } }, | ||
| }) | ||
| .then((res) => res?.system); | ||
|
|
||
| if (system == null) { | ||
| log.error("System not found", { environmentId: environment.id }); | ||
| return; | ||
| } | ||
|
|
||
| const { workspaceId, deployments } = system; | ||
|
|
||
| const newlyMatchedResources = await getNewlyMatchedResources( | ||
| workspaceId, | ||
| oldSelector, | ||
| environment.resourceSelector, | ||
| ); | ||
|
|
||
| if (newlyMatchedResources.length === 0) return; | ||
|
|
||
| const releaseTargets = deployments.flatMap((deployment) => | ||
| newlyMatchedResources.map((resource) => ({ | ||
| resourceId: resource.id, | ||
| deploymentId: deployment.id, | ||
| environmentId: environment.id, | ||
| })), | ||
| ); | ||
|
|
||
| await db.insert(schema.releaseTarget).values(releaseTargets); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify release target uniqueness.
If the system receives rapid consecutive updates with overlapping resource matches, there’s a risk of duplicate release targets unless the database schema enforces a uniqueness constraint or your logic performs deduplication. Please confirm there’s either a unique index across (resourceId, deploymentId, environmentId) or an upsert approach.
You can verify any existing uniqueness constraints with a script like:
If no constraint is found, consider updating the schema or implementing deduplication logic.
🏁 Script executed:
#!/bin/bash
# Description: Verify if releaseTarget has a unique constraint across (resourceId, deploymentId, environmentId).
ast-grep --pattern $'CREATE TABLE $_ ( $$$ )'
rg 'releaseTarget'Length of output: 5395
Ensure Release Target Uniqueness in Database or via Deduplication
The current implementation in apps/event-worker/src/workers/env-selector-update.ts flattens and inserts release targets without any deduplication. Our script output did not reveal any explicit unique constraint or index enforcing uniqueness across (resourceId, deploymentId, environmentId) in the releaseTarget table (as defined in files like packages/db/src/schema/release.ts). This leaves the system vulnerable to duplicate entries if rapid, overlapping updates occur.
Please confirm one of the following:
- There is a unique constraint (or unique index) on
(resourceId, deploymentId, environmentId)at the database schema level. - Deduplication or an upsert mechanism is implemented during the insert operation.
Action Items:
- Verify and, if necessary, update the schema in
packages/db/src/schema/release.tsto enforce uniqueness. - Alternatively, implement deduplication/upsert logic in
apps/event-worker/src/workers/env-selector-update.tsto avoid duplicate records.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/event-worker/src/workers/env-selector-update.ts (1)
167-219: Consider wrapping database operations in a single transaction.All database calls (inserts, deletes, event dispatch) run concurrently in separate promises. In the event of an error or concurrent updates, partial changes could be committed. Wrapping them in a transaction (or applying consistent concurrency safeguards) would maintain data integrity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/workers/env-selector-update.ts(1 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/env-selector-update.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (1)
apps/event-worker/src/workers/env-selector-update.ts (1)
62-77: Revisit Release Target deduplication or uniqueness constraints.This code path inserts multiple release targets without enforcing uniqueness across
(resourceId, deploymentId, environmentId). If the system receives overlapping updates, duplicates may be inserted unless there’s a uniqueness constraint or deduplication logic.
Summary by CodeRabbit
New Features
Bug Fixes