chore: init create release util #455
Conversation
WalkthroughThis pull request introduces asynchronous release creation within the deployment worker and expands release management functionality in the rule engine. The event worker now maps over inserted release targets to create releases concurrently, while the rule engine gains a new method and function for fetching policy-matching releases. Additionally, exports in the module have been reorganized, and new utility functions for creating releases have been added with proper error handling and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as newDeploymentVersionWorker
participant DB as Database
participant Resource as Resource Lookup
participant ReleaseUtil as createRelease
Worker->>DB: Insert release targets
Worker->>Worker: Map over releaseTargets to build promises
loop For each releaseTarget
Worker->>DB: Query for resource by ID
alt Resource found
Worker->>ReleaseUtil: Call createRelease(db, releaseTarget, workspaceId)
else Resource not found
Worker-->>Worker: Throw error
end
end
Worker->>Worker: Await Promise.all(createReleasePromises)
sequenceDiagram
participant Caller as Release Creator
participant DB as Database (Tx)
participant Repo as Repository
participant ReleaseUtil as createRelease
Caller->>DB: Call createRelease(db, releaseTarget, workspaceId, versionId?)
DB->>DB: Query for release target
alt Release target exists
ReleaseUtil->>Repo: Get repository instance
ReleaseUtil->>Repo: Call getDesiredVersionId(db, repo, versionId)
alt Version ID found
ReleaseUtil->>DB: Upsert new release with latest variables
DB->>Repo: Validate against policy-matching releases
ReleaseUtil->>Repo: Set new release as desired release
else Version ID missing
ReleaseUtil-->>Caller: Throw error (missing version)
end
else
ReleaseUtil-->>Caller: Throw error (release target not found)
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 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...
🧬 Code Definitions (3)apps/event-worker/src/workers/update-deployment-variable.ts (1)
apps/event-worker/src/workers/env-selector-update.ts (1)
apps/event-worker/src/workers/update-resource-variable.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (13)
✨ 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
|
![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 (6)
packages/rule-engine/src/repositories/get-releases.ts (1)
146-160: LGTM: Well-implemented function for policy-matching releasesThe new
findAllPolicyMatchingReleasesfunction follows the established patterns of the codebase, logically extending the existing functionality fromfindLatestPolicyMatchingReleaseto return all matching releases instead of just the latest one.One suggestion would be to add JSDoc comments for this function similar to the other functions in this file to maintain consistency and improve code documentation.
+/** + * Finds all releases that match the applicable policy for a given release target + * + * @param tx - Database transaction object for querying + * @param policy - The policy to filter releases by + * @param releaseTarget - The release target to find matching releases for + * @returns Promise resolving to an array of all matching releases + */ export const findAllPolicyMatchingReleases = async ( tx: Tx, policy: Policy | null, releaseTarget: { id: string }, ): Promise<CompleteRelease[]> =>packages/rule-engine/src/utils/create-release.ts (5)
9-28: Consider adding explicit return type to the function signature.The
getDesiredVersionIdfunction can return either a string or null, but this isn't explicitly defined in the function signature. Adding an explicit return type would improve type safety and make the API more clear.-const getDesiredVersionId = async ( +const getDesiredVersionId = async ( db: Tx, repo: DatabaseReleaseRepository, versionId?: string, -) => { +): Promise<string | null> => {
30-35: Add explicit return type to createRelease function.The
createReleasefunction doesn't specify a return type. Consider addingPromise<void>to make it clear that the function doesn't return any value.-export const createRelease = async ( +export const createRelease = async ( db: Tx, releaseTarget: ReleaseTargetIdentifier, workspaceId: string, versionId?: string, -) => { +): Promise<void> => {
58-58: Consider returning a more informative result.The function currently returns early with no value when a release isn't created. Consider returning a more informative result, such as an object indicating whether a release was created and potentially including the release details.
- if (!created) return; + if (!created) return { created: false, release };And update the return type of the function:
-export const createRelease = async ( +export const createRelease = async ( db: Tx, releaseTarget: ReleaseTargetIdentifier, workspaceId: string, versionId?: string, -): Promise<void> => { +): Promise<{ created: boolean; release?: typeof release }> => {
60-64: Handle both valid and invalid release scenarios.Currently, the function sets the desired release only if it's valid, but doesn't handle the case when the release is invalid. Consider adding error handling or at least logging for the invalid case.
const isReleaseValid = allValidIds.includes(release.id); - if (isReleaseValid) repo.setDesiredRelease(release.id); + if (isReleaseValid) { + await repo.setDesiredRelease(release.id); + return { created: true, release, valid: true }; + } else { + // Handle the invalid release case - perhaps log or throw an error + return { created: true, release, valid: false }; + }
30-65: Add error handling around repository operations.The
createReleasefunction has error handling for specific cases but doesn't have a general try/catch to handle unexpected errors that might occur during repository operations. Consider adding comprehensive error handling.export const createRelease = async ( db: Tx, releaseTarget: ReleaseTargetIdentifier, workspaceId: string, versionId?: string, ): Promise<void> => { + try { const rt = await db.query.releaseTarget.findFirst({ where: and( eq(schema.releaseTarget.deploymentId, releaseTarget.deploymentId), eq(schema.releaseTarget.environmentId, releaseTarget.environmentId), eq(schema.releaseTarget.resourceId, releaseTarget.resourceId), ), }); if (rt == null) throw new Error("Release target not found"); const repo = await DatabaseReleaseRepository.create({ ...rt, workspaceId }); const desiredVersionId = await getDesiredVersionId(db, repo, versionId); if (desiredVersionId == null) throw new Error("Could not find desired version"); const variables = await repo.getLatestVariables(); const { release, created } = await repo.upsert( releaseTarget, desiredVersionId, variables, ); if (!created) return; const allPolicyMatchingReleases = await repo.findAllPolicyMatchingReleases(); const allValidIds = allPolicyMatchingReleases.map((r) => r.id); const isReleaseValid = allValidIds.includes(release.id); if (isReleaseValid) repo.setDesiredRelease(release.id); + } catch (error) { + // Consider more specific error handling or logging + throw new Error(`Failed to create release: ${error.message}`); + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/workers/new-deployment-version.ts(2 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(3 hunks)packages/rule-engine/src/repositories/get-releases.ts(1 hunks)packages/rule-engine/src/utils/create-release.ts(1 hunks)packages/rule-engine/src/utils/index.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.
packages/rule-engine/src/index.tspackages/rule-engine/src/repositories/get-releases.tspackages/rule-engine/src/utils/index.tspackages/rule-engine/src/repositories/db-release-repository.tsapps/event-worker/src/workers/new-deployment-version.tspackages/rule-engine/src/utils/create-release.ts
🧬 Code Definitions (4)
packages/rule-engine/src/repositories/get-releases.ts (2)
packages/rule-engine/src/repositories/db-release-repository.ts (1)
findAllPolicyMatchingReleases(118-121)packages/rule-engine/src/repositories/types.ts (1)
CompleteRelease(41-41)
packages/rule-engine/src/repositories/db-release-repository.ts (2)
packages/rule-engine/src/repositories/types.ts (1)
CompleteRelease(41-41)packages/rule-engine/src/repositories/get-releases.ts (1)
findAllPolicyMatchingReleases(146-160)
apps/event-worker/src/workers/new-deployment-version.ts (1)
packages/rule-engine/src/utils/create-release.ts (1)
createRelease(30-65)
packages/rule-engine/src/utils/create-release.ts (1)
packages/rule-engine/src/repositories/db-release-repository.ts (1)
DatabaseReleaseRepository(45-248)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (7)
packages/rule-engine/src/utils/index.ts (1)
1-2: LGTM: Clean barrel module structure for utility exportsThe index file follows the standard barrel pattern for re-exporting utilities, providing a clean and organized access point for both create-release and merge-policies modules.
packages/rule-engine/src/index.ts (1)
6-6: Improved module organizationReplacing the direct export of merge-policies with the utils/index.js export creates a more maintainable structure, providing a single entry point for all utility functions including the new release creation functionality.
apps/event-worker/src/workers/new-deployment-version.ts (1)
8-8: LGTM: New import added for release creation functionalityThe import for the
createReleasefunction from the rule-engine package is properly added to support the new functionality.packages/rule-engine/src/repositories/db-release-repository.ts (3)
23-23: Well-organized module import.The import of
findAllPolicyMatchingReleasesfrom the get-releases.js module aligns well with the existing imports and supports the new method being added to the class.
100-100: Documentation improvement enhances clarity.The updated documentation clarifies that
findMatchingReleasesspecifically retrieves releases between the latest deployed and desired release, which helps distinguish it from the new method being added.
113-121: Good implementation of the new method.The new
findAllPolicyMatchingReleasesmethod follows the established pattern in the class, correctly obtaining the policy before delegating to the imported function. The method is well-documented with appropriate JSDoc comments.packages/rule-engine/src/utils/create-release.ts (1)
44-44: Good error handling for missing release target.Properly checking for the existence of the release target and throwing a descriptive error message if not found is a good practice.
| const createReleasePromises = releaseTargets.map(async (rt) => { | ||
| const resource = await db.query.resource.findFirst({ | ||
| where: eq(schema.resource.id, rt.resourceId), | ||
| }); | ||
| if (resource == null) throw new Error("Resource not found"); | ||
|
|
||
| await createRelease(db, rt, resource.workspaceId); | ||
| }); | ||
| await Promise.all(createReleasePromises); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider optimizing resource queries for better performance
The current implementation queries resources individually for each release target, which could lead to performance issues with many targets. Consider fetching all resources at once and then mapping them to release targets.
- const createReleasePromises = releaseTargets.map(async (rt) => {
- const resource = await db.query.resource.findFirst({
- where: eq(schema.resource.id, rt.resourceId),
- });
- if (resource == null) throw new Error("Resource not found");
-
- await createRelease(db, rt, resource.workspaceId);
- });
- await Promise.all(createReleasePromises);
+ // Fetch all resources in a single query
+ const resourceIds = releaseTargets.map(rt => rt.resourceId);
+ const resources = await db.query.resource.findMany({
+ where: eq(schema.resource.id, { in: resourceIds }),
+ });
+
+ // Create a map for quick lookups
+ const resourceMap = new Map(resources.map(r => [r.id, r]));
+
+ // Create releases with proper error handling
+ const createReleasePromises = releaseTargets.map(async (rt) => {
+ const resource = resourceMap.get(rt.resourceId);
+ if (resource == null) throw new Error(`Resource not found: ${rt.resourceId}`);
+
+ await createRelease(db, rt, resource.workspaceId);
+ });
+ await Promise.all(createReleasePromises);Also consider adding error handling to prevent a single failure from stopping all release creation:
const results = await Promise.allSettled(createReleasePromises);
const failures = results.filter(r => r.status === 'rejected');
if (failures.length > 0) {
console.error(`Failed to create ${failures.length} releases:`,
failures.map(f => f.reason?.message || f.reason));
// Decide whether to throw or continue
}Let's verify if the in operator is supported with Drizzle ORM:
🌐 Web query:
Does Drizzle ORM support the `in` operator for filtering multiple IDs?
💡 Result:
Drizzle ORM supports filtering by multiple IDs using the inArray operator, which serves the same purpose as the SQL IN clause. Here's how it works:
Using inArray for Multiple IDs
-
Basic usage: Pass the table column and an array of values to
inArrayimport { inArray } from 'drizzle-orm'; const userIds = [1, 2, 3]; const users = await db .select() .from(usersTable) .where(inArray(usersTable.id, userIds));
-
Combining with other filters: Use
and/oroperators for complex queries[1][5][8]import { and, eq } from 'drizzle-orm'; const result = await db .select() .from(books) .where(and( inArray(books.id, [4, 7, 9]), eq(books.category, 'fiction') ));
Key Details
- Type-safe: Works with typed arrays[1][4][8]
- SQL Injection protection: Automatically parameterizes values[8]
- Cross-dialect support: Works consistently across PostgreSQL, MySQL, and SQLite[1][4]
For cases where you need to check if a value exists in a JSON array column, you might need to use raw SQL expressions with the sql template[6][7], but inArray is preferred for standard ID filtering[8][9].
Example with optional ID list filtering:
const filterByIds = (ids?: number[]) =>
ids ? inArray(usersTable.id, ids) : undefined;
const users = await db
.select()
.from(usersTable)
.where(and(
eq(usersTable.active, true),
filterByIds([101, 202, 303])
));
```[5][8]
Citations:
- 1: https://orm.drizzle.team/docs/operators
- 2: https://www.youtube.com/watch?v=R4ZgTB-1wEg
- 3: https://orm.drizzle.team/docs/indexes-constraints
- 4: https://orm.drizzle.team/docs/rqb
- 5: https://www.answeroverflow.com/m/1172728238968156230
- 6: https://frontendmasters.com/blog/introducing-drizzle/
- 7: https://orm.drizzle.team/docs/sql
- 8: https://www.answeroverflow.com/m/1195618774590697493
- 9: https://www.prisma.io/docs/orm/more/comparisons/prisma-and-drizzle
- 10: https://orm.drizzle.team/docs/relations
---
</details>
**Optimize Resource Query & Correct Operator Usage**
The current implementation queries each resource individually. To improve performance, fetch all resources at once and map them to the corresponding release targets. Also, note that Drizzle ORM doesn't support the native `in` operator—instead, use its `inArray` operator for filtering.
**Points to address:**
- **Bulk Querying:** Instead of querying for each release target, extract all resource IDs and perform a single query.
- **Operator Correction:** Replace the invalid `in` filtering syntax with Drizzle ORM’s `inArray` operator.
- **Error Handling:** Consider using `Promise.allSettled` to ensure that a single failure does not disrupt the release creation process.
**Revised Code Snippet Example:**
```diff
- // Fetch all resources in a single query
- const resourceIds = releaseTargets.map(rt => rt.resourceId);
- const resources = await db.query.resource.findMany({
- where: eq(schema.resource.id, { in: resourceIds }),
- });
+ import { inArray } from 'drizzle-orm';
+
+ // Fetch all resources in a single query using inArray
+ const resourceIds = releaseTargets.map(rt => rt.resourceId);
+ const resources = await db.query.resource.findMany({
+ where: inArray(schema.resource.id, resourceIds),
+ });
- // Create a map for quick lookups
- const resourceMap = new Map(resources.map(r => [r.id, r]));
-
- // Create releases with proper error handling
- const createReleasePromises = releaseTargets.map(async (rt) => {
- const resource = resourceMap.get(rt.resourceId);
- if (resource == null) throw new Error(`Resource not found: ${rt.resourceId}`);
-
- await createRelease(db, rt, resource.workspaceId);
- });
- await Promise.all(createReleasePromises);
+ // Create a map for quick lookups
+ const resourceMap = new Map(resources.map(r => [r.id, r]));
+
+ // Create releases with proper error handling
+ const createReleasePromises = releaseTargets.map(async (rt) => {
+ const resource = resourceMap.get(rt.resourceId);
+ if (resource == null) throw new Error(`Resource not found: ${rt.resourceId}`);
+
+ await createRelease(db, rt, resource.workspaceId);
+ });
+ await Promise.all(createReleasePromises);Error handling suggestion:
const results = await Promise.allSettled(createReleasePromises);
const failures = results.filter(r => r.status === 'rejected');
if (failures.length > 0) {
console.error(`Failed to create ${failures.length} releases:`,
failures.map(f => f.reason?.message || f.reason));
// Decide whether to throw or continue with partial success
}
| const createReleasePromises = releaseTargets.map(async (rt) => { | ||
| const resource = resources.find((r) => r.id === rt.resourceId); | ||
| if (resource == null) throw new Error("Resource not found"); | ||
|
|
||
| await createRelease(db, rt, resource.workspaceId, version.id); | ||
| }); | ||
| await Promise.all(createReleasePromises); |
There was a problem hiding this comment.
| const createReleasePromises = releaseTargets.map(async (rt) => { | |
| const resource = resources.find((r) => r.id === rt.resourceId); | |
| if (resource == null) throw new Error("Resource not found"); | |
| await createRelease(db, rt, resource.workspaceId, version.id); | |
| }); | |
| await Promise.all(createReleasePromises); | |
| const releases = releaseTargets.map((rt) => { | |
| const resource = resources.find((r) => r.id === rt.resourceId); | |
| if (resource == null) throw new Error("Resource not found"); | |
| return createRelease(db, rt, resource.workspaceId, version.id); | |
| }); | |
| await Promise.all(releases); |
| tx.query.release.findMany({ | ||
| where: and( | ||
| eq(schema.release.releaseTargetId, releaseTarget.id), | ||
| schema.deploymentVersionMatchesCondition( | ||
| tx, | ||
| policy?.deploymentVersionSelector?.deploymentVersionSelector, | ||
| ), | ||
| ), | ||
| with: { variables: true, version: { with: { metadata: true } } }, | ||
| }); |
There was a problem hiding this comment.
You are applying a version selector to a release?
|
rename to update-environment not this weird selector crap |
| getQueue(Channel.EvaluateReleaseTarget).addBulk( | ||
| .then(async (releaseTargets) => { | ||
| const createReleasePromises = releaseTargets.map((rt) => | ||
| createRelease(db, rt, workspaceId), |
There was a problem hiding this comment.
you shouldn't be creating releases in a function that is suppose to createReleaseTargets
| .then((results) => ({ | ||
| deploymentIds: results.map((result) => result.deployment.id), | ||
| resource: results[0]?.resource, | ||
| })); |
There was a problem hiding this comment.
no, make this different, const resource = results.at(0)
You are making this way more complicated
| }); | ||
| }; | ||
|
|
||
| export const findAllPolicyMatchingReleases = async ( |
| const isReleaseValid = allValidIds.includes(release.id); | ||
| if (isReleaseValid) repo.setDesiredRelease(release.id); |
There was a problem hiding this comment.
don't set desired release, the function is called createRelease, yet you are setting desired versions?
There was a problem hiding this comment.
don't set the desired release anywhere
Summary by CodeRabbit
New Features
Bug Fixes