feat: Init new dpeloyment version worker#447
Conversation
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant API as Deployment API
participant Q as Queue (NewDeploymentVersion)
participant WD as NewDeploymentVersionWorker
participant DB as Database/Services
participant CR as createReleases
API->>Q: Enqueue new deployment version event
Q->>WD: Dispatch event to worker
WD->>DB: Retrieve deployment details & resources
DB-->>WD: Return deployment and resource data
WD->>CR: Map resources to release targets and call createReleases
CR-->>WD: Return aggregated results
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (8)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
113-122: Fallback logic for workspaceId retrieval is sound.
You correctly retrieve the workspace ID from the resource ifworkspaceIdis not provided. Consider whether additional handling is needed if no correspondences are found, thoughtakeFirstwill throw in that case.apps/event-worker/src/workers/releases/create-release.ts (3)
13-13: Commented-out queue code.
The line disablingpolicyEvaluateQueueimport suggests a pivot from asynchronous usage. Consider removing it if there are no current plans to reactivate it.
61-61: Commented-out policy evaluation queue.
With queue-based actions currently disabled, consider removing this line if it’s no longer needed.
67-91: Bulk release creation approach.
YourcreateReleasesfunction concurrently processes each release target with a mutex on each repository. The concurrency approach is valid; just be mindful of potential performance overhead if you expect many targets.apps/event-worker/src/workers/releases/new-deployment-version.ts (4)
2-2: Remove unused lodash import.It appears that the
_import from lodash is not used within this file. Removing unused imports helps keep the codebase clean.Apply this diff to remove the unused import:
-import _ from "lodash";
20-21: Clarify error message content.The error message "System or deployment not found" could be more specific. Consider rephrasing it to reflect precisely what was not found.
Proposed change:
-if (system == null) throw new Error("System or deployment not found"); +if (system == null) throw new Error("System not found for specified deployment");
23-41: Consider optimizing multiple environment queries.Each environment is queried individually. For deployments with many environments, this could become inefficient. Investigate using a single aggregated query if possible, or caching frequently accessed data, to reduce multiple sequential lookups.
56-77: Ensure behavior when resources array is empty.Currently, the worker directly invokes
createReleaseseven ifresourcesis empty. While this won’t cause an error, you may want to log or handle the scenario of no matching resources to maintain operational visibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/workers/releases/create-release.ts(2 hunks)apps/event-worker/src/workers/releases/evaluate/index.ts(1 hunks)apps/event-worker/src/workers/releases/new-deployment-version.ts(1 hunks)packages/events/src/types.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.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/releases/evaluate/index.tsapps/event-worker/src/workers/releases/new-deployment-version.tspackages/events/src/types.tspackages/rule-engine/src/db/get-applicable-policies.tsapps/event-worker/src/workers/releases/create-release.ts
🧬 Code Definitions (4)
apps/event-worker/src/workers/releases/evaluate/index.ts (1)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-140)
apps/event-worker/src/workers/releases/new-deployment-version.ts (1)
apps/event-worker/src/workers/releases/create-release.ts (1)
createReleases(68-91)
packages/rule-engine/src/db/get-applicable-policies.ts (2)
packages/db/src/common.ts (1)
takeFirst(9-13)packages/db/src/schema/policy.ts (1)
policy(24-40)
apps/event-worker/src/workers/releases/create-release.ts (1)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-140)
⏰ 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 (12)
apps/event-worker/src/workers/releases/evaluate/index.ts (1)
32-32: Confirmdbusage as transaction object.
The updatedgetApplicablePolicies(db, job.data, workspaceId)call appears valid given the new function signature(tx, repo, workspaceId?). Please ensure thatdbindeed implements theTxinterface or a compatible transaction context.packages/events/src/types.ts (2)
13-13: New event channel addition looks good.
AddingNewDeploymentVersionto theChannelenum is consistent and clear.
27-27: Channel map extension is consistent.
AssociatingNewDeploymentVersionwithschema.DeploymentVersionaligns well with existing patterns.packages/rule-engine/src/db/get-applicable-policies.ts (3)
4-4: Unused import note (no action required).
The newly importedtakeFirstOrNullis currently unused here. If this is intentional for future enhancements, feel free to leave it in.
111-112: Optional workspace ID aligns with flexible design.
Changing the parameter toworkspaceId?: stringhelps accommodate scenarios where the workspace ID is determined at runtime.
125-125: Ensuring policy query uses derived workspace ID.
Usingeq(schema.policy.workspaceId, wsId)ensures the correct workspace scope. No issues here.apps/event-worker/src/workers/releases/create-release.ts (5)
3-9: Imports refactored for direct database and schema usage.
Switching to directand, eq,db, andschemaimports is consistent with your updated approach.
15-28: NewcreateReleaseWithLockfunction introduction.
Renaming/introducing this function with a mutex-based approach is a solid way to avoid concurrent modifications. This design is clear and maintainable.
29-33: Immediate release logic if versionId is null.
Setting a desired release without policy checks appears intentional. Confirm if skipping policy evaluation in this scenario is fully aligned with your requirements.
34-39: Merged policy null-check short-circuits.
By returning early whendeploymentVersionSelectoris null, you’re effectively opting for an immediate release. This is reasonable, but ensure no unintended skipping of other checks.
41-55: Selective release based on version selector match.
The logic to verify if the deployment version matches the provided selector is clear. If there's no match, the function ends without setting the release. Double-check that no fallback is required in that case.apps/event-worker/src/workers/releases/new-deployment-version.ts (1)
45-55: Good use of documentation.The docstring provides a concise, clear overview of the worker’s responsibilities and sequence of operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/event-worker/src/workers/releases/create-release.ts (1)
47-61: Consider simplifying the version selector match check for readability.The nested condition with database query could be refactored for better readability.
- const isVersionSelectorMatch = - (await db.query.deploymentVersion.findFirst({ - where: and( - eq(schema.deploymentVersion.id, versionId), - schema.deploymentVersionMatchesCondition( - db, - deploymentVersionSelector, - ), - ), - })) != null; + const matchingVersion = await db.query.deploymentVersion.findFirst({ + where: and( + eq(schema.deploymentVersion.id, versionId), + schema.deploymentVersionMatchesCondition( + db, + deploymentVersionSelector, + ), + ), + }); + const isVersionSelectorMatch = matchingVersion != null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/releases/create-release.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/index.tsapps/event-worker/src/workers/releases/create-release.ts
🧬 Code Definitions (1)
apps/event-worker/src/workers/releases/create-release.ts (1)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-130)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (3)
apps/event-worker/src/workers/index.ts (1)
8-8: Clean integration of the new deployment version worker.The new worker has been properly imported and registered in the workers map, following the existing pattern. This addition enables the system to process events from the
NewDeploymentVersionchannel.Also applies to: 17-17
apps/event-worker/src/workers/releases/create-release.ts (2)
15-72: Well-structured refactoring with improved version-based logic.The function has been thoughtfully renamed to
createReleaseWithLockwhich better reflects its purpose. The new implementation properly handles different paths based on the presence ofversionIdand elegantly short-circuits when appropriate conditions are met.However, there are several commented-out lines related to queues (line 6, 13, 67). Is this intentional as part of an architectural change away from queue-based processing?
74-97: Effective implementation of batch release creation with concurrency.The new
createReleasesfunction efficiently handles multiple release targets by:
- First upserting all targets in a single database operation
- Then processing them concurrently using
Promise.allThis approach optimizes performance while maintaining the integrity of each release creation through the mutex lock.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/event-worker/src/workers/releases/new-deployment-version.ts (2)
18-18: Consider enhancing error message with more specific details.The current error message "System or deployment not found" doesn't indicate which entity couldn't be found or provide identifiers for debugging.
- if (system == null) throw new Error("System or deployment not found"); + if (system == null) throw new Error(`System not found for systemId: ${deployment.systemId}`);
61-61: Consider adding logging for better observability.Adding log statements would help track worker execution and make debugging easier, especially for error cases.
- if (!deployment) throw new Error("Deployment not found"); + if (!deployment) { + logger.error(`Deployment not found for id: ${version.deploymentId}`); + throw new Error("Deployment not found"); + } + + logger.info(`Processing new deployment version for deployment: ${deployment.id}`);apps/event-worker/src/index.ts (1)
10-16: Consider adding error handling for the shutdown process.The current shutdown function doesn't handle errors that might occur during worker closure. If any worker's close method throws an exception, the process might not exit cleanly.
const shutdown = () => { logger.warn("Exiting..."); - Promise.all(Object.values(workers).map((w) => w?.close())).then(async () => { - await redis.quit(); - process.exit(0); - }); + Promise.all(Object.values(workers).map((w) => w?.close().catch(err => { + logger.error(`Error closing worker: ${err.message}`); + return null; + }))).then(async () => { + try { + await redis.quit(); + } catch (err) { + logger.error(`Error closing Redis connection: ${err.message}`); + } finally { + process.exit(0); + } + }); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/index.ts(1 hunks)apps/event-worker/src/workers/releases/create-release.ts(0 hunks)apps/event-worker/src/workers/releases/new-deployment-version.ts(1 hunks)apps/event-worker/src/workers/releases/new-version/index.ts(0 hunks)apps/event-worker/src/workers/releases/variable-change/index.ts(0 hunks)
💤 Files with no reviewable changes (3)
- apps/event-worker/src/workers/releases/create-release.ts
- apps/event-worker/src/workers/releases/variable-change/index.ts
- apps/event-worker/src/workers/releases/new-version/index.ts
🧰 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/index.tsapps/event-worker/src/workers/releases/new-deployment-version.ts
🧬 Code Definitions (1)
apps/event-worker/src/index.ts (1)
apps/event-worker/src/workers/index.ts (1)
workers(15-22)
⏰ 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 (4)
apps/event-worker/src/workers/releases/new-deployment-version.ts (3)
9-41: Solid implementation of resource retrieval logic.The
getDeploymentResourcesfunction effectively handles the complex task of fetching resources that match both environment and deployment selectors. The error handling is appropriate, and the use of Promise.all for parallel execution is efficient.
43-53: Well-documented worker implementation.The documentation clearly explains the worker's purpose and processing steps, which greatly enhances code maintainability.
54-76: LGTM: Worker implementation correctly handles new deployment versions.The worker properly retrieves resources matching selectors and creates release targets. The use of
onConflictDoNothingis a good approach for handling potential duplicate entries.apps/event-worker/src/index.ts (1)
12-12: Improved shutdown logic using dynamic worker collection.The change to use
Object.values(workers)directly makes the code more maintainable. When new workers are added to the workers object, the shutdown logic will automatically include them without requiring updates.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor