policy selector #430
policy selector #430
Conversation
WalkthroughThis pull request introduces a new release evaluation worker that processes Changes
Sequence Diagram(s)sequenceDiagram
participant W as ReleaseEvaluateWorker
participant DB as Database (createCtx)
participant P as Policy Engine (getApplicablePolicies)
participant E as Evaluator (evaluate)
W->>DB: createCtx(job.data)
alt Context is null
DB-->>W: null
W->>W: Log error and exit job
else Context exists
DB-->>W: context data with workspaceId
W->>P: getApplicablePolicies(workspaceId, repo)
P-->>W: list of policies
W->>E: evaluate(policies, context)
E-->>W: evaluation result
W->>Console: Log evaluation result
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 (1)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
93-127: Optimize repeated queries for large policy sets.
When workspaces have many policies, iterating through each policy and invokingmatchPolicyTargetForResourcefor every target may become expensive. Consider caching or performing a combined query that filters out non-matching environments/deployments before iterating, avoiding multiple repeated lookups.
📜 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 (7)
apps/event-worker/src/releases/evaluate/index.ts(1 hunks)packages/db/src/schema/policy.ts(2 hunks)packages/rule-engine/package.json(1 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)packages/rule-engine/src/db/index.ts(1 hunks)packages/rule-engine/src/types.ts(4 hunks)packages/validators/src/events/index.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.
packages/rule-engine/src/db/index.tsapps/event-worker/src/releases/evaluate/index.tspackages/validators/src/events/index.tspackages/db/src/schema/policy.tspackages/rule-engine/src/types.tspackages/rule-engine/src/db/get-applicable-policies.ts
🧬 Code Definitions (1)
packages/rule-engine/src/db/get-applicable-policies.ts (2)
packages/rule-engine/src/types.ts (1)
ReleaseRepository(70-74)packages/db/src/schema/policy.ts (2)
PolicyTarget(225-225)policy(22-38)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (11)
packages/rule-engine/package.json (1)
33-33: Dependency addition looks goodThe addition of "ts-is-present" with the "catalog:" version specifier is consistent with other dependencies in the package. This utility will likely help with type checking in policy evaluation logic.
packages/rule-engine/src/db/index.ts (1)
2-2: Export addition looks goodThe export statement correctly exposes the policy-related functionalities from the get-applicable-policies.js file, maintaining consistency with existing export patterns.
apps/event-worker/src/releases/evaluate/index.ts (1)
1-8: Imports look goodAll necessary imports are present for the worker implementation, including the event type, database connections, and rule engine utilities.
packages/validators/src/events/index.ts (2)
9-9: Enum addition looks goodThe ReleaseEvaluate enum value follows the existing naming pattern and provides a clear identifier for the new event channel.
23-28: Event definition is well-structuredThe releaseEvaluateEvent definition and corresponding type export correctly define the structure of release evaluation events using the Zod schema, consistent with other event definitions in the file.
packages/rule-engine/src/db/get-applicable-policies.ts (1)
29-91: Consider explicitly handling both-null scenario for selectors.
Currently, if bothdeploymentSelectorandenvironmentSelectorare null, the function returnsnull. If the intended behavior is that a policy with neither selector applies universally, you may want to add an explicit check for that scenario. Otherwise, the current behavior is correct if you intend to skip policies in such a case.packages/db/src/schema/policy.ts (1)
45-50: Typed JSONB fields look good.
Applying explicit types (DeploymentCondition | null,EnvironmentCondition | null) todeploymentSelectorandenvironmentSelectoris a solid approach for stricter type validation.packages/rule-engine/src/types.ts (4)
2-3: Stricter typing for deployment and resource conditions is clear.
The addition ofDeploymentVersionConditionandResourceConditionimports provides more clarity and safety when parsing or handling these selectors.
22-23: Improved type definition for deployment selectors.
UsingResourceConditionandDeploymentVersionConditioninDeploymentclarifies how each property is expected to be structured and validated.
34-34: ApplyingResourceConditionto environments.
DefiningresourceSelectorinEnvironmentwith an explicit type fosters consistency across models.
69-74: NewReleaseRepositorytype is well-structured.
DefiningdeploymentId,environmentId, andresourceIdas strings aligns perfectly with usage in the policy-matching code.
| export const createReleaseEvaluateWorker = () => | ||
| new Worker<ReleaseEvaluateEvent>(Channel.ReleaseEvaluate, async (job) => { | ||
| job.log( | ||
| `Evaluating release for deployment ${job.data.deploymentId} and resource ${job.data.resourceId}`, | ||
| ); | ||
|
|
||
| const ctx = await createCtx(db, job.data); | ||
| if (ctx == null) { | ||
| job.log( | ||
| `Resource ${job.data.resourceId} not found for deployment ${job.data.deploymentId} and environment ${job.data.environmentId}`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const { workspaceId } = ctx.resource; | ||
| const policy = await getApplicablePolicies(db, workspaceId, job.data); | ||
| const result = await evaluate(policy, [], ctx); | ||
| console.log(result); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to worker implementation
The worker function is well-structured, but lacks explicit error handling for the database operations and policy evaluation. A try/catch block would make the worker more resilient against unexpected errors.
Also, consider using a structured logger instead of console.log for the evaluation result.
export const createReleaseEvaluateWorker = () =>
new Worker<ReleaseEvaluateEvent>(Channel.ReleaseEvaluate, async (job) => {
job.log(
`Evaluating release for deployment ${job.data.deploymentId} and resource ${job.data.resourceId}`,
);
+ try {
const ctx = await createCtx(db, job.data);
if (ctx == null) {
job.log(
`Resource ${job.data.resourceId} not found for deployment ${job.data.deploymentId} and environment ${job.data.environmentId}`,
);
return;
}
const { workspaceId } = ctx.resource;
const policy = await getApplicablePolicies(db, workspaceId, job.data);
const result = await evaluate(policy, [], ctx);
- console.log(result);
+ job.log(`Evaluation result: ${JSON.stringify(result)}`);
+ } catch (error) {
+ job.log(`Error during release evaluation: ${error instanceof Error ? error.message : String(error)}`);
+ throw error; // Rethrow to let BullMQ handle the job failure
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createReleaseEvaluateWorker = () => | |
| new Worker<ReleaseEvaluateEvent>(Channel.ReleaseEvaluate, async (job) => { | |
| job.log( | |
| `Evaluating release for deployment ${job.data.deploymentId} and resource ${job.data.resourceId}`, | |
| ); | |
| const ctx = await createCtx(db, job.data); | |
| if (ctx == null) { | |
| job.log( | |
| `Resource ${job.data.resourceId} not found for deployment ${job.data.deploymentId} and environment ${job.data.environmentId}`, | |
| ); | |
| return; | |
| } | |
| const { workspaceId } = ctx.resource; | |
| const policy = await getApplicablePolicies(db, workspaceId, job.data); | |
| const result = await evaluate(policy, [], ctx); | |
| console.log(result); | |
| }); | |
| export const createReleaseEvaluateWorker = () => | |
| new Worker<ReleaseEvaluateEvent>(Channel.ReleaseEvaluate, async (job) => { | |
| job.log( | |
| `Evaluating release for deployment ${job.data.deploymentId} and resource ${job.data.resourceId}`, | |
| ); | |
| try { | |
| const ctx = await createCtx(db, job.data); | |
| if (ctx == null) { | |
| job.log( | |
| `Resource ${job.data.resourceId} not found for deployment ${job.data.deploymentId} and environment ${job.data.environmentId}`, | |
| ); | |
| return; | |
| } | |
| const { workspaceId } = ctx.resource; | |
| const policy = await getApplicablePolicies(db, workspaceId, job.data); | |
| const result = await evaluate(policy, [], ctx); | |
| job.log(`Evaluation result: ${JSON.stringify(result)}`); | |
| } catch (error) { | |
| job.log(`Error during release evaluation: ${error instanceof Error ? error.message : String(error)}`); | |
| throw error; // Rethrow to let BullMQ handle the job failure | |
| } | |
| }); |
Signed-off-by: Justin Brooks <jsbroks@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/validators/src/deployments/conditions/deployment-condition.ts (1)
14-19: Consider enumerating allowed types in doc comments.
Ensure that future maintainers easily understand which condition types are part of theDeploymentConditionunion by adding short doc comments.packages/rule-engine/src/db/get-applicable-policies.ts (2)
29-91: Add specialized tests for partial matches.
ThematchPolicyTargetForResourcefunction handles various selector combinations (deployment/environment). Consider adding test cases ensuring correct behavior when only one of the selectors is present or null.
108-128: Optimize for large policy sets.
Iterating over every policy can cause performance bottlenecks in workspaces with a high policy count. Evaluate whether indexing or more targeted queries can filter out non-matching policies earlier.packages/db/src/common.ts (1)
54-65: Well-structured column operator mapping
This record neatly encapsulates SQL operators for columns. Consider adding case-sensitive operators or others (like “In” or numeric comparisons) to handle more advanced filtering scenarios.packages/db/src/schema/deployment.ts (1)
121-135: Robust condition-building function
The logic correctly maps each condition type to an SQL expression. Consider adding a fallback or explicit error if a new or unknown condition type is encountered to prevent silent failures.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/db/src/common.ts(3 hunks)packages/db/src/schema/deployment.ts(3 hunks)packages/db/src/schema/policy.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)packages/validators/src/deployments/conditions/comparison-condition.ts(1 hunks)packages/validators/src/deployments/conditions/deployment-condition.ts(1 hunks)packages/validators/src/deployments/conditions/id-condition.ts(1 hunks)packages/validators/src/deployments/conditions/index.ts(1 hunks)packages/validators/src/deployments/conditions/name-condition.ts(1 hunks)packages/validators/src/deployments/conditions/slug-condition.ts(1 hunks)packages/validators/src/deployments/conditions/system-condition.ts(1 hunks)packages/validators/src/deployments/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/validators/src/deployments/conditions/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/policy.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/validators/src/deployments/index.tspackages/validators/src/deployments/conditions/system-condition.tspackages/validators/src/deployments/conditions/name-condition.tspackages/validators/src/deployments/conditions/slug-condition.tspackages/validators/src/deployments/conditions/id-condition.tspackages/rule-engine/src/db/get-applicable-policies.tspackages/validators/src/deployments/conditions/deployment-condition.tspackages/validators/src/deployments/conditions/comparison-condition.tspackages/db/src/common.tspackages/db/src/schema/deployment.ts
🧬 Code Definitions (4)
packages/rule-engine/src/db/get-applicable-policies.ts (3)
packages/db/src/common.ts (2)
Tx(22-22)takeFirstOrNull(15-20)packages/rule-engine/src/types.ts (1)
ReleaseRepository(70-74)packages/db/src/schema/policy.ts (2)
PolicyTarget(223-223)policy(20-36)
packages/validators/src/deployments/conditions/deployment-condition.ts (5)
packages/validators/src/deployments/conditions/comparison-condition.ts (2)
ComparisonCondition(23-30)comparisonCondition(12-21)packages/validators/src/deployments/conditions/name-condition.ts (1)
nameCondition(5-9)packages/validators/src/deployments/conditions/slug-condition.ts (1)
slugCondition(5-9)packages/validators/src/deployments/conditions/system-condition.ts (1)
systemCondition(3-7)packages/validators/src/deployments/conditions/id-condition.ts (1)
idCondition(3-7)
packages/validators/src/deployments/conditions/comparison-condition.ts (4)
packages/validators/src/deployments/conditions/name-condition.ts (2)
nameCondition(5-9)NameCondition(11-11)packages/validators/src/deployments/conditions/slug-condition.ts (2)
slugCondition(5-9)SlugCondition(11-11)packages/validators/src/deployments/conditions/system-condition.ts (2)
systemCondition(3-7)SystemCondition(9-9)packages/validators/src/deployments/conditions/id-condition.ts (2)
idCondition(3-7)IdCondition(9-9)
packages/db/src/schema/deployment.ts (2)
packages/validators/src/deployments/conditions/deployment-condition.ts (1)
DeploymentCondition(14-19)packages/db/src/common.ts (1)
ColumnOperatorFn(54-65)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (17)
packages/validators/src/deployments/index.ts (1)
21-22: Appropriate module re-exportThe addition properly exports all condition-related schemas and types, making them accessible through this module. This follows the established pattern in the file and is a clean way to organize related validation schemas.
packages/validators/src/deployments/conditions/system-condition.ts (1)
1-9: Well-structured validation schemaThe schema correctly defines a system condition with appropriate constraints. The pattern of defining a Zod schema and exporting a derived TypeScript type is a good practice for ensuring type safety throughout the application.
packages/validators/src/deployments/conditions/name-condition.ts (1)
1-11: Well-defined validation schema with appropriate operator flexibilityThe schema correctly defines a name condition and imports
columnOperatorto support multiple comparison operators, unlike other condition types that use fixed literals. This provides appropriate flexibility for name-based conditions while maintaining type safety.packages/validators/src/deployments/conditions/id-condition.ts (1)
1-9: Properly implemented ID condition schemaThe schema correctly defines an ID condition with appropriate constraints. The implementation is consistent with other condition schemas and follows the established pattern of pairing Zod schemas with derived TypeScript types.
packages/validators/src/deployments/conditions/slug-condition.ts (2)
11-11: Export statement looks good.
TheSlugConditionexport aids in strongly typed usage of this schema across the codebase.
5-9:❓ Verification inconclusive
Ensure consistent testing coverage.
Consider adding unit tests specifically for theslugConditionobject to make sure theoperatorandvaluefields are validated correctly against various input scenarios.Would you like to verify the test coverage using a command to locate relevant test files referencing
slugCondition?
🏁 Script executed:
#!/bin/bash # Searching for tests that reference slugCondition fd --type f --extension ts --exec rg 'slugCondition' {}Length of output: 358
Coverage Concern for
slugConditionThe search results only returned references within production code rather than dedicated unit tests. Please add specific unit tests for the
slugConditionobject to ensure that both theoperatorandvaluefields are validated correctly across different input scenarios.
- Verify that tests are added in the appropriate test directory.
- Ensure that edge cases (invalid/valid input combinations) are thoroughly covered.
packages/validators/src/deployments/conditions/deployment-condition.ts (1)
21-29: Schema design is aligned with best practices.
Usingz.lazyproperly prevents circular reference issues. The union of condition schemas is both elegant and extensible.packages/validators/src/deployments/conditions/comparison-condition.ts (2)
12-21: Validate nested conditions coverage.
Theconditionsarray can contain multiple types. Confirm that your test cases cover complex nested structures (AND/OR logic) to ensure correctness.
23-30: Type definition mirrors the schema accurately.
This strongly typed approach reduces potential schema drift.packages/db/src/common.ts (4)
3-3: Appropriate usage of drizzle-orm imports
These expanded imports (eq,getTableColumns,ilike,sql) are valid for building and composing queries. No concerns here.
5-5: Correct import for column operator handling
Bringing inColumnOperatorfrom@ctrlplane/validators/conditionsaligns well with the new operator-based logic.
24-28: Improved type safety for table columns
DefiningColumnKeyandColumnTypeensures better clarity and correctness when working with table columns.
32-32: Refined type parameter promotes clarity
UsingQ extends ColumnKey<T>inbuildConflictUpdateColumnsconsistently enforces valid column keys, eliminating potential mismatches.packages/db/src/schema/deployment.ts (4)
1-4: Updated imports for condition-based logic
ImportingDeploymentConditionfrom the correct validators package and the new drizzle-orm operators is consistent with the revised query-building functionality.
16-16: Comparison operator imported successfully
IncludingComparisonOperatorsets up the framework for handling compound conditions in deployment filtering.
22-22: ColumnOperatorFn usage recognized
This import from../common.jswill integrate seamlessly with the new deployment condition logic.
137-142: Graceful handling of null or empty conditions
Returningundefinedfor null or empty conditions is a clean approach, letting callers bypass condition logic when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/db/src/schema/environment.ts (1)
289-361: Metadata condition builder covers common cases thoroughly.All operators (
Null,StartsWith,EndsWith,Contains, and equality) are handled, and queries appear safely parameterized through drizzle-orm.Consider adding a more descriptive error message in
throw Error("invalid metadata conditions");to help indicate which operator or scenario was unsupported.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/db/src/schema/environment.ts(3 hunks)packages/validators/package.json(1 hunks)packages/validators/src/conditions/id-condition.ts(1 hunks)packages/validators/src/conditions/index.ts(1 hunks)packages/validators/src/conditions/name-condition.ts(1 hunks)packages/validators/src/conditions/system-condition.ts(1 hunks)packages/validators/src/deployments/conditions/comparison-condition.ts(1 hunks)packages/validators/src/deployments/conditions/deployment-condition.ts(1 hunks)packages/validators/src/deployments/conditions/index.ts(1 hunks)packages/validators/src/environments/comparison-condition.ts(1 hunks)packages/validators/src/environments/directory-condition.ts(1 hunks)packages/validators/src/environments/environment-condition.ts(1 hunks)packages/validators/src/environments/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/validators/src/environments/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/validators/src/deployments/conditions/index.ts
- packages/validators/src/deployments/conditions/deployment-condition.ts
- packages/validators/src/deployments/conditions/comparison-condition.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/validators/src/conditions/id-condition.tspackages/validators/src/conditions/index.tspackages/validators/src/environments/directory-condition.tspackages/validators/src/environments/environment-condition.tspackages/validators/src/conditions/system-condition.tspackages/validators/src/environments/comparison-condition.tspackages/validators/src/conditions/name-condition.tspackages/db/src/schema/environment.ts
🧬 Code Definitions (4)
packages/validators/src/environments/directory-condition.ts (1)
packages/validators/src/conditions/index.ts (1)
columnOperator(15-15)
packages/validators/src/environments/environment-condition.ts (5)
packages/validators/src/deployments/conditions/comparison-condition.ts (2)
ComparisonCondition(23-30)comparisonCondition(12-21)packages/validators/src/conditions/name-condition.ts (2)
NameCondition(11-11)nameCondition(5-9)packages/validators/src/conditions/system-condition.ts (2)
SystemCondition(9-9)systemCondition(3-7)packages/validators/src/environments/directory-condition.ts (2)
DirectoryCondition(11-11)directoryCondition(5-9)packages/validators/src/conditions/id-condition.ts (2)
IdCondition(9-9)idCondition(3-7)
packages/validators/src/environments/comparison-condition.ts (4)
packages/validators/src/conditions/name-condition.ts (2)
nameCondition(5-9)NameCondition(11-11)packages/validators/src/environments/directory-condition.ts (2)
directoryCondition(5-9)DirectoryCondition(11-11)packages/validators/src/conditions/system-condition.ts (2)
systemCondition(3-7)SystemCondition(9-9)packages/validators/src/conditions/id-condition.ts (2)
idCondition(3-7)IdCondition(9-9)
packages/validators/src/conditions/name-condition.ts (1)
packages/validators/src/conditions/index.ts (1)
columnOperator(15-15)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (19)
packages/validators/package.json (1)
55-57: New "environments" Export AddedThis new export block for
"./environments"correctly exposes both the TypeScript definitions (./src/environments/index.ts) and the compiled JavaScript implementation (./dist/environments/index.js). This change enhances the module interface in line with the PR objectives by making additional functionality available to package consumers.Please verify that the corresponding files exist at the specified paths and that they integrate well with other parts of the system.
packages/validators/src/conditions/name-condition.ts (1)
1-11: Looks good - well-structured schema definition.The implementation follows a clean pattern for condition schema definition with appropriate validations for each field. The use of
columnOperatorfrom the shared index file maintains consistency with other condition types.packages/validators/src/conditions/id-condition.ts (1)
1-9: Looks good - straightforward ID condition schema.The implementation constrains the operator to only "equals", which is appropriate for ID conditions that typically only need exact matching. This differs from the approach in name-condition.ts but seems intentional and well-reasoned.
packages/validators/src/conditions/system-condition.ts (1)
1-9: Looks good - clean system condition implementation.The implementation follows the same pattern as id-condition.ts, restricting the operator to only "equals" which makes sense for system identification. The schema structure is clear and appropriately constrained.
packages/validators/src/conditions/index.ts (1)
5-6: Good additions to the index exports.These exports properly integrate the new condition types into the module's public API, following the established pattern of re-exporting from individual files.
packages/validators/src/environments/directory-condition.ts (3)
1-4: No issues with imports.The imports are straightforward and appropriate for the new condition schema.
5-9: Schema definition looks good.The
directoryConditionschema correctly enforces atypeliteral of"directory", usescolumnOperator, and requires a stringvalue.
11-11: No concerns with type inference.Exporting
DirectoryConditionusingz.infermatches the schema properly.packages/validators/src/environments/environment-condition.ts (3)
1-15: Imports and type references are consistent.No issues found with the organization of type imports and references for the sub-condition schemas.
16-22: Union type is well-defined.Having
EnvironmentConditionas a union of supported condition types cleanly captures each variant of environment conditions.
24-34: Zod lazy union is appropriate.Defining the
environmentConditionschema usingz.lazyis a good design for recursive or union-based validations. Looks great.packages/validators/src/environments/comparison-condition.ts (3)
1-13: No issues with imports.Imports for condition types and Zod are straightforward and neatly organized.
14-29: Lazy-loaded schema is implemented correctly.The
comparisonConditionschema leveragesz.lazyand a union of various condition schemas effectively. Good use of optionalnotwith a default offalse.
31-42: Strong alignment between schema and type.The
ComparisonConditiontype matches the Zod schema nicely, ensuring clarity and consistency.packages/db/src/schema/environment.ts (5)
1-2: New type imports are appropriate.Using
MetadataConditionandEnvironmentConditionreferences is aligned with the updated validators.
6-6: No issues with added imports.The additional imports for
drizzle-ormfunctions (likeand,eq, etc.) appear correct and standard for query construction.
22-25: Imports for operators look correct.The introduction of
ComparisonOperatorandMetadataOperatorfrom@ctrlplane/validators/conditionsis consistent with the new condition logic.
362-390: Composite condition building is well-structured.
- Each condition type is correctly transformed into SQL constraints.
- The handling of empty
conditionsreturningFALSEis a logical approach forcomparisonCondition.
394-398:✅ Verification successful
Behaves as expected when condition is null or empty.
Returning
undefinedifconditionis empty or null means no WHERE clause, thereby matching all environments. Confirm that this behavior is intentional.Would you like to verify that this default (“match all”) logic is correct in contexts where no condition is provided?
🏁 Script executed:
#!/bin/bash # Explanation: Search usage of environmentMatchSelector calls that may pass an undefined or empty condition. # We'll look for references to environmentMatchSelector: rg -A 5 "environmentMatchSelector"Length of output: 992
Match-All Filter Logic Verified
The implementation inpackages/db/src/schema/environment.tscorrectly returnsundefinedwhenconditionis null or empty, ensuring that no additional WHERE clause is added (i.e., all environments are matched). Our search confirms that this behavior is consistently and intentionally relied upon—in particular, the usage inpackages/rule-engine/src/db/get-applicable-policies.tsindicates that an undefined condition is expected and handled appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/db/get-applicable-policies.ts (2)
10-92: Confirm handling of multiple query results and unusedresourceId.
takeFirstOrNullreturnsnullif more than one matching row is found. Please verify if this strict "single row or nothing" approach matches your expected policy logic, or if you might prefer to select the first matching row instead.- The function signature accepts a
resourceId(part ofReleaseRepository) but does not utilize it in the matching logic. Confirm that this omission is intentional and that you do not need resource-level matching.
93-128: Consider performance optimizations and matched-target reporting.
- Iterating through every policy and retrieving matches with
Promise.allcould become costly for large workspaces. If performance becomes an issue, consider partial queries or caching.- Currently, if any target matches, this code returns the entire policy without indicating which target matched. If you need insight into specific matches, store or return the matched target(s) for downstream usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/src/schema/policy.ts(4 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/policy.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/db/get-applicable-policies.ts
🧬 Code Definitions (1)
packages/rule-engine/src/db/get-applicable-policies.ts (3)
packages/db/src/common.ts (2)
Tx(22-22)takeFirstOrNull(15-20)packages/rule-engine/src/types.ts (1)
ReleaseRepository(70-74)packages/db/src/schema/policy.ts (2)
PolicyTarget(228-228)policy(23-39)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (1)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
1-9: Imports and initial setup look good.No immediate issues identified in these lines. The imports are consistent with the usage throughout the file, and type references appear to be correctly organized.
Summary by CodeRabbit
New Features
ReleaseEvaluatefor enhanced event handling.Refactor/Improvements
Chores