init filter rule #435
init filter rule #435
Conversation
WalkthroughThis PR introduces a new asynchronous mechanism for retrieving releases. The event worker now uses a dynamically queried list of releases via the Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Release Worker
participant Evaluate as Evaluate Function
participant DB as Database Service
Worker->>Evaluate: execute(policy, getReleases, context)
Evaluate-->>Evaluate: merge policies
alt mergedPolicy is null
Evaluate->>Worker: Return {allowed: false, release: undefined}
else
Evaluate->>DB: getReleases(mergedPolicy)
DB-->>Evaluate: releases array
Evaluate->>Worker: Continue with evaluation using releases
end
Possibly related PRs
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
|
![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 (3)
packages/db/src/schema/policy.ts (1)
76-256: Consider adding insert/update schemas for the new table.Unlike the other tables in this file, there are no corresponding insert/update schemas defined for the new
policyDeploymentVersionSelectortable. Consider adding these for consistency.You could add schemas similar to how the other tables are handled, for example:
const policyDeploymentVersionSelectorInsertSchema = createInsertSchema( policyDeploymentVersionSelector, { policyId: z.string().uuid(), name: z.string().min(1, "Name is required"), description: z.string().optional(), deploymentVersionSelector: z.any(), // You might want to create a proper schema for this } ).omit({ id: true }); export const createPolicyDeploymentVersionSelector = policyDeploymentVersionSelectorInsertSchema; export type CreatePolicyDeploymentVersionSelector = z.infer<typeof createPolicyDeploymentVersionSelector>; export const updatePolicyDeploymentVersionSelector = policyDeploymentVersionSelectorInsertSchema.partial(); export type UpdatePolicyDeploymentVersionSelector = z.infer<typeof updatePolicyDeploymentVersionSelector>;apps/event-worker/src/releases/evaluate/index.ts (2)
43-64: Consider using the Policy parameter.The function accepts a
Policyparameter but doesn't use it. Based on the comments, this parameter is intended to be used for filtering releases based on policy conditions.Once the TODOs are addressed, ensure that the Policy parameter is properly utilized to filter releases based on the deployment version selector.
43-64: Add error handling for database operations.There's no error handling for the database query. Consider adding try/catch blocks to handle potential database errors gracefully.
const getReleases = async (_: Policy) => { + try { return await db.query.release.findMany({ where: and( eq(schema.release.deploymentId, ctx.deploymentId), eq(schema.release.resourceId, ctx.resourceId), eq(schema.release.environmentId, ctx.environmentId), ), with: { version: true, variables: true, }, orderBy: desc(schema.release.createdAt), }); + } catch (error) { + console.error("Error fetching releases:", error); + return []; + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/releases/evaluate/index.ts(2 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/release-manager/src/variables/types.ts(1 hunks)packages/rule-engine/src/evaluate.ts(1 hunks)packages/rule-engine/src/types.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/db/src/schema/policy.tspackages/release-manager/src/variables/types.tspackages/rule-engine/src/types.tsapps/event-worker/src/releases/evaluate/index.tspackages/rule-engine/src/evaluate.ts
🧬 Code Definitions (2)
packages/rule-engine/src/types.ts (1)
packages/db/src/schema/policy.ts (1)
PolicyDeploymentVersionSelector(254-256)
packages/rule-engine/src/evaluate.ts (2)
packages/db/src/schema/policy.ts (2)
policy(24-40)Policy(249-249)packages/rule-engine/src/types.ts (3)
Policy(66-69)Release(7-17)DeploymentResourceContext(37-42)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
packages/release-manager/src/variables/types.ts (1)
1-1: Import path update looks good.The updated import path uses a relative path with a
.jsextension which is a good practice in TypeScript projects using ESM.packages/rule-engine/src/types.ts (1)
68-68: Type extension for Policy looks good.Adding
deploymentVersionSelectorto thePolicytype properly extends it to support the new capability being introduced. The type is correctly defined asschema.PolicyDeploymentVersionSelector | null.packages/db/src/schema/policy.ts (3)
3-3: LGTM - Import added for DeploymentVersionCondition.The import is correctly added to support the new table definition.
76-95: Table definition is well-structured.The new
policyDeploymentVersionSelectortable is defined with appropriate constraints:
- Primary key for the ID
- Foreign key relationship to the policy table
- Unique constraint on policyId ensuring one-to-one relationship
- Required fields marked as notNull
254-256: Type export is consistent with schema pattern.The
PolicyDeploymentVersionSelectortype correctly usesInferSelectModelto generate the type from the table definition.apps/event-worker/src/releases/evaluate/index.ts (2)
1-9: Import statements look good.All required imports are properly added to support the new functionality.
66-66: Updated evaluate call looks good.The
evaluatefunction is now correctly called with the newgetReleasesfunction as a parameter.packages/rule-engine/src/evaluate.ts (3)
24-27: Good function signature update to support asynchronous release retrieval.The change from accepting a static array of releases to using a function parameter that can asynchronously fetch releases is a good architectural improvement. This allows for more flexibility in how releases are obtained and makes the evaluation process more dynamic.
33-38: Good defensive programming with explicit handling of null merged policy.Adding this early return when no valid policy is available improves error handling and makes the behavior more predictable. It correctly prevents further processing when there's no policy to evaluate against.
41-41: Correct implementation of async release retrieval.The code properly awaits the result of
getReleases(mergedPolicy), ensuring that releases are available before proceeding with evaluation. This correctly handles both synchronous and asynchronous release retrieval functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/utils/get-releases.ts (1)
16-28: Utilize consistent logging practices.The logger child object is created for the
rule-engine/getReleasesscope, which is good for isolating logs. Consider adding context fields (e.g.deploymentId,resourceId) to enhance debugging if there are future issues with date bounds validation.packages/rule-engine/src/evaluate.ts (1)
49-49: Consider capturing potential errors from getReleases.While the call to
await getReleases(mergedPolicy)is appropriate, any internal errors will bubble up directly. Verify that the caller or higher-level logic can handle these failures properly if necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/event-worker/src/releases/evaluate/index.ts(2 hunks)packages/db/src/schema/resource.ts(1 hunks)packages/release-manager/package.json(1 hunks)packages/rule-engine/package.json(2 hunks)packages/rule-engine/src/db/create-ctx.ts(1 hunks)packages/rule-engine/src/evaluate.ts(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/utils/get-releases.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/event-worker/src/releases/evaluate/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.
packages/rule-engine/src/index.tspackages/rule-engine/src/db/create-ctx.tspackages/rule-engine/src/evaluate.tspackages/db/src/schema/resource.tspackages/rule-engine/src/utils/get-releases.ts
🧬 Code Definitions (2)
packages/db/src/schema/resource.ts (3)
packages/db/src/schema/release.ts (1)
release(18-37)packages/db/src/schema/deployment.ts (1)
deployment(66-90)packages/db/src/schema/environment.ts (1)
environment(57-82)
packages/rule-engine/src/utils/get-releases.ts (1)
packages/db/src/schema/resource.ts (1)
resourceRelease(91-120)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (11)
packages/rule-engine/src/index.ts (1)
7-7: Added export for the new get-releases moduleThe new export statement correctly makes all entities from the
./utils/get-releases.jsmodule available for external importing. This aligns with the PR objective of introducing a new asynchronous mechanism for retrieving releases.packages/release-manager/package.json (1)
32-32: Added TypeScript type definitions for lodashAdding
@types/lodashas a dev dependency is appropriate sincelodashis already listed as a main dependency. This improves type safety and developer experience when using lodash in TypeScript files.packages/rule-engine/src/db/create-ctx.ts (1)
3-3: Updated import source for database utilitiesThe import for
andandeqfunctions has been updated to source from@ctrlplane/dbinstead of directly fromdrizzle-orm. This is a good practice that centralizes database utilities and creates a more consistent API across the project.packages/rule-engine/package.json (2)
28-28: Added logger dependencyThe addition of
@ctrlplane/loggeras a workspace dependency is appropriate for the new asynchronous operations that may require logging capabilities, especially for error handling in the updatedevaluatefunction mentioned in the PR summary.
41-41: Added TypeScript type definitions for lodashThe addition of
@types/lodashas a dev dependency improves type safety and developer experience when using lodash in TypeScript files. This change is consistent with the same addition in the release-manager package.packages/db/src/schema/resource.ts (1)
122-142: Looks good!These newly introduced relations properly define one-to-one mappings for each related table field. The syntax and references appear consistent with the existing schema conventions.
packages/rule-engine/src/utils/get-releases.ts (2)
1-14: Imports and type definitions look appropriate.No concerns about importing
Txfrom@ctrlplane/dborisAfterfromdate-fns. ThePolicytype definition is clear, and the usage of typed properties (e.g.,denyWindows) is consistent with the rest of the codebase.
30-119:❓ Verification inconclusive
Ensure error handling aligns with best practices.
The function
getReleasesutilizes two database queries but does not implement an explicit error or exception handling flow. Should either query fail (e.g., a connection issue or an invalid schema operation), it may reject or produce an unhandled rejection. While your guidelines do not strictly requiretry/catch, consider ensuring upstream error handling is robust and logs or short-circuits gracefully when these queries fail.Is an upstream mechanism in place for handling any errors thrown by these queries (e.g., your request pipeline or a standard error boundary)? If not, we can create a follow-up script to trace all
getReleasesinvocations to ensure that the potential rejections are handled properly.
🏁 Script executed:
#!/bin/bash # Searching for all getReleases invocations to verify error handling in the rule engine or upper layers rg -A 3 "getReleases("Length of output: 99
Below is a new shell script that uses proper escaping to search for all invocations of
getReleases(in the codebase. This will help verify whether upstream error handling is in place for potential rejections from those database queries. Please run the script and share the output if available.Once we have the correct output, we can determine if there's an upstream error handling mechanism or if further action is required.
🏁 Script executed:
#!/bin/bash # Correctly search for all invocations of getReleases in the repository using escaped parenthesis rg -A 3 "getReleases\("Length of output: 802
Action Required: Verify Upstream Error Handling for Database Query Failures
It appears that the
getReleasesfunction (inpackages/rule-engine/src/utils/get-releases.ts, lines 30–119) does not include explicit error or exception handling around its two database queries. Our search of its invocations revealed that:
- In
packages/rule-engine/src/evaluate.ts, the call is made viaawait getReleases(mergedPolicy)without any local try/catch.- In
apps/event-worker/src/releases/evaluate/index.ts, the function is similarly invoked without explicit error handling.Please verify that a robust, upstream error handling mechanism (such as a global error boundary or middleware) exists to catch and appropriately handle rejections from these queries. If not, consider implementing dedicated try/catch blocks within these contexts to ensure graceful failure handling.
packages/rule-engine/src/evaluate.ts (3)
25-26: Accurate JSDoc comments.Documenting
getReleasesas a function returning a list of releases for the given policy clarifies usage for other contributors.
32-34: Async function signature is properly declared.Converting
evaluateinto anasyncfunction is consistent with the newgetReleasesusage. No issues noted.
41-45: Clear null check for mergedPolicy.Returning
{ allowed: false, release: undefined }whenmergedPolicyisnullis a logical safeguard, ensuring no further steps proceed with an undefined policy.
Summary by CodeRabbit