init new policy engine #411
Conversation
WalkthroughThis change introduces a new package for a rule engine along with complete TypeScript and ESLint configurations. Multiple source files are added to define the core rule engine, including its evaluation logic and release management utilities. A variety of deployment rules are implemented along with corresponding tests, and the package’s public API is consolidated in an index file. In addition, modifications were made to the event worker and database schemas to support the new rule engine functionalities and event validations. Changes
Sequence Diagram(s)sequenceDiagram
participant Job as RuleEngineEvaluationJob
participant Worker as RuleEngineEvaluationWorker
participant DB as Database
participant Engine as RuleEngine
Job->>Worker: Trigger evaluation event
Worker->>DB: Retrieve deployment resource context & release candidates
DB-->>Worker: Return context and candidates
Worker->>Engine: Evaluate candidates with rules
Engine-->>Worker: Return evaluation result
Worker->>DB: Log result and update status
sequenceDiagram
participant Engine as RuleEngine
participant Rule as DeploymentResourceRule
participant Releases as Releases Collection
Engine->>Rule: Apply rule.filter(context, candidates)
Rule-->>Engine: Return allowed releases
Engine->>Releases: Call selectFinalRelease(allowedReleases)
Releases-->>Engine: Return final release decision
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: 0
🧹 Nitpick comments (5)
packages/policy-engine/tsconfig.json (1)
1-11: Consider excluding the 'dist' folder from compilation.
Since generated output is emitted todist, you likely don’t need to re-compile those build artifacts."exclude": [ "node_modules" + "dist" ]packages/policy-engine/package.json (1)
1-36: Consider removing the 'dist' folder in your clean script.
If you want a fully fresh build environment, includedistin your clean command."scripts": { - "clean": "rm -rf .turbo node_modules", + "clean": "rm -rf .turbo node_modules dist", ... },packages/policy-engine/src/deployment-concurency-policy.ts (1)
46-50: Add curly braces to improve readability.
A single-lineifwith a multi-line object can be confusing. Consider adding braces for clarity.- if (runningDeployments >= this.concurrencyLimit) - return { - allowedReleases: [], - reason: `Concurrency limit reached (${runningDeployments} of ${this.concurrencyLimit}). No new deployments allowed.`, - }; + if (runningDeployments >= this.concurrencyLimit) { + return { + allowedReleases: [], + reason: `Concurrency limit reached (${runningDeployments} of ${this.concurrencyLimit}). No new deployments allowed.`, + }; + }packages/policy-engine/src/types.ts (1)
23-28: Consider makingdesiredReleaseIdoptional if it may be unset.If users need to handle scenarios where no desired ID is provided, consider making
desiredReleaseIdoptional or nullable. For example:-export type DeploymentResourceContext = { - desiredReleaseId: string; +export type DeploymentResourceContext = { + desiredReleaseId?: string;packages/policy-engine/src/policy-engine.ts (1)
47-71: Consider creating a copy of the array before sorting.Sorting modifies the original array in-place. If any downstream logic expects the original array order, consider using:
-const sorted = candidates.sort( +const sorted = [...candidates].sort( (a, b) => b.createdAt.getTime() - a.createdAt.getTime(), );
📜 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 (6)
packages/policy-engine/eslint.config.js(1 hunks)packages/policy-engine/package.json(1 hunks)packages/policy-engine/src/deployment-concurency-policy.ts(1 hunks)packages/policy-engine/src/policy-engine.ts(1 hunks)packages/policy-engine/src/types.ts(1 hunks)packages/policy-engine/tsconfig.json(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/policy-engine/src/deployment-concurency-policy.tspackages/policy-engine/src/types.tspackages/policy-engine/src/policy-engine.ts
🧬 Code Definitions (1)
packages/policy-engine/src/deployment-concurency-policy.ts (1)
packages/policy-engine/src/types.ts (4)
DeploymentResourcePolicy(47-53)DeploymentResourceContext(23-28)Release(1-11)DeploymentResourcePolicyResult(33-36)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
packages/policy-engine/eslint.config.js (1)
1-10: Looks good.
No issues found in your ESLint config.packages/policy-engine/src/deployment-concurency-policy.ts (2)
1-5: No issues with imports.
They appear correct and well-organized.
22-38:✅ Verification successful
Verify usage of
resourceIdwithreleaseJobTrigger.id.
Double-check thatresourceIdindeed matchesschema.releaseJobTrigger.id. If you're intending to link a different resource table, you might need a different column reference or join condition.
🏁 Script executed:
#!/bin/bash # This script searches for references to eq(schema.releaseJobTrigger.id # to verify that resourceId is correctly referencing the releaseJobTrigger ID. rg -A 5 $'eq\\(schema\\.releaseJobTrigger\\.id'Length of output: 567
Resource ID usage is correctly implemented.
The execution of the search confirms that the code inpackages/policy-engine/src/deployment-concurency-policy.tsuseseq(schema.releaseJobTrigger.id, resourceId)as intended. There is no discrepancy between the referenced column and the passedresourceId, so no changes are needed.packages/policy-engine/src/types.ts (6)
1-11: Well-structured type definition forRelease.This definition looks good. The nested type for
versionis clear and organizes metadata effectively.
13-16: Simple and clearDeploymenttype.No issues noted.
18-21: ConsistentResourcetype.Matches the structure of
Deployment, maintaining consistency.
30-36: ClearDeploymentResourcePolicyResulttype.This succinctly communicates the allowed releases and any reason for filtering.
38-42: Concise structure forDeploymentResourceSelectionResult.No issues noted; it provides all necessary info on final selection outcomes.
44-53: InterfaceDeploymentResourcePolicyis appropriately designed.The policy name and filter method signature are straightforward and flexible.
packages/policy-engine/src/policy-engine.ts (5)
1-6: Imports are well-organized.Type-only import usage is a good practice for clarity and performance.
8-10: Helpful doc comment.Keeps the codebase maintainable by explaining the engine's purpose at a glance.
11-13: Constructor is straightforward.Injecting policies through the constructor offers easy extensibility.
14-31: Robustevaluatemethod logic.Sequentially applying each policy and aborting early if no candidates remain is optimal.
33-45: Clear behavior for final check.Returning a clear message if no release is chosen after applying all policies.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
packages/rule-engine/src/rules/time-window-rule.ts (3)
8-24: Helpful documentation.The JSDoc block provides a clear overview. Consider adding a timezone usage example in the snippet for completeness.
28-43: Validate hour boundaries.Currently, there's no validation to ensure
startHourandendHourfall within [0, 24). Consider adding checks to prevent unexpected values.constructor( private options: { startHour: number; endHour: number; days?: string[]; timezone?: string; }, ) { + if (options.startHour < 0 || options.startHour > 23) { + throw new Error("startHour must be between 0 and 23"); + } + if (options.endHour < 0 || options.endHour > 23) { + throw new Error("endHour must be between 0 and 23"); + } }
45-104: Consider finer-grained filtering and stricter typing.
- Minutes are not checked, which could matter for half-hour or quarter-hour time restrictions.
- The
days.includes(dayOfWeek as any)approach obscures type safety.packages/rule-engine/src/rules/version-regex-rule.ts (1)
48-49: Consider a more comprehensive approach for detecting pre-release versions.Using a single regex for “alpha|beta|rc|dev|test” might overlook other common prerelease tags like “preview,” “nightly,” or “canary.” If you need full semantic versioning support (including build metadata), consider leveraging a semantic version parser library or expanding the regex.
packages/rule-engine/src/rules/maintenance-window-rule.ts (1)
44-59: Be mindful of time zones and partial overlaps.The logic straightforwardly checks if the current UTC time is between the maintenance window’s start and end. If end-users’ local time differs significantly (e.g., scheduled maintenance in a different timezone) or partial overlaps matter, you may need additional logic or clarity. Otherwise, this approach is correct for strict UTC-based usage.
packages/rule-engine/src/rules/resource-concurency-rule.ts (1)
1-1: Consider renaming the file to correct the spelling.
The file name is spelled “resource-concurency-rule” instead of “resource-concurrency-rule.” Renaming would help maintain consistency and clarity across the codebase.packages/rule-engine/src/types.ts (2)
30-36: Encourage explicit reasons for denied deployments.
For better traceability, encourage using thereasonfield wheneverallowedReleasesis empty. This can simplify debugging rule failures, especially when multiple rules are chained together.
44-53: Allow for asynchronous or advanced filtering logic in future expansions.
TheDeploymentResourceRuleinterface is straightforward and currently sufficient. If more nuanced filtering steps arise (e.g., multiple passes or user interactions), consider a more flexible structure (e.g., chaining, partial approvals).packages/rule-engine/src/rules/approval-required-rule.ts (3)
23-34: Validate constructor parameters to avoid accidental misconfiguration.
Currently, the code allows patterns to be undefined, which can be valid. However, ensure theapprovalMetadataKeyis always non-empty and that arequiredApproversvalue makes sense (e.g., not negative).
36-46: Consider logging rejections for easier auditing.
When the environment does not match the pattern, this rule silently passes or rejects deployments. Logging or tracing might help teams understand why a deployment was allowed or skipped.
65-86: Offer the possibility to track individual approvals.
Currently, the rule checks the total count of approvers as a comma-separated list. If you need to ensure unique or specific user approvals, consider normalizing the metadata (e.g., storing approvals as an array) and verifying uniqueness.packages/rule-engine/src/rule-engine.ts (1)
47-71: Slightly simplify the desired release selection logic.You could streamline the block that checks
desiredReleaseIdby using a singlefindcall. This eliminates redundant checks and maps.Here is a small refactor example:
- if ( - context.desiredReleaseId && - candidates.map((r) => r.id).includes(context.desiredReleaseId) - ) - return candidates.find((r) => r.id === context.desiredReleaseId); + if (context.desiredReleaseId) { + const desired = candidates.find((r) => r.id === context.desiredReleaseId); + if (desired) return desired; + }
📜 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 (13)
packages/rule-engine/eslint.config.js(1 hunks)packages/rule-engine/package.json(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/approval-required-rule.ts(1 hunks)packages/rule-engine/src/rules/gradual-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/maintenance-window-rule.ts(1 hunks)packages/rule-engine/src/rules/resource-concurency-rule.ts(1 hunks)packages/rule-engine/src/rules/time-window-rule.ts(1 hunks)packages/rule-engine/src/rules/version-regex-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)packages/rule-engine/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/rule-engine/src/rules/index.ts
- packages/rule-engine/package.json
- packages/rule-engine/tsconfig.json
🧰 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/rules/version-regex-rule.tspackages/rule-engine/src/rules/time-window-rule.tspackages/rule-engine/src/rules/gradual-rollout-rule.tspackages/rule-engine/src/rules/resource-concurency-rule.tspackages/rule-engine/src/rules/maintenance-window-rule.tspackages/rule-engine/src/rules/approval-required-rule.tspackages/rule-engine/src/types.tspackages/rule-engine/src/rule-engine.ts
🧬 Code Definitions (2)
packages/rule-engine/src/rules/gradual-rollout-rule.ts (2)
packages/rule-engine/src/rules/index.ts (1)
GradualRolloutRule(3-3)packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(47-53)DeploymentResourceContext(23-28)Release(1-11)DeploymentResourceRuleResult(33-36)
packages/rule-engine/src/rules/approval-required-rule.ts (2)
packages/rule-engine/src/rules/index.ts (1)
ApprovalRequiredRule(2-2)packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(47-53)DeploymentResourceContext(23-28)Release(1-11)DeploymentResourceRuleResult(33-36)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (8)
packages/rule-engine/src/index.ts (1)
1-5: Well-structured aggregator file.This file provides a clear and concise aggregation of exports, improving the discoverability of package features. No issues noted.
packages/rule-engine/eslint.config.js (1)
1-13: Lint configuration looks good.Merging base configs and selective rule overrides provides flexibility. The disabling of
@typescript-eslint/require-awaitis acceptable if your asynchronous patterns are sufficiently tested.packages/rule-engine/src/rules/version-regex-rule.ts (1)
61-66: Verify the effect of an empty result set on deployment pipelines.Here, an empty
allowedReleasesarray indicates no valid versions remain. Ensure that the broader pipeline handles this gracefully (e.g., failing fast or skipping the step). If you intend to bail out entirely, this is fine; otherwise, you might want to provide a fallback release or raise an explicit error.packages/rule-engine/src/rules/gradual-rollout-rule.ts (1)
70-89: Implementation looks solid otherwise.Aside from the missing time cutoff, the logic thoroughly prevents deployments when maximum deployments within the window are exceeded. Once the cutoff is added, the rule should work as intended for gradual rollouts.
packages/rule-engine/src/rule-engine.ts (4)
11-13: Constructor is straightforward and clear.
No issues with the constructor logic. Instantiatingrulesas a private member is appropriate for encapsulating their usage within the class.
33-45: Return shape is consistent.The fallback scenario where no release is selected returns a consistent JSON-like structure. This is good for consumer code. Keep an eye out for places in the codebase where no final release might cause unexpected behavior.
1-6:✅ Verification successful
Consider the import file extension consistency.
You're importing from
./types.jsin a TypeScript file. Iftypes.tsexists, consider referencing it to maintain consistency, unless you specifically intend to reference a compiled.js.
🏁 Script executed:
#!/bin/bash # Verify the correct file extension for the 'types' import and see if `types.ts` exists. fd -E 'node_modules' -e ts --exec grep -Hn 'export .* from.*types' {}Length of output: 137
Resolution on Import File Extension Consistency
The verification shows that in
packages/rule-engine/src/index.tsthe export is also referencing"./types.js", suggesting that the project consistently uses the compiled.jsextension in these imports. Since there is no evidence of atypes.tsfile intended for direct TypeScript imports, the current usage appears intentional and consistent across the codebase.
14-31:❓ Verification inconclusive
Validate early exit conditions for empty or invalid candidate lists.
Currently, if
context.availableReleasesis empty, the loop is skipped, and the final selection check handles the empty list. This logic is sensible, but ensure your downstream usage expects an empty result if no releases are available from the start or if all are disqualified by the first rule.
🏁 Script executed:
#!/bin/bash # Attempt to confirm that no usage in the codebase expects a non-empty candidate list after evaluate(). # We'll search for calls to evaluate() and see if they handle/assume a non-empty result. rg -A 3 "evaluate\(.*\)"Length of output: 26
Below is a revised review comment. Since the initial search for evaluate call sites returned no results, please run a more targeted search (or manually verify) that downstream usage—whether explicitly or indirectly via the produced DeploymentResourceSelectionResult—is indeed robust against an empty candidate list. In particular, confirm that clients of evaluate() can correctly handle a result where candidateReleases remains empty when no releases are available or have been disqualified.
- The early exit logic seems correct if an empty candidate list is allowed.
- However, we haven’t been able to confirm via automated search that all consumers of evaluate() expect or properly handle an empty list.
- Please verify manually or via additional tooling that, for example, when context.availableReleases is initially empty or if a rule disqualifies all candidates, downstream processing (perhaps in the calling service) does not assume a non-empty list.
| private async getRunningCount(resourceId: string): Promise<number> { | ||
| return db | ||
| .select({ count: count() }) | ||
| .from(schema.job) | ||
| .innerJoin( | ||
| schema.releaseJobTrigger, | ||
| eq(schema.job.id, schema.releaseJobTrigger.jobId), | ||
| ) | ||
| .where( | ||
| and( | ||
| eq(schema.releaseJobTrigger.id, resourceId), | ||
| inArray(schema.job.status, [JobStatus.InProgress, JobStatus.Pending]), | ||
| ), | ||
| ) | ||
|
|
||
| .then((r) => r[0]?.count ?? 0); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that releaseJobTrigger.id correctly references the resource identifier.
In the getRunningCount method, the filter uses eq(schema.releaseJobTrigger.id, resourceId). Confirm that id in releaseJobTrigger indeed maps to a resource ID rather than an internal trigger record ID.
🏁 Script executed:
#!/bin/bash
# Description: Check usages and definitions of `releaseJobTrigger` to ensure `id` is the resource's identifier.
# 1) Search for the schema definition of `releaseJobTrigger`
rg -A 15 "releaseJobTrigger"
# 2) Look for references to `releaseJobTrigger.id` usage
rg -A 5 "releaseJobTrigger.id"Length of output: 81970
Action Required: Update the Filter Field in getRunningCount
Based on our review of the schema usage across the codebase, it appears that the field releaseJobTrigger.id is the primary key for the trigger record—not the resource identifier. In other parts of the code (for example, in resource/delete.ts), the resource identifier is referenced through releaseJobTrigger.resourceId. Please verify this mapping and update the filter in the getRunningCount method to use releaseJobTrigger.resourceId instead of releaseJobTrigger.id if the intention is to retrieve counts based on a resource identifier.
- Location:
packages/rule-engine/src/rules/resource-concurency-rule.ts(lines 31-47) - Current Code:
.where( and( eq(schema.releaseJobTrigger.id, resourceId), inArray(schema.job.status, [JobStatus.InProgress, JobStatus.Pending]), ), )
- Suggested Update:
.where( and( eq(schema.releaseJobTrigger.resourceId, resourceId), inArray(schema.job.status, [JobStatus.InProgress, JobStatus.Pending]), ), )
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rule-engine/src/rules/gradual-rollout-rule.ts (1)
53-71:⚠️ Potential issueCritical bug: Missing time window filter in database query
The database query retrieves all successful deployments for a release without filtering by the cutoff time, which defeats the purpose of the time window restriction.
Import the
gtefunction and add a condition to theWHEREclause:-import { and, count, eq } from "@ctrlplane/db"; +import { and, count, eq, gte } from "@ctrlplane/db"; .where( and( eq(schema.releaseJobTrigger.versionId, releaseId), eq(schema.job.status, JobStatus.Successful), + gte(schema.job.createdAt, cutoffTime) ), )
🧹 Nitpick comments (4)
packages/rule-engine/src/rules/maintenance-window-rule.ts (1)
43-47: Small readability improvementThe
isActivevariable assignment is only used once in the return statement, making it somewhat redundant.- const activeWindows = this.maintenanceWindows.filter((window) => { - const isActive = now >= window.start && now <= window.end; - return isActive; - }); + const activeWindows = this.maintenanceWindows.filter((window) => + now >= window.start && now <= window.end + );packages/rule-engine/src/rules/version-regex-rule.ts (1)
49-49: Improve pre-release detection regexThe current regex for detecting pre-releases is limited and might miss some common pre-release patterns. Consider enhancing it to cover more cases according to SemVer specification.
- isPreRelease = /-(alpha|beta|rc|dev|test)/.test(tag); + isPreRelease = /-(alpha|beta|rc|dev|test|pre|preview|nightly)(\.\d+)?$/.test(tag);packages/rule-engine/src/rules/gradual-rollout-rule.ts (2)
133-133: Redundant calculation of current percentageThe current percentage is calculated twice - once in the eligibility check and again when preparing the reason message.
- const isEligible = this.isResourceEligibleForDeterministicRollout( - ctx.resource.id, - ctx.desiredReleaseId, - daysSinceRelease - ); - - if (!isEligible) { - // Filter out the desired release - const filteredReleases = currentCandidates.filter( - (r) => r.id !== ctx.desiredReleaseId, - ); - - const { percentagePerDay } = this.options.deterministicRollout; - const currentPercentage = Math.min(percentagePerDay * daysSinceRelease, 100); + const { percentagePerDay } = this.options.deterministicRollout; + const currentPercentage = Math.min(percentagePerDay * daysSinceRelease, 100); + + const isEligible = this.isResourceEligibleForDeterministicRollout( + ctx.resource.id, + ctx.desiredReleaseId, + daysSinceRelease + ); + + if (!isEligible) { + // Filter out the desired release + const filteredReleases = currentCandidates.filter( + (r) => r.id !== ctx.desiredReleaseId, + );
119-139: Suggestion: Consider extracting the deterministic rollout logicThe deterministic rollout check contains several steps and is complex enough to warrant extraction into a dedicated method to improve readability.
Consider refactoring to a method like:
private handleDeterministicRollout( ctx: DeploymentResourceContext, currentCandidates: Release[], desiredRelease: Release ): DeploymentResourceRuleResult | null { if (!this.options.deterministicRollout) { return null; // No deterministic rollout configured } const daysSinceRelease = this.getDaysSinceRelease(desiredRelease); const { percentagePerDay } = this.options.deterministicRollout; const currentPercentage = Math.min(percentagePerDay * daysSinceRelease, 100); const isEligible = this.isResourceEligibleForDeterministicRollout( ctx.resource.id, ctx.desiredReleaseId, daysSinceRelease ); if (!isEligible) { // Filter out the desired release const filteredReleases = currentCandidates.filter( (r) => r.id !== ctx.desiredReleaseId, ); return { allowedReleases: filteredReleases, reason: `Resource not eligible for release yet. Currently at ${currentPercentage}% rollout (${daysSinceRelease} days since release).`, }; } return null; // Eligible for rollout }Then you can simplify the main filter method by calling this method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rule-engine/src/rules/gradual-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/maintenance-window-rule.ts(1 hunks)packages/rule-engine/src/rules/version-regex-rule.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/rules/maintenance-window-rule.tspackages/rule-engine/src/rules/version-regex-rule.tspackages/rule-engine/src/rules/gradual-rollout-rule.ts
🧬 Code Definitions (1)
packages/rule-engine/src/rules/version-regex-rule.ts (2)
packages/rule-engine/src/rules/index.ts (1)
VersionRegexRule(6-6)packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(47-53)DeploymentResourceContext(23-28)Release(1-11)DeploymentResourceRuleResult(33-36)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
packages/rule-engine/src/rules/maintenance-window-rule.ts (1)
1-59: LGTM! Well-implemented maintenance window rule.The
MaintenanceWindowRuleclass effectively implements theDeploymentResourceRuleinterface to block deployments during scheduled maintenance periods. The code is well-documented with clear examples, and the logic for detecting active maintenance windows is straightforward and effective.packages/rule-engine/src/rules/version-regex-rule.ts (2)
1-70: The version regex rule implementation looks solidThe
VersionRegexRuleclass provides a flexible solution for filtering releases based on version patterns and pre-release identifiers. The implementation offers good configurability with options to allow non-matching releases and to block pre-releases.
68-68:⚠️ Potential issueCritical bug: Incorrect property name in return object
There's a typo in the return statement that would cause the rule to return an incorrect property name (
allowedReleasesis misspelled asallowedReleases), which would break the rule engine's functionality.- return { allowedReleases }; + return { allowedReleases };Likely an incorrect or invalid review comment.
packages/rule-engine/src/rules/gradual-rollout-rule.ts (2)
1-162: Well-designed gradual rollout implementation with deterministic capabilityThe
GradualRolloutRuleprovides a comprehensive solution for controlling deployment pace with both time-window limits and optional deterministic rollout patterns. The deterministic rollout feature using hash-based distribution is particularly well-implemented.
92-92:❓ Verification inconclusive
Potential issue with hash value calculation
The current implementation uses modulo 100 which provides an even distribution, but might not handle edge cases where certain resources consistently get higher values.
Consider verifying the distribution characteristics of the hash calculation:
🏁 Script executed:
#!/bin/bash # Test the distribution of hash values across sample resources python3 -c ' import hashlib import sys def get_hash_value(resource_id, release_id, seed): hash_input = f"{resource_id}-{release_id}-{seed}" hash_hex = hashlib.sha256(hash_input.encode()).hexdigest() return int(hash_hex[:8], 16) % 100 seed = "test-seed" release_id = "test-release" # Generate distribution for 1000 resources distribution = [0] * 100 for i in range(1000): resource_id = f"resource-{i}" hash_val = get_hash_value(resource_id, release_id, seed) distribution[hash_val] += 1 # Print distribution stats print(f"Min: {min(distribution)}") print(f"Max: {max(distribution)}") print(f"Avg: {sum(distribution)/len(distribution)}") print(f"Distribution uniformity (higher is better): {min(distribution)/max(distribution):.2f}") 'Length of output: 837
Action Required: Reassess Hash Distribution for Edge Case Scenarios
After executing the distribution test on 1,000 sample resources, we observed:
- Minimum count: 3
- Maximum count: 16
- Average count: 10
- Uniformity ratio (min/max): 0.19
While the modulo operation on the first 8 hex digits does spread values across buckets, the variability (with some buckets receiving as few as 3 and others up to 16) could lead to edge cases where certain resources consistently yield higher values. Please verify whether this degree of variation is acceptable for your gradual rollout logic. If it poses a risk, consider adjusting the hash calculation (for example, by utilizing a larger portion of the hash or an alternative method) to achieve a more balanced distribution.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rule-engine/src/rules/index.ts (1)
1-7: Resolve the spelling discrepancy in file reference.
The filenameresource-concurency-rule.jsappears to have a spelling mistake. Consider renaming it to maintain clarity and consistency (e.g.,resource-concurrency-rule.js).-export { ResourceConcurrencyRule } from "./resource-concurency-rule.js"; +export { ResourceConcurrencyRule } from "./resource-concurrency-rule.js";packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
36-57: Consider handling concurrent deployment triggers.
If multiple deployment processes querygetLastSuccessfulDeploymentTimearound the same time, they might pass the cooldown check concurrently, bypassing the intended restriction. To mitigate this, you could introduce transaction-based row locks or additional coordination logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/version-cooldown-rule.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/rules/index.tspackages/rule-engine/src/rules/version-cooldown-rule.ts
🧬 Code Definitions (1)
packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
packages/rule-engine/src/rules/index.ts (1)
VersionCooldownRule(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
1-5: File initialization check.
The imports and initial declarations appear consistent with the existing project structure.
| async filter( | ||
| ctx: DeploymentResourceContext, | ||
| currentCandidates: Release[], | ||
| ): Promise<DeploymentResourceRuleResult> { | ||
| // Get the time of the last successful deployment | ||
| const lastDeploymentTime = await this.getLastSuccessfulDeploymentTime( | ||
| ctx.deployment.id, | ||
| ctx.resource.id, | ||
| ); | ||
|
|
||
| // If there's no previous deployment, cooldown doesn't apply | ||
| if (lastDeploymentTime == null) | ||
| return { allowedReleases: currentCandidates }; | ||
|
|
||
| // Check if the cooldown period has elapsed | ||
| const cooldownMs = this.options.cooldownMinutes * 60 * 1000; | ||
| const earliestAllowedTime = new Date( | ||
| lastDeploymentTime.getTime() + cooldownMs, | ||
| ); | ||
|
|
||
| const now = new Date(); | ||
| if (now > earliestAllowedTime) { | ||
| return { allowedReleases: currentCandidates }; | ||
| } | ||
|
|
||
| // Calculate remaining cooldown time | ||
| const remainingMs = earliestAllowedTime.getTime() - now.getTime(); | ||
| const remainingMinutes = Math.ceil(remainingMs / (60 * 1000)); | ||
| const remainingHours = Math.floor(remainingMinutes / 60); | ||
| const remainingMins = remainingMinutes % 60; | ||
|
|
||
| const remainingTimeStr = | ||
| remainingHours > 0 | ||
| ? `${remainingHours} hour${remainingHours > 1 ? "s" : ""}${remainingMins > 0 ? ` ${remainingMins} minute${remainingMins > 1 ? "s" : ""}` : ""}` | ||
| : `${remainingMinutes} minute${remainingMinutes > 1 ? "s" : ""}`; | ||
|
|
||
| return { | ||
| allowedReleases: [], | ||
| reason: `Deployment cooldown period not yet elapsed. Please wait ${remainingTimeStr} before deploying again.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Edge case consideration for zero or negative cooldown values.
A zero or negative cooldownMinutes will allow immediate or past-time deployments, which might be unintentional. Confirm whether additional validation is needed to disallow non-positive cooldowns.
Would you like me to help verify this scenario in the wider codebase by checking if negative or zero cooldowns are used anywhere else?
🏁 Script executed:
#!/bin/bash
# Searching code references for version-cooldown-rule usage to see if negative or zero cooldown is possible.
rg -A 5 'VersionCooldownRule'Length of output: 1091
Action: Consider Adding Validation for Non-Positive Cooldown Values
Our search confirms that current usage of VersionCooldownRule only specifies positive values (e.g., cooldownMinutes: 1440), so no immediate issues are observed. However, note that the filter method does not check whether cooldownMinutes is greater than zero. If someone accidentally configures this value as zero or negative, the code would allow deployments immediately or in the past, which is likely unintended.
- Consider: Adding validation (e.g., in the constructor) to ensure that
cooldownMinutesis a positive number. - Verify: Whether the design should allow non-positive cooldown values or if this is an oversight.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/rule-engine/src/rules/gradual-rollout-rule.ts (1)
1-99:⚠️ Potential issueCritical bug: missing cutoff time in query
This is the same issue flagged in a previous review. AlthoughcutoffTimeis computed, it is never used in the.where(...)clause, so older successful deployments still count.Apply the following diff to fix it:
.where( and( eq(schema.releaseJobTrigger.versionId, releaseId), eq(schema.job.status, JobStatus.Successful), + gte(schema.job.createdAt, cutoffTime) ), )packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
22-35: 🛠️ Refactor suggestionValidate zero/negative cooldown edge cases.
As previously noted, the code does not restrict negative or zero values for
cooldownMinutes. A zero or negative value could inadvertently allow immediate deployments, countering any intended cooldown. Consider adding validation to reject or handle non-positive cooldowns.+ if (options.cooldownMinutes <= 0) { + throw new Error("Cooldown period must be a positive number."); + }
🧹 Nitpick comments (5)
packages/rule-engine/src/rules/index.ts (1)
1-7: Consider correcting the filename spelling of "resource-concurency-rule".
The word "concurrency" is misspelled. This might cause confusion or inconsistencies later on.packages/rule-engine/src/rules/approval-required-rule.ts (1)
53-119: Implementation logic is sound
• The rule correctly checks patterns for environment, resource, and version, avoiding unnecessary approval checks when they do not match.
• For releases requiring approval, it properly looks up metadata and applies therequiredApproversthreshold if configured.
• Returning a reason when no releases remain clarifies why approvals are missing.Overall, this rule is coherent and follows the expected interface contract. Consider adding unit tests to verify edge cases (e.g., multiple approvers, empty
approvalMetadataKey, etc.).packages/rule-engine/src/rules/previous-deploy-status-rule.ts (1)
107-167: Rule logic is properly enforced
• The filter method correctly checks for the desired release, retrieves its successful deployments, and compares against the required threshold.
• Good approach returning a clear reason when filtering out the desired release.
• Consider adding tests to verify boundary conditions (e.g., zero dependent environments, partial meets, or mismatch in environment IDs).packages/rule-engine/src/rules/gradual-rollout-rule.ts (1)
133-189: Filter logic meets gradual rollout needs
• The code properly checks for deterministic eligibility, skipping release when under the allowed percentage.
• The time-window-based check ensures a paced rollout (once the missing cutoff time condition is added).
• Returning a reason clarifies the gating conditions for each case.Recommend adding tests for the deterministic rollout scenario and time-window gating to confirm day-based percentages and daily expansions.
packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
80-88: Consider customizing the default cooldown.While 24 hours (1440 minutes) is a sensible default, some deployments might require shorter or longer cooldowns. Encourage project-specific customization or a shorter default for more frequent deployments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/rule-engine/src/rules/approval-required-rule.ts(1 hunks)packages/rule-engine/src/rules/gradual-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/maintenance-window-rule.ts(1 hunks)packages/rule-engine/src/rules/previous-deploy-status-rule.ts(1 hunks)packages/rule-engine/src/rules/resource-concurency-rule.ts(1 hunks)packages/rule-engine/src/rules/time-window-rule.ts(1 hunks)packages/rule-engine/src/rules/version-cooldown-rule.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/rules/resource-concurency-rule.tspackages/rule-engine/src/rules/maintenance-window-rule.tspackages/rule-engine/src/rules/time-window-rule.tspackages/rule-engine/src/rules/gradual-rollout-rule.tspackages/rule-engine/src/rules/approval-required-rule.tspackages/rule-engine/src/rules/previous-deploy-status-rule.tspackages/rule-engine/src/rules/version-cooldown-rule.tspackages/rule-engine/src/rules/index.ts
🧬 Code Definitions (2)
packages/rule-engine/src/rules/approval-required-rule.ts (2)
packages/rule-engine/src/rules/index.ts (1)
ApprovalRequiredRule(2-2)packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(47-53)DeploymentResourceContext(23-28)Release(1-11)DeploymentResourceRuleResult(33-36)
packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
packages/rule-engine/src/rules/index.ts (1)
VersionCooldownRule(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (14)
packages/rule-engine/src/rules/maintenance-window-rule.ts (1)
1-76: Looks great!
The maintenance window logic is clearly defined and easily extensible.packages/rule-engine/src/rules/time-window-rule.ts (1)
1-118: Solid implementation.
All relevant edge cases such as invalid timezone handling and optional day constraints are meticulously handled.packages/rule-engine/src/rules/resource-concurency-rule.ts (1)
31-35: Use resourceId instead of id in the filter.
Previously flagged:releaseJobTrigger.idis likely the primary key for the trigger record and not the resource identifier. Please update it to usereleaseJobTrigger.resourceIdif your intention is to handle concurrency at a per-resource level.- eq(schema.releaseJobTrigger.id, resourceId), + eq(schema.releaseJobTrigger.resourceId, resourceId),packages/rule-engine/src/rules/approval-required-rule.ts (3)
1-6: Imports are well organized
No concerns. The import statements cleanly pull in required types and align with best practices.
8-36: Interface fields look consistent
TheApprovalRequiredRuleOptionstype is well-defined and includes clear doc comments. The optional patterns allow for flexible constraint definitions. No issues found.
38-52: No action needed
The doc block is informative and well-structured.packages/rule-engine/src/rules/previous-deploy-status-rule.ts (3)
1-31: Config options are clear
ThePreviousDeployStatusRuleOptionstype covers essential fields. The fallback tominSuccessfulDeployments=0ensures a default behavior. Good job.
33-66: Constructor sets sensible defaults
AssigningminSuccessfulDeployments=0when neitherrequireAllResourcesnor a minimum is provided streamlines usage.
68-106: Async DB helpers are well-structured
BothgetResourceCountInEnvironmentsandgetSuccessfulDeploymentsCountfunctions properly join on relevant tables and retrieve the count. Looks good from a correctness perspective.packages/rule-engine/src/rules/gradual-rollout-rule.ts (2)
101-125: Hash-based deterministic rollout logic is good
The approach uses a consistent hash to map resources to a rollout percentage. This is a neat solution. No immediate issues.
126-131: Helper method is straightforward
getDaysSinceReleaseeffectively calculates day differences. No concerns.packages/rule-engine/src/rules/version-cooldown-rule.ts (3)
1-5: Clean and straightforward imports.The import statements appear well-organized and reflect correct usage of the referenced packages. No changes needed.
37-64: Efficient implementation to retrieve the last successful deployment.The query correctly filters for successful deployments, leveraging
descordering and a one-row limit for performance. Implementation looks good.
90-139: Well-structured cooldown logic with clear user feedback.The
filtermethod succinctly handles cooldown checks, returning an empty list and descriptive message if the cooldown remains in effect. This approach is user-friendly and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
packages/rule-engine/src/rules/deployment-resource-selector-rule.ts (3)
8-9: Consider specifying a more descriptive type.
Currently,DeploymentResourceSelectorOptionsis defined asobject, which might be too general and could lead to unclear code or usage. Consider refining or extending this type to reflect actual configuration parameters, improving type safety.
19-21: Implement theTODOfor resource validation.
The comment suggests that resource validation is pending. Finalizing validation logic will ensure the rule accurately blocks or allows releases based on real matching criteria.Do you want me to propose a resource validation snippet or open a new issue to track it?
27-30: Consider adding supplementary logging or metrics.
When validation is passed, returning all candidates is helpful, but it may be beneficial to provide context about why they passed or include logging. This insight can help in debugging deployments.packages/rule-engine/src/rules/deployment-version-selector-rule.ts (3)
8-9: Refine the options type.
DeploymentVersionSelectorOptionsis declared asobject. In future iterations, providing specialized fields or interfaces for version constraints can make the rule more robust and easier to maintain.
19-21: Complete theTODOfor version resource validation.
The placeholder indicates that the logic for validating version-specific constraints is not yet implemented. Addressing this ensures the rule actively filters out incompatible releases.Do you want me to supply a basic version check snippet, or shall we create a separate story to handle this?
27-30: Include additional rationale for passed validation.
When returning the current candidates, consider returning or logging details explaining why they are valid, especially if multiple rules chain together in the Rule Engine.packages/rule-engine/src/rules/dependency-check-rule.ts (3)
12-15: Clarify the contract for the dependency check result.
Since theDependencyCheckFunctionspecifies amessagein addition tosatisfied, ensure calling code or documentation clarifies how these details are surfaced to the user or logs.
78-90: Optimize asynchronous checks when multiple dependencies exist.
The checks run sequentially in the nestedforloops, which can slow down deployments when multiple dependencies must be checked for each release. Consider parallelizing dependency checks per release to reduce overall wait time.
99-107: Enhance blocked release reporting.
Only the first blocked release triggers a global block. If multiple releases fail, providing a combined reason could aid troubleshooting. Either aggregate reasons or note the other blocked releases so teams can swiftly diagnose.packages/rule-engine/src/rules/webhook-check-rule.ts (3)
28-35: Consider supporting additional HTTP methods outside "POST" and "GET".
While theWebhookCallertype currently restricts the method to "POST" or "GET", some users may require alternative methods like "PUT" or "PATCH". Broadening support could enhance flexibility.- export type WebhookCaller = ( - url: string, - payload: Record<string, unknown>, - options?: WebhookCallerOptions, - ) => Promise<WebhookResponse>; + export type WebhookCaller = ( + url: string, + payload: Record<string, unknown>, + options?: WebhookCallerOptions, + ) => Promise<WebhookResponse | undefined>;You may then expand the
methodfield inWebhookCallerOptionsaccordingly.
57-111: Validate JSON response parsing exceptions.
Usingawait response.json()inside try/catch is generally fine, but if the response body isn't valid JSON, it will throw—currently caught by thecatchblock as a generic webhook error. Consider providing a more explicit error message to distinguish JSON parsing issues from network errors.
175-230: Avoid potential data race in theresultCacheusage if invoked concurrently.
Node.js is single-threaded by default, but concurrency can arise with worker threads or cluster mode. Simultaneous calls tofilter()could cause race conditions in theresultCacheMap updates. If concurrency is expected, consider synchronizing access or using a more robust caching mechanism.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rule-engine/src/rules/dependency-check-rule.ts(1 hunks)packages/rule-engine/src/rules/deployment-resource-selector-rule.ts(1 hunks)packages/rule-engine/src/rules/deployment-version-selector-rule.ts(1 hunks)packages/rule-engine/src/rules/webhook-check-rule.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/rules/deployment-version-selector-rule.tspackages/rule-engine/src/rules/dependency-check-rule.tspackages/rule-engine/src/rules/deployment-resource-selector-rule.tspackages/rule-engine/src/rules/webhook-check-rule.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/rule-engine/src/rules/webhook-check-rule.ts (2)
232-334: Well-structured filtering logic with fallback for webhook failures.
The overall design—checking cache expiration, calling the webhook, then applyingblockOnFailure—is clear and maintainable. Great job!
113-152:❓ Verification inconclusive
Be cautious about sending sensitive metadata in the default payload.
TheWebhookCheckRuleOptionsallows customizing payload creation. The default implementation includes various details about deployments and releases, which may contain sensitive information. Ensure that downstream systems are trusted to handle these fields securely, or explicitly remove them viapreparePayload.Would you like to confirm that no sensitive data is included by default? You can run a quick check for references to access tokens, secrets, or API keys in the codebase:
🏁 Script executed:
#!/bin/bash # Searching for typical secret references: rg -A 2 -i "api[_-]?key|secret|token|credential"Length of output: 65846
Action Required: Verify Sensitive Data Handling in Default Payload
The definition for
WebhookCheckRuleOptionsshows that, by default, thepreparePayloadfunction creates a payload containing deployment context and release information. While our repository scan didn’t reveal explicit sensitive data in this type definition, the broader codebase does reference various API keys, tokens, and secrets. Please manually verify that the default payload does not inadvertently include any sensitive metadata. If necessary, overridepreparePayloadto explicitly remove or filter out sensitive fields before sending data to downstream systems.
- File to check:
packages/rule-engine/src/rules/webhook-check-rule.ts(see lines 113–152)- Action: Manually review the payload composition to ensure that no sensitive data (e.g., access tokens, API keys, or secrets) is included by default.
| export type DependencyCheckRuleOptions = { | ||
| /** | ||
| * Map of dependency names to check functions | ||
| */ | ||
| dependencyChecks: Record<string, DependencyCheckFunction>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider fallback behavior for missing dependencies.
dependencyChecks is a record. If a dependency name is missing from this record or is undefined, determine if code should implicitly treat it as satisfied or block the release for safety.
|
A TimeWindow rule should be modeled differently. A valid one should be a series of DayOfWeek/Hour start/end pairs. Sa/03->Su/23, Mo/20->Tue/04, etc. Validation should also ensure that no start/end pairs overlap any other. The simplest and least error prone way to implement is to convert the rule into a series of UTC times, as this will cover daylight savings conditions. The pairs will need to be ordered chronologically. You could do this by ordering each pair's start date/hour where Su is the lowest and Sa is the highest. As long as it's consistently done, the actual heuristic won't matter. Next, pick a start/end pair. Simplest is to pick the first in the ordinal list. Imagine that the ordinal list of pairs is really a ring buffer. The start/end pair after the end of the ordinal list is just the starting start/end pair. Construct a starting pair where the pair's start (in UTC) before or equal to the current time (in UTC). Construct an ending pair where the pair's end (in UTC) is after or equal to the current time (in UTC). Now walk the right buffer from your starting point and convert each pair to UTC. For each pair:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rule-engine/src/rules/gradual-rollout-rule.ts (1)
59-64:⚠️ Potential issue[Duplicate Comment] Missing time-based cutoff in the query.
This query skips comparing the job’s creation time to the cutoff, effectively counting all successful deployments ever made for the release. This leads to an unintended perpetual block once you exceed the quota.
Apply a condition to ensure only deployments newer than
cutoffTimeare counted:.where( and( eq(schema.releaseJobTrigger.versionId, releaseId), eq(schema.job.status, JobStatus.Successful), + gte(schema.job.createdAt, cutoffTime) ), )
🧹 Nitpick comments (4)
packages/rule-engine/src/rules/index.ts (1)
1-1: Correct the spelling in the file import path.The file name "resource-concurency-rule.js" is missing one 'r' in "concurrency." This could cause confusion or broken imports if the file name differs from what is expected.
Apply this diff to correct the spelling:
- export { ResourceConcurrencyRule } from "./resource-concurency-rule.js"; + export { ResourceConcurrencyRule } from "./resource-concurrency-rule.js";packages/rule-engine/src/rules/previous-deploy-status-rule.ts (3)
85-99: Consider default behavior when neither option is provided.When both
requireAllResourcesandminSuccessfulDeploymentsare undefined, the code defaults tominSuccessfulDeployments = 0, which effectively disables the rule. This might lead to confusion since users would expect some validation to occur by default.Consider setting a more restrictive default, or adding a warning in the documentation about this behavior:
constructor(private options: PreviousDeployStatusRuleOptions) { // Set default values if ( this.options.requireAllResources === undefined && this.options.minSuccessfulDeployments === undefined ) { - this.options.minSuccessfulDeployments = 0; + this.options.minSuccessfulDeployments = 1; // Default to requiring at least one successful deployment } // Set default get functions if not provided this.getResourceCount = options.getResourceCount ?? this.defaultGetResourceCount; this.getSuccessfulDeployments = options.getSuccessfulDeployments ?? this.defaultGetSuccessfulDeployments; }
143-149: Consider the clarity of the early return condition.The early return logic is functionally correct but could be more readable by restructuring it.
// Skip validation if no dependent environments or minimum is 0 if ( - this.options.dependentEnvironments.length === 0 || - (this.options.minSuccessfulDeployments === 0 && - !this.options.requireAllResources) + this.options.dependentEnvironments.length === 0 || + (this.options.minSuccessfulDeployments === 0 && !this.options.requireAllResources) ) { return { allowedReleases: currentCandidates }; }
80-200: Consider adding error handling for database queries.The database queries in
defaultGetResourceCountanddefaultGetSuccessfulDeploymentsdon't include explicit error handling. While they'll propagate errors up the call stack, it might be helpful to catch and handle database errors more gracefully.Consider adding try/catch blocks around database operations to provide more context-specific error messages:
private defaultGetResourceCount: GetResourceCountFunction = async ( environments: string[], ) => { - return db - .select({ count: count() }) - .from(schema.resource) - .innerJoin( - schema.deployment, - eq(schema.resource.deploymentId, schema.deployment.id), - ) - .where(inArray(schema.deployment.environmentId, environments)) - .then((r) => r[0]?.count ?? 0); + try { + return db + .select({ count: count() }) + .from(schema.resource) + .innerJoin( + schema.deployment, + eq(schema.resource.deploymentId, schema.deployment.id), + ) + .where(inArray(schema.deployment.environmentId, environments)) + .then((r) => r[0]?.count ?? 0); + } catch (error) { + console.error(`Error fetching resource count: ${error}`); + throw new Error(`Failed to get resource count for environments: ${environments.join(', ')}`); + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/gradual-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/previous-deploy-status-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rule-engine/src/types.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/rules/gradual-rollout-rule.tspackages/rule-engine/src/rules/index.tspackages/rule-engine/src/rule-engine.tspackages/rule-engine/src/rules/previous-deploy-status-rule.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
packages/rule-engine/src/rules/index.ts (1)
2-10: Centralized export structure looks good!Exporting these rule classes in a single index file is a neat pattern that simplifies imports across the codebase. This improves discoverability and maintainability of the rules.
packages/rule-engine/src/rules/gradual-rollout-rule.ts (1)
98-131:❓ Verification inconclusive
Implementation of gradual rollout limit appears sound.
Beyond adding the cutoff condition for time windows, the rest of the filtering logic correctly enforces a maximum limit for recent successful deployments. Please ensure that you have tests covering edge cases (e.g., exactly hitting the limit, no past deployments, etc.).
🏁 Script executed:
#!/bin/bash # Checking if GradualRolloutRule is tested fd --regex 'test.*\.ts' | xargs rg -A 5 'GradualRolloutRule'Length of output: 60
Action Required: Verify Test Coverage for Gradual Rollout Rule
The gradual rollout logic, including the time window and deployment count checks, appears correct. However, the automated search did not return any tests for the
GradualRolloutRule. Please manually verify if tests exist for edge cases (e.g., exactly hitting the deployment limit, no past deployments, etc.). If tests are missing, kindly add them to cover these scenarios.packages/rule-engine/src/rule-engine.ts (2)
119-127: Graceful handling of no remaining candidates.Halting evaluation when a rule disqualifies all candidates is a good approach, ensuring early feedback to the user. This avoids performing unnecessary checks on an empty set.
183-205: Final selection strategy is coherent.Prioritizing
desiredReleaseIdif present, otherwise choosing the newest release is a clear approach. This logic helps preserve user intent while still providing a fallback for normal deployments.packages/rule-engine/src/rules/previous-deploy-status-rule.ts (6)
1-5: LGTM: Import statements are well organized.The imports are correctly structured, with database-related functions and types imported first, followed by the internal types needed for the rule implementation.
13-56: Well-defined type declarations with good documentation.The type definitions are clear and well-documented. The function types for getting resource counts and successful deployments are abstracted correctly, allowing for custom implementations if needed.
58-79: Excellent documentation with useful examples.The class documentation clearly explains the purpose of the rule and includes practical examples that demonstrate different use cases. This helps developers understand how to properly configure and use this rule.
101-113: Verify ifdefaultGetResourceCountneeds to filter for successful deployments.The current implementation counts all resources in the specified environments without checking their deployment status. Depending on the rule's purpose, you might want to count only resources that have been deployed (successfully or not).
Please verify if this is the intended behavior. If it should only count deployed resources, consider modifying the query to include an appropriate filter.
115-136: Good implementation ofdefaultGetSuccessfulDeployments.The database query correctly joins the necessary tables and filters for successful deployments of the specified release in the required environments.
176-196: Good implementation of validation logic with clear error messages.The validation logic is well implemented with descriptive error messages that clearly explain why a release is being filtered out. The messages include specific numbers to help users understand exactly what's missing.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (1)
125-128:⚠️ Potential issueHandle edge case when totalResources is 0
If
requireAllResourcesis true but the total resources count is 0,requiredDeploymentswill be set to 0, which will always pass the validation check. This might not be the intended behavior.const requiredDeployments = requireAllResources ? await this.getResourceCount(dependentEnvironments.map(({ id }) => id)) : (minSuccessfulDeployments ?? 0); +// If there are no resources in the dependent environments, +// we should consider if this is a valid state or an error condition +if (requireAllResources && requiredDeployments === 0) { + // Option 1: Allow deployment (current behavior) + // Option 2: Block deployment with a clear message + return { + allowedReleases: [], + reason: `No resources found in dependent environments (${dependentEnvironments.map(({ name }) => name).join(", ")}). Cannot validate deployment requirements.` + }; +}
🧹 Nitpick comments (8)
packages/rule-engine/src/types.ts (1)
1-29: Consider stronger typing for selectors and metadata.
Currently,resourceSelector?: objectandversionSelector?: objectas well asmetadata: Record<string, string>might become catch-alls, potentially complicating future validations. Defining more specific types or interfaces for these fields could improve maintainability and correctness.packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (1)
41-59: Consider caching or memoizing the DB query.
IfgetRecentDeploymentCountis called frequently, you could introduce caching or memoization to reduce repetitive queries and enhance performance, especially if multiple calls target the same release within close intervals.packages/rule-engine/src/rules/maintenance-window-rule.ts (3)
11-26: The MaintenanceWindow type looks good, but consider adding timezone handling.The type definition is well-documented with clear JSDoc comments. However, based on the PR comments about TimeWindow rules, you might want to consider adding explicit timezone handling or documentation about how dates should be provided (e.g., in UTC).
export type MaintenanceWindow = { /** * Descriptive name of the maintenance window */ name: string; /** * Start date and time of the maintenance window */ start: Date; /** * End date and time of the maintenance window * + * Note: All dates should be provided in UTC to avoid timezone and + * daylight savings time issues. */ end: Date; };
49-49: Consider adding validation in the constructor.The constructor accepts maintenance windows without validation. Moving the validation from the filter method to the constructor would fail fast on invalid configurations.
- constructor(private maintenanceWindows: MaintenanceWindow[]) {} + constructor(private maintenanceWindows: MaintenanceWindow[]) { + // Validate maintenance windows at construction time + for (const window of maintenanceWindows) { + if (window.start > window.end) { + throw new Error(`Invalid maintenance window "${window.name}": start date must be before end date`); + } + } + + // Check for overlapping windows + for (let i = 0; i < maintenanceWindows.length; i++) { + for (let j = i + 1; j < maintenanceWindows.length; j++) { + const a = maintenanceWindows[i]; + const b = maintenanceWindows[j]; + if ( + (a.start <= b.end && a.end >= b.start) || + (b.start <= a.end && b.end >= a.start) + ) { + throw new Error(`Overlapping maintenance windows: "${a.name}" and "${b.name}"`); + } + } + } + }
63-69: Refine maintenance window filtering logic.The current implementation doesn't leverage the early validation I suggested for the constructor. If you add that validation, you can simplify this logic to only check if the window is currently active.
const activeWindows = this.maintenanceWindows.filter((window) => { - // Validate start date is before end date - const isValid = window.start <= window.end; // Check if window is currently active - const isActive = isValid && now >= window.start && now <= window.end; + const isActive = now >= window.start && now <= window.end; return isActive; });Also, consider adding more detailed comments about the logic applied here, especially since it's a central part of this rule's functionality.
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (3)
93-100: Clarify behavior when both requireAllResources and minSuccessfulDeployments are setThe code sets a default value for
minSuccessfulDeploymentswhen both options are null, but it doesn't address what happens when both are explicitly provided. This could lead to confusion about which takes precedence.constructor(private options: PreviousDeployStatusRuleOptions) { // Set default values if ( this.options.requireAllResources == null && this.options.minSuccessfulDeployments == null ) { this.options.minSuccessfulDeployments = 0; } + + // Warn if both options are provided + if (this.options.requireAllResources && this.options.minSuccessfulDeployments !== undefined) { + console.warn( + "Both requireAllResources and minSuccessfulDeployments are set. " + + "requireAllResources will take precedence." + ); + }
157-160: Include desired release ID in reason message for clarityThe reason message currently mentions the best candidate, but it would be more helpful to also mention the desired release ID that was being evaluated.
const envNames = dependentEnvironments.map(({ name }) => name).join(", "); +const desiredReleaseId = ctx.desiredReleaseId; const reasonMessage = this.options.requireAllResources - ? `Not all resources in ${envNames} have been successfully deployed for any release candidate. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.successfulDeployments}/${requiredDeployments} deployments.` - : `Minimum deployment requirement not met for any release candidate. Need at least ${requiredDeployments} successful deployments in ${envNames}. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.successfulDeployments} deployments.`; + ? `Not all resources in ${envNames} have been successfully deployed for desired release ${desiredReleaseId}. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.successfulDeployments}/${requiredDeployments} deployments.` + : `Minimum deployment requirement not met for desired release ${desiredReleaseId}. Need at least ${requiredDeployments} successful deployments in ${envNames}. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.successfulDeployments} deployments.`;
30-30: Consider using only environment IDs to simplify interfaceThe
dependentEnvironmentsoption requires both name and id, but the logic mostly uses the id for functional operations and name for messages. Consider simplifying this to accept just the IDs and having a separate mapping for names if needed.- dependentEnvironments: { name: string; id: string }[]; + dependentEnvironments: string[]; + /** + * Optional mapping of environment IDs to display names for error messages + */ + environmentNames?: Record<string, string>;Then update the related code to use this simplified structure.
📜 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)
packages/rule-engine/package.json(1 hunks)packages/rule-engine/src/rules/__tests__/maintenance-window-rule.test.ts(1 hunks)packages/rule-engine/src/rules/gradual-version-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/maintenance-window-rule.ts(1 hunks)packages/rule-engine/src/rules/previous-deploy-status-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rule-engine/package.json
- packages/rule-engine/src/rules/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/rules/__tests__/maintenance-window-rule.test.tspackages/rule-engine/src/rules/previous-deploy-status-rule.tspackages/rule-engine/src/types.tspackages/rule-engine/src/rules/maintenance-window-rule.tspackages/rule-engine/src/rules/gradual-version-rollout-rule.ts
🧬 Code Definitions (1)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (1)
packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(56-62)DeploymentResourceContext(31-37)Release(1-11)DeploymentResourceRuleResult(42-45)
⏰ 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/types.ts (2)
31-37: Looks good!
TheDeploymentResourceContexttype is clear and will be easy to use across different rules.
56-62: Interface definition is consistent and comprehensive.
Defining thefiltermethod return type as both a synchronous or promise-basedDeploymentResourceRuleResultis a good design choice for flexibility in complex asynchronous workflows.packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (2)
76-84: Constructor design is clear.
Injecting dependencies throughGradualVersionRolloutRuleOptionsand allowing an override forgetRecentDeploymentCountoffers excellent testability and flexibility.
94-163: Well-structured logic for filtering.
The parallel checks usingPromise.alland separating allowed and disallowed releases is clean. The step that builds a detailed reason for disallowed releases is sufficiently informative. Nice job!packages/rule-engine/src/rules/__tests__/maintenance-window-rule.test.ts (2)
13-26: Good approach to testing with a fixed date/time.
OverridinggetCurrentTimein a subclass is a neat way to ensure deterministic tests without reliance on mocking global date objects.
205-221: Robust handling of invalid windows.
Verifying that windows where the end date precedes the start date are gracefully ignored helps prevent unexpected deployment blocks. This test ensures safe usage of the rule even with poorly formed data.packages/rule-engine/src/rules/maintenance-window-rule.ts (4)
28-45: Consider addressing the TimeWindow requirements mentioned in PR comments.The PR comments mention specific requirements for TimeWindow rules, including converting to UTC to handle daylight savings, ensuring no overlap between windows, and implementing a ring buffer approach. These concepts don't seem to be reflected in this implementation.
Could you clarify if these comments were meant for a different rule type, or if the MaintenanceWindowRule should incorporate these concepts? If they apply to this rule, we may need to implement a more sophisticated approach to handling time windows.
52-54: Good pattern for testability.The protected getCurrentTime method allows for easier testing by providing a way to inject a custom time. This is a good practice for time-dependent code.
71-77: The blocking logic is well implemented.The code provides clear feedback when deployments are blocked, including the names of all active maintenance windows. This is helpful for users to understand why their deployment was blocked.
1-6: Import statement is well structured.The imports are clearly defined with proper typing, enhancing code clarity and maintainability.
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (1)
66-87: Great documentation with clear examplesThe class documentation is excellent with detailed explanations and clear examples showing how to use the rule for different scenarios.
| const getSuccessfulDeployments: GetSuccessfulDeploymentsFunction = ( | ||
| _: string, | ||
| __: string[], | ||
| ) => { | ||
| // TODO: Count of successful deployments in the dependent environments | ||
| return 0; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TODO comments should be addressed before production
Similarly, the default implementation of getSuccessfulDeployments returns 0 with a TODO comment. This should also be replaced with an actual implementation.
const getSuccessfulDeployments: GetSuccessfulDeploymentsFunction = (
_: string,
__: string[],
) => {
- // TODO: Count of successful deployments in the dependent environments
- return 0;
+ // Implement actual deployment counting logic
+ throw new Error("getSuccessfulDeployments not implemented. Please provide an implementation.");
};📝 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.
| const getSuccessfulDeployments: GetSuccessfulDeploymentsFunction = ( | |
| _: string, | |
| __: string[], | |
| ) => { | |
| // TODO: Count of successful deployments in the dependent environments | |
| return 0; | |
| }; | |
| const getSuccessfulDeployments: GetSuccessfulDeploymentsFunction = ( | |
| _: string, | |
| __: string[], | |
| ) => { | |
| // Implement actual deployment counting logic | |
| throw new Error("getSuccessfulDeployments not implemented. Please provide an implementation."); | |
| }; |
| const getResourceCount: GetResourceCountFunction = (_: string[]) => { | ||
| // TODO: Sum of all resources in the dependent environments | ||
| return 0; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TODO comments should be addressed before production
The default implementation of getResourceCount currently returns 0 with a TODO comment. Before deploying to production, this should be replaced with an actual implementation that properly counts resources.
const getResourceCount: GetResourceCountFunction = (_: string[]) => {
- // TODO: Sum of all resources in the dependent environments
- return 0;
+ // Implement actual resource counting logic
+ throw new Error("getResourceCount not implemented. Please provide an implementation.");
};📝 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.
| const getResourceCount: GetResourceCountFunction = (_: string[]) => { | |
| // TODO: Sum of all resources in the dependent environments | |
| return 0; | |
| }; | |
| const getResourceCount: GetResourceCountFunction = (_: string[]) => { | |
| // Implement actual resource counting logic | |
| throw new Error("getResourceCount not implemented. Please provide an implementation."); | |
| }; |
| async filter( | ||
| _: DeploymentResourceContext, | ||
| currentCandidates: Release[], | ||
| ): Promise<DeploymentResourceRuleResult> { | ||
| const { | ||
| dependentEnvironments, | ||
| minSuccessfulDeployments, | ||
| requireAllResources, | ||
| } = this.options; | ||
|
|
||
| const hasDependentEnvironments = dependentEnvironments.length > 0; | ||
| const hasMinimumRequirement = | ||
| (minSuccessfulDeployments ?? 0) > 0 || requireAllResources; | ||
|
|
||
| if (!hasDependentEnvironments || !hasMinimumRequirement) | ||
| return { allowedReleases: currentCandidates }; | ||
|
|
There was a problem hiding this comment.
Use the context parameter appropriately
The filter method doesn't use the DeploymentResourceContext parameter (renamed to _). Since the rule needs to check if a specific release is allowed, it's important to use this context properly.
async filter(
- _: DeploymentResourceContext,
+ ctx: DeploymentResourceContext,
currentCandidates: Release[],
): Promise<DeploymentResourceRuleResult> {
const {
dependentEnvironments,
minSuccessfulDeployments,
requireAllResources,
} = this.options;
const hasDependentEnvironments = dependentEnvironments.length > 0;
const hasMinimumRequirement =
(minSuccessfulDeployments ?? 0) > 0 || requireAllResources;
if (!hasDependentEnvironments || !hasMinimumRequirement)
return { allowedReleases: currentCandidates };
+ // If the desired release is not in the candidates, no need to check further
+ const desiredRelease = currentCandidates.find(r => r.id === ctx.desiredReleaseId);
+ if (!desiredRelease) {
+ return { allowedReleases: currentCandidates };
+ }📝 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.
| async filter( | |
| _: DeploymentResourceContext, | |
| currentCandidates: Release[], | |
| ): Promise<DeploymentResourceRuleResult> { | |
| const { | |
| dependentEnvironments, | |
| minSuccessfulDeployments, | |
| requireAllResources, | |
| } = this.options; | |
| const hasDependentEnvironments = dependentEnvironments.length > 0; | |
| const hasMinimumRequirement = | |
| (minSuccessfulDeployments ?? 0) > 0 || requireAllResources; | |
| if (!hasDependentEnvironments || !hasMinimumRequirement) | |
| return { allowedReleases: currentCandidates }; | |
| async filter( | |
| ctx: DeploymentResourceContext, | |
| currentCandidates: Release[], | |
| ): Promise<DeploymentResourceRuleResult> { | |
| const { | |
| dependentEnvironments, | |
| minSuccessfulDeployments, | |
| requireAllResources, | |
| } = this.options; | |
| const hasDependentEnvironments = dependentEnvironments.length > 0; | |
| const hasMinimumRequirement = | |
| (minSuccessfulDeployments ?? 0) > 0 || requireAllResources; | |
| if (!hasDependentEnvironments || !hasMinimumRequirement) | |
| return { allowedReleases: currentCandidates }; | |
| // If the desired release is not in the candidates, no need to check further | |
| const desiredRelease = currentCandidates.find(r => r.id === ctx.desiredReleaseId); | |
| if (!desiredRelease) { | |
| return { allowedReleases: currentCandidates }; | |
| } | |
| // ... rest of the function logic | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (3)
152-156:⚠️ Potential issueHandle empty candidate list case
The code assumes that
releaseChecksis not empty when it reduces it to find the best candidate. If there are no candidates, this could lead to an error.// If no releases allowed, find best candidate and return reason -const bestCandidate = releaseChecks.reduce((best, current) => - current.successfulDeployments > best.successfulDeployments - ? current - : best, -); +const bestCandidate = releaseChecks.length > 0 + ? releaseChecks.reduce((best, current) => + current.successfulDeployments > best.successfulDeployments + ? current + : best, + ) + : { release: { id: "none" }, successfulDeployments: 0 };
125-127: 🛠️ Refactor suggestionHandle edge case when totalResources is 0
If
requireAllResourcesis true buttotalResourcesis 0,requiredDeploymentswill be set to 0, which will always pass the validation check. This might not be the intended behavior if there are actually no resources in the dependent environments.const requiredDeployments = requireAllResources ? await this.getResourceCount(dependentEnvironments.map(({ id }) => id)) : (minSuccessfulDeployments ?? 0); + +// If there are no resources in the dependent environments and requiring all resources, +// we should consider if this is a valid state or an error condition +if (requireAllResources && requiredDeployments === 0) { + return { + allowedReleases: Releases.empty(), + reason: `No resources found in dependent environments (${dependentEnvironments.map(({ name }) => name).join(", ")}). Cannot validate deployment requirements.` + }; +}
108-110: 💡 Verification agent🧩 Analysis chain
Use the context parameter in the filter method
The
filtermethod doesn't use theDeploymentResourceContextparameter (renamed to_). Since the rule needs to check if a specific release is allowed, it's important to use this context properly.async filter( - _: DeploymentResourceContext, + ctx: DeploymentResourceContext, releases: Releases, ): Promise<DeploymentResourceRuleResult> {
🏁 Script executed:
#!/bin/bash # Check if the context parameter is used in other rule implementations # Look for filter method implementations in other rules echo "Checking how other rules use the context parameter in filter methods:" rg -A 3 "async filter\(" --type ts packages/rule-engine/src/rules/Length of output: 2379
Action Required: Rename the context parameter in
previous-deploy-status-rule.tsThe file currently declares the first parameter of the
filtermethod as_(i.e., ignored), but as seen in other rule implementations (e.g., inwebhook-check-rule.tsandversion-cooldown-rule.ts), this parameter is namedctxto indicate that theDeploymentResourceContextis available for use. Since this rule should check if a specific release is allowed using the context, please rename the parameter toctxas shown below:async filter( - _: DeploymentResourceContext, + ctx: DeploymentResourceContext, releases: Releases, ): Promise<DeploymentResourceRuleResult> {packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
23-35: 🛠️ Refactor suggestionConsider validating cooldownMinutes.
A zero or negative cooldown would allow immediate or retroactive deployments, which may be unintentional. This concern was also raised previously in a past review.You could enhance the constructor or filter method to verify that
cooldownMinutesis a positive integer. For example:constructor( private options: VersionCooldownRuleOptions = { cooldownMinutes: 1440, // 24 hours getLastSuccessfulDeploymentTime, }, ) { + if (this.options.cooldownMinutes <= 0) { + throw new Error("The cooldownMinutes value must be a positive integer."); + } }
🧹 Nitpick comments (9)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (3)
94-97: Use the context parameter in the filter methodSimilar to the
PreviousDeployStatusRule, thefiltermethod doesn't use theDeploymentResourceContextparameter (renamed to_). This context might be necessary for more sophisticated filtering logic.async filter( - _: DeploymentResourceContext, + ctx: DeploymentResourceContext, releases: Releases, ): Promise<DeploymentResourceRuleResult> {
143-153: Fix potential undefined access in bestCandidate calculationThe code that finds the best candidate might try to access properties of an undefined value if
disallowedReleasesis empty. The null check ondisallowedReleases.length > 0should handle this, but the default initialization todisallowedReleases[0]could beundefinedif the array is empty.const bestCandidate = disallowedReleases.length > 0 ? disallowedReleases.reduce( (best, current) => best && current.count < best.count ? current : (best ?? current), - disallowedReleases[0], + undefined, ) : null;
154-157: Improve error handling for the bestCandidate caseThe current code might have an undefined access if
bestCandidateisnulland we try to accessbestCandidate.release.id. It's better to handle this case explicitly.-reason = bestCandidate - ? `Gradual rollout limit reached for all release candidates. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.count}/${this.options.maxDeploymentsPerTimeWindow} deployments in the last ${this.options.timeWindowMinutes} minutes.` - : `Gradual rollout limit reached for all release candidates.`; +reason = bestCandidate + ? `Gradual rollout limit reached for all release candidates. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.count}/${this.options.maxDeploymentsPerTimeWindow} deployments in the last ${this.options.timeWindowMinutes} minutes.` + : `Gradual rollout limit reached for all release candidates. No candidates found.`;packages/rule-engine/src/rules/__tests__/sequential-upgrade-multiple.test.ts (1)
85-87: Minor wording discrepancy in the comment.The code references three sequential releases, but the comment says "both sequential releases." Consider updating it to reflect all three.
packages/rule-engine/src/rules/__tests__/sequential-upgrade-rule.test.ts (1)
7-78: Consider clarifying or modularizingcreateTestReleases.The helper function is quite verbose and covers multiple release scenarios (non-sequential vs. sequential). You could break it into smaller, semantically named helpers such as
createSequentialRelease()orcreateNonSequentialRelease()to improve readability and help future maintainers quickly grasp the intent of each release definition.packages/rule-engine/src/rules/sequential-upgrade-rule.ts (1)
88-96: Avoid code duplication when finding older sequential releases.You repeat blocks of code for retrieving older sequential releases in both the target-is-sequential scenario and the timestamp-check scenario. Consider extracting the logic into a private helper method or reusing the same snippet to reduce future maintenance overhead.
- const olderSequentialReleases = sequentialReleases.getCreatedBefore(effectiveTargetRelease); // repeated logic in multiple places... + private getOlderSequentialReleases( + from: Releases, + reference: Release + ): Releases { + return from.getCreatedBefore(reference); + } // Then replace repeated lines with: const olderSequentialReleases = this.getOlderSequentialReleases(sequentialReleases, effectiveTargetRelease);packages/rule-engine/src/rule-engine.ts (1)
188-191: Ensure consistent metadata handling with SequentialUpgradeRule.Here, the engine hard-codes
"requiresSequentialUpgrade"as the metadata key for sequential releases, whileSequentialUpgradeRulesupports custom metadata keys (e.g.,'mustApplySequentially'). If your use case requires flexible metadata logic, consider adding a similar configuration option to the engine or delegating all sequential logic to the rule. This prevents confusion if the rule uses a different metadataKey than the engine.packages/rule-engine/README.md (2)
14-49: Specify a language for fenced code blocks.
Markdownlint flags the ASCII diagram block for missing code language. Consider labeling it asplaintextor another suitable language to fix lint warnings:- ``` + ```plaintext ┌─────────────────┐ │ All Available │ ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
127-127: Minor punctuation improvement.
Adding a comma after "independently" improves clarity and grammar:- 3. **Rule Independence**: Each rule should operate independently without relying + 3. **Rule Independence**: Each rule should operate independently, without relying🧰 Tools
🪛 LanguageTool
[uncategorized] ~127-~127: Possible missing comma found.
Context: ...ndependence**: Each rule should operate independently without relying on the outcome of ot...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/rule-engine/README.md(1 hunks)packages/rule-engine/src/__tests__/rule-engine-selection.test.ts(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/__tests__/sequential-upgrade-multiple.test.ts(1 hunks)packages/rule-engine/src/rules/__tests__/sequential-upgrade-rule.test.ts(1 hunks)packages/rule-engine/src/rules/approval-required-rule.ts(1 hunks)packages/rule-engine/src/rules/deployment-version-selector-rule.ts(1 hunks)packages/rule-engine/src/rules/gradual-version-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/maintenance-window-rule.ts(1 hunks)packages/rule-engine/src/rules/previous-deploy-status-rule.ts(1 hunks)packages/rule-engine/src/rules/resource-concurency-rule.ts(1 hunks)packages/rule-engine/src/rules/sequential-upgrade-rule.ts(1 hunks)packages/rule-engine/src/rules/time-window-rule.ts(1 hunks)packages/rule-engine/src/rules/version-cooldown-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)packages/rule-engine/src/utils/index.ts(1 hunks)packages/rule-engine/src/utils/releases.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rule-engine/src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rule-engine/src/rules/time-window-rule.ts
- packages/rule-engine/src/rules/maintenance-window-rule.ts
- packages/rule-engine/src/rules/approval-required-rule.ts
- packages/rule-engine/src/rules/deployment-version-selector-rule.ts
- packages/rule-engine/src/types.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/__tests__/rule-engine-selection.test.tspackages/rule-engine/src/index.tspackages/rule-engine/src/rules/__tests__/sequential-upgrade-rule.test.tspackages/rule-engine/src/rules/__tests__/sequential-upgrade-multiple.test.tspackages/rule-engine/src/rules/version-cooldown-rule.tspackages/rule-engine/src/rules/sequential-upgrade-rule.tspackages/rule-engine/src/rules/previous-deploy-status-rule.tspackages/rule-engine/src/rules/gradual-version-rollout-rule.tspackages/rule-engine/src/rules/resource-concurency-rule.tspackages/rule-engine/src/rules/index.tspackages/rule-engine/src/rule-engine.tspackages/rule-engine/src/utils/releases.ts
🧬 Code Definitions (7)
packages/rule-engine/src/__tests__/rule-engine-selection.test.ts (3)
packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(58-64)DeploymentResourceContext(33-39)DeploymentResourceRuleResult(44-47)Release(3-13)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)packages/rule-engine/src/rule-engine.ts (1)
RuleEngine(70-192)
packages/rule-engine/src/rules/__tests__/sequential-upgrade-multiple.test.ts (3)
packages/rule-engine/src/types.ts (2)
Release(3-13)DeploymentResourceContext(33-39)packages/rule-engine/src/rules/sequential-upgrade-rule.ts (1)
SequentialUpgradeRule(52-162)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)
packages/rule-engine/src/rules/version-cooldown-rule.ts (4)
packages/rule-engine/src/rules/index.ts (1)
VersionCooldownRule(9-9)packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(58-64)DeploymentResourceContext(33-39)DeploymentResourceRuleResult(44-47)packages/rule-engine/src/utils/index.ts (1)
Releases(1-1)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)
packages/rule-engine/src/rules/sequential-upgrade-rule.ts (2)
packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(58-64)DeploymentResourceContext(33-39)DeploymentResourceRuleResult(44-47)Release(3-13)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (3)
packages/rule-engine/src/rules/index.ts (1)
PreviousDeployStatusRule(6-6)packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(58-64)DeploymentResourceContext(33-39)DeploymentResourceRuleResult(44-47)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (2)
packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(58-64)DeploymentResourceContext(33-39)DeploymentResourceRuleResult(44-47)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)
packages/rule-engine/src/rules/resource-concurency-rule.ts (2)
packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(58-64)DeploymentResourceContext(33-39)DeploymentResourceRuleResult(44-47)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-312)
🪛 LanguageTool
packages/rule-engine/README.md
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...**: Interface for all rules - name: Identifying the rule for logging/debugg...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~127-~127: Possible missing comma found.
Context: ...ndependence**: Each rule should operate independently without relying on the outcome of ot...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
packages/rule-engine/README.md
14-14: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (40)
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (2)
53-56: TODO comments should be addressed before productionThe default implementation of
getResourceCountreturns 0 with a TODO comment. Before deploying to production, this should be replaced with an actual implementation that properly counts resources.const getResourceCount: GetResourceCountFunction = (_: string[]) => { - // TODO: Sum of all resources in the dependent environments - return 0; + // Implement actual resource counting logic + throw new Error("getResourceCount not implemented. Please provide an implementation."); };
58-64: TODO comments should be addressed before productionSimilarly, the default implementation of
getSuccessfulDeploymentsreturns 0 with a TODO comment. This should also be replaced with an actual implementation.const getSuccessfulDeployments: GetSuccessfulDeploymentsFunction = ( _: string, __: string[], ) => { - // TODO: Count of successful deployments in the dependent environments - return 0; + // Implement actual deployment counting logic + throw new Error("getSuccessfulDeployments not implemented. Please provide an implementation."); };packages/rule-engine/src/index.ts (1)
1-8: LGTM! Clean index exportsThe index file exports are well-organized, making it easy to access the various components of the rule engine.
packages/rule-engine/src/rules/resource-concurency-rule.ts (1)
31-34: Resource ID mismatch remains unresolved.As previously noted,
schema.releaseJobTrigger.idappears to be the trigger record's primary key, not the resource identifier. If the intention is to filter by resource ID, switch toschema.releaseJobTrigger.resourceIdinstead.packages/rule-engine/src/__tests__/rule-engine-selection.test.ts (3)
12-22: PassThroughRule implementation looks straightforward.No issues found. The rule passes all candidates as intended.
24-42: SequentialFilterRule is concise and clear.Filtering by metadata is a neat approach. Implementation is logically consistent.
43-178: Tests thoroughly validate release selection scenarios.All test cases appear well-structured, covering desired, newest, and sequential logic. No major concerns.
packages/rule-engine/src/rules/__tests__/sequential-upgrade-multiple.test.ts (1)
1-186: Comprehensive test coverage confirmed.These tests thoroughly validate multi-step sequential upgrade logic. No other issues identified.
packages/rule-engine/src/rules/__tests__/sequential-upgrade-rule.test.ts (1)
244-278:❓ Verification inconclusive
Verify the test outcome for multiple sequential releases.
When both release-2 and release-4 are tagged with the same custom key/value, they are both sequential and release-2 is older. However, this test expects release-4 to be chosen as the oldest sequential release. This appears contradictory to the logic in many of the earlier tests, which select release-2 under similar conditions. Confirm whether you truly intend to skip release-2 here or if there's a logic or test expectation mismatch.
If needed, run additional checks to confirm the creation dates and confirm that release-2 is indeed older.
🏁 Script executed:
#!/bin/bash # Searching for references to release-2 and release-4 in test code rg "release-(2|4)" -A 3Length of output: 6491
Clarify Sequential Upgrade Behavior for Custom Metadata Cases
It appears that the expected outcome in this custom metadata test (lines 244–278 in
packages/rule-engine/src/rules/__tests__/sequential-upgrade-rule.test.ts) differs from other tests. In this test, the logic chooses release‑4 even though release‑2—being older—is selected in similar conditions in other tests. Please confirm whether this behavior is intentional or if we need to adjust either the test expectations or the underlying logic.
- Check if the SequentialUpgradeRule should indeed skip release‑2 when both releases are tagged with the custom key/value.
- Review the ordering logic to ensure a consistent treatment of sequential releases across tests.
packages/rule-engine/src/rules/version-cooldown-rule.ts (5)
1-5: Imports look good.
No issues spotted with the import statements for database and status enums.
37-65: Database retrieval logic seems correct.
This code safely uses parameterized filters (eqandand) rather than string concatenation, mitigating injection risks. Good job.
80-88: Class instantiation is straightforward.
Providing defaults forcooldownMinutesandgetLastSuccessfulDeploymentTimeis a good approach.
90-109: Early exit logic on null deployment time looks good.
This ensures that if there is no previous deployment, the cooldown does not apply.
111-137: Cooldown calculation is clear and user-friendly.
The code to calculate the remaining time is well-structured. Returning a clear reason helps with user communication.packages/rule-engine/src/utils/releases.ts (26)
1-17: Documentation and imports look good.
No issues found in the class-level docstring or imports.
18-31: Constructor approach is clear.
Cloning the array of releases ensures immutability.
33-40: Factory method for empty instance is well-structured.
42-50: Static factory method 'from' is straightforward.
52-60: Getter returns a cloned array.
Minimizing side effects is excellent practice.
61-77: getOldest is implemented correctly.
Using reduce to find the oldest makes sense.
78-93: getNewest matches the getOldest logic.
This symmetry is clean and consistent.
95-108: getDesired method is straightforward.
No concerns; it neatly returns undefined if no desired ID or no match is found.
110-122: getEffectiveTarget merges desired release & fallback.
This approach is concise, returning the newest if no desired is specified.
124-135: filterByMetadata is reasonable.
It leverages the existing filter mechanism for clarity.
137-148: Sorting by ascending creation date is correct.
150-161: Sorting by descending creation date is correct.
163-175: getCreatedBefore logic is correct.
177-189: getCreatedAfter logic is correct.
191-199: findById method is concise.
201-208: Getter for length is straightforward.
210-217: isEmpty is well-implemented.
219-229: add method preserves immutability.
230-239: only method is useful for constraining releases.
241-249: map method is well-defined.
251-258: forEach passes a simple callback.
260-269: filter method returns a new Releases instance.
Good practice to maintain immutability.
271-280: find method is straightforward.
282-291: some method is correctly implemented.
293-301: every method is correctly implemented.
303-311: filterByTag is consistent with filterByMetadata.
No issues found.
| export { ResourceConcurrencyRule } from "./resource-concurency-rule.js"; | ||
| export { ApprovalRequiredRule } from "./approval-required-rule.js"; | ||
| export { DependencyCheckRule } from "./dependency-check-rule.js"; | ||
| export { GradualRolloutRule } from "./gradual-version-rollout-rule.js"; | ||
| export { MaintenanceWindowRule } from "./maintenance-window-rule.js"; | ||
| export { PreviousDeployStatusRule } from "./previous-deploy-status-rule.js"; | ||
| export { DeploymentResourceSelectorRule } from "./deployment-resource-selector-rule.js"; | ||
| export { TimeWindowRule } from "./time-window-rule.js"; | ||
| export { VersionCooldownRule } from "./version-cooldown-rule.js"; | ||
| export { WebhookCheckRule } from "./webhook-check-rule.js"; | ||
| export { SequentialUpgradeRule } from "./sequential-upgrade-rule.js"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix typo in file path import
There's a typo in the import path for ResourceConcurrencyRule. The file path contains "concurency" instead of "concurrency".
-export { ResourceConcurrencyRule } from "./resource-concurency-rule.js";
+export { ResourceConcurrencyRule } from "./resource-concurrency-rule.js";🏁 Script executed:
#!/bin/bash
# Check if the file with the typo exists or if the correct spelling exists
echo "Checking for the resource concurrency rule file:"
find packages/rule-engine/src/rules/ -name "resource-con*-rule.ts" -o -name "resource-con*-rule.js"Length of output: 267
Action Required: Correct the Import Path and File Extension for ResourceConcurrencyRule
- The current export in
packages/rule-engine/src/rules/index.tsreferences./resource-concurency-rule.js, which contains both a typo ("concurency" should be "concurrency") and an incorrect file extension. - Our verification shows that the actual file is located at
packages/rule-engine/src/rules/resource-concurency-rule.ts. - To maintain consistency, please update both the file name and the import path. For example, you could rename the file to
resource-concurrency-rule.tsand update the export, as shown below:
-export { ResourceConcurrencyRule } from "./resource-concurency-rule.js";
+export { ResourceConcurrencyRule } from "./resource-concurrency-rule.ts";This change ensures the file name is spelled correctly and that the file extension matches the actual file.
📝 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 { ResourceConcurrencyRule } from "./resource-concurency-rule.js"; | |
| export { ApprovalRequiredRule } from "./approval-required-rule.js"; | |
| export { DependencyCheckRule } from "./dependency-check-rule.js"; | |
| export { GradualRolloutRule } from "./gradual-version-rollout-rule.js"; | |
| export { MaintenanceWindowRule } from "./maintenance-window-rule.js"; | |
| export { PreviousDeployStatusRule } from "./previous-deploy-status-rule.js"; | |
| export { DeploymentResourceSelectorRule } from "./deployment-resource-selector-rule.js"; | |
| export { TimeWindowRule } from "./time-window-rule.js"; | |
| export { VersionCooldownRule } from "./version-cooldown-rule.js"; | |
| export { WebhookCheckRule } from "./webhook-check-rule.js"; | |
| export { SequentialUpgradeRule } from "./sequential-upgrade-rule.js"; | |
| export { ResourceConcurrencyRule } from "./resource-concurrency-rule.ts"; | |
| export { ApprovalRequiredRule } from "./approval-required-rule.js"; | |
| export { DependencyCheckRule } from "./dependency-check-rule.js"; | |
| export { GradualRolloutRule } from "./gradual-version-rollout-rule.js"; | |
| export { MaintenanceWindowRule } from "./maintenance-window-rule.js"; | |
| export { PreviousDeployStatusRule } from "./previous-deploy-status-rule.js"; | |
| export { DeploymentResourceSelectorRule } from "./deployment-resource-selector-rule.js"; | |
| export { TimeWindowRule } from "./time-window-rule.js"; | |
| export { VersionCooldownRule } from "./version-cooldown-rule.js"; | |
| export { WebhookCheckRule } from "./webhook-check-rule.js"; | |
| export { SequentialUpgradeRule } from "./sequential-upgrade-rule.js"; |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/rule-engine/src/utils/releases.ts (1)
1-303: 💡 Verification agent❓ Verification inconclusive
Add or verify unit test coverage for the new class.
This file introduces a critical building block of the rule engine. Ensuring robust test coverage would validate that filtering, sorting, and retrieval methods behave correctly under various scenarios (e.g., empty sets, single release, multiple releases, invalid metadata keys).
Below is a sample script to look for existing test files referencing
releases.ts. If none are found, consider adding a dedicated test suite:
🏁 Script executed:
#!/bin/bash # Searching for test files that reference "releases.ts" to verify coverage fd -t f "releases.test.ts" --exec cat {} \; | rg "describe|it|test" -A3Length of output: 74
Ensure robust unit test coverage for the
ReleasesclassThe
Releasesclass is a foundational component of the rule engine, handling various operations like filtering, sorting, and retrieval of release candidates. However, our initial automated search for test files (e.g. any file namedreleases.test.ts) yielded no results. This suggests that there may currently be little or no unit test coverage for this module.Action Items:
- Verify or add unit tests:
- If a test suite already exists (possibly under another naming convention), please validate that it exercises the methods of this class thoroughly.
- Otherwise, please add dedicated tests for scenarios including:
- Empty collections: Confirming that methods like
getOldest,getNewest, andgetEffectiveTargethandle empty inputs gracefully.- Single and multiple releases: Validating correct behavior when one or more release candidates are present.
- Edge cases: Testing filtering functions (e.g.,
filterByMetadata,getCreatedBefore,getCreatedAfter) with invalid or missing metadata keys.- Sorting logic: Ensuring that both ascending and descending sorts correctly order releases based on
createdAt.Given the importance of this class to downstream rules in the rule engine and the lack of detected test coverage, enhancing or adding tests here is imperative.
♻️ Duplicate comments (1)
packages/rule-engine/src/rules/index.ts (1)
1-1:⚠️ Potential issueAction Required: Correct the Import Path for ResourceConcurrencyRule
The current export references
./resource-concurency-rule.js, which contains a typo ("concurency" should be "concurrency").-export { ResourceConcurrencyRule } from "./resource-concurency-rule.js"; +export { ResourceConcurrencyRule } from "./resource-concurrency-rule.js";
🧹 Nitpick comments (10)
apps/event-worker/src/rule-engine-evaluation/index.ts (2)
56-106: Consider parameterizing the 100-record limit.
The limit is hard-coded to 100, which could be insufficient or excessive depending on business needs. Exposing it as a parameter may improve flexibility.- .limit(100) + .limit(candidateLimit ?? 100)
114-151: Enhance error handling or logging increateRuleEngineEvaluationWorker.
While the try/finally ensures the mutex is released, an explicit catch block can log or handle evaluation errors more gracefully before rethrowing or returning.try { ... } catch (error) { + console.error("Rule engine evaluation failed:", error); throw error; } finally { await mutex.release(); }packages/rule-engine/src/rules/version-metadata-validation-rule.ts (1)
115-125: Consider more advanced or explicit environment pattern matching.
Currently usingenvName.includes(pattern.toLowerCase())may unintentionally match partial tokens. A more precise or regex-based check might be clearer.- const shouldApply = environmentPatterns.some((pattern) => - envName.includes(pattern.toLowerCase()), - ); + const shouldApply = environmentPatterns.some((pattern) => { + // Example using exact case-insensitive match or a regex approach + const regex = new RegExp(`^${pattern}$`, "i"); + return regex.test(envName); + });packages/rule-engine/src/rules/webhook-check-rule.ts (2)
58-112: Suggest stronger error detail or retry mechanism
The default webhook caller returns a generic error message if the fetch fails or the status is non-OK. Consider implementing a retry strategy or more nuanced logging for 4xx vs 5xx errors.
192-335: Consider limiting cache size or adding eviction
Caching results in-memory is efficient for short bursts, but may grow large if there are frequent or varied deployments. A size-based eviction mechanism (e.g., LRU) may prevent memory issues.packages/rule-engine/README.md (4)
14-49: Add a fenced code block for the ASCII diagram.Markdown linting rules (MD040) suggest specifying a language for fenced blocks. Wrapping this ASCII diagram in a code fence makes it clearer and avoids lint warnings.
- ┌─────────────────┐ - │ All Available │ - │ Releases │ - └────────┬────────┘ +```text + ┌─────────────────┐ + │ All Available │ + │ Releases │ + └────────┬────────┘ │ ▼ (diagram lines omitted for brevity) +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
70-70: Improve punctuation or phrasing for clarity.Currently, the text after "DeploymentResourceRule:" seems to have an awkward punctuation flow. Consider revising it for improved clarity.
-3. **DeploymentResourceRule**: Interface for all rules +3. **DeploymentResourceRule** – an interface used by all rules🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...**: Interface for all rules -name: Identifying the rule for logging/debugg...(UNLIKELY_OPENING_PUNCTUATION)
127-127: Insert a missing comma to clarify the sentence.The sentence would read more clearly with a comma after "independently."
-3. **Rule Independence**: Each rule should operate independently without relying +3. **Rule Independence**: Each rule should operate independently, without relying🧰 Tools
🪛 LanguageTool
[uncategorized] ~127-~127: Possible missing comma found.
Context: ...ndependence**: Each rule should operate independently without relying on the outcome of ot...(AI_HYDRA_LEO_MISSING_COMMA)
150-151: Consider incorporating the ring buffer approach recommended by j7m4.While the README alludes to the
TimeWindowRule, user feedback suggests a more robust approach that accounts for day-of-week transitions and daylight savings by organizing time windows in a ring buffer. This can prevent overlaps and ensure accurate scheduling in UTC.packages/rule-engine/src/utils/releases.ts (1)
18-18: Use consistent naming between documentation and implementation.The README refers to “CandidateReleases,” while the class is named
Releases. Consider renaming the class for clarity and consistency, especially since the JSDoc repeatedly mentions "CandidateReleases."-export class Releases { +export class CandidateReleases {
📜 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 (23)
apps/event-worker/package.json(2 hunks)apps/event-worker/src/job-sync/index.ts(0 hunks)apps/event-worker/src/rule-engine-evaluation/index.ts(1 hunks)packages/db/src/schema/index.ts(1 hunks)packages/db/src/schema/release.ts(1 hunks)packages/db/src/schema/resource.ts(2 hunks)packages/db/src/schema/rule.ts(1 hunks)packages/rule-engine/README.md(1 hunks)packages/rule-engine/src/evaluate.ts(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/dependency-check-rule.ts(1 hunks)packages/rule-engine/src/rules/deployment-resource-selector-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/sequential-upgrade-rule.ts(1 hunks)packages/rule-engine/src/rules/time-window-rule.ts(1 hunks)packages/rule-engine/src/rules/version-cooldown-rule.ts(1 hunks)packages/rule-engine/src/rules/version-metadata-validation-rule.ts(1 hunks)packages/rule-engine/src/rules/webhook-check-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)packages/rule-engine/src/utils/index.ts(1 hunks)packages/rule-engine/src/utils/releases.ts(1 hunks)packages/validators/src/events/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/event-worker/src/job-sync/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rule-engine/src/index.ts
- packages/rule-engine/src/rules/deployment-resource-selector-rule.ts
- packages/rule-engine/src/utils/index.ts
- packages/rule-engine/src/rules/time-window-rule.ts
- packages/rule-engine/src/types.ts
- packages/rule-engine/src/rules/sequential-upgrade-rule.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/evaluate.tspackages/db/src/schema/index.tspackages/db/src/schema/rule.tspackages/rule-engine/src/rules/webhook-check-rule.tspackages/validators/src/events/index.tspackages/db/src/schema/resource.tspackages/rule-engine/src/rules/dependency-check-rule.tspackages/db/src/schema/release.tspackages/rule-engine/src/rules/version-cooldown-rule.tspackages/rule-engine/src/rules/index.tspackages/rule-engine/src/rule-engine.tspackages/rule-engine/src/rules/version-metadata-validation-rule.tsapps/event-worker/src/rule-engine-evaluation/index.tspackages/rule-engine/src/utils/releases.ts
🧬 Code Definitions (8)
packages/rule-engine/src/evaluate.ts (3)
packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
VersionCooldownRule(80-138)packages/rule-engine/src/rule-engine.ts (1)
evaluate(111-146)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/webhook-check-rule.ts (2)
packages/rule-engine/src/types.ts (4)
DeploymentResourceContext(33-38)Release(3-13)DeploymentResourceRule(57-63)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-302)
packages/db/src/schema/resource.ts (3)
packages/db/src/schema/environment.ts (1)
environment(49-74)packages/validators/src/cac/index.ts (1)
deployment(9-14)packages/db/src/schema/release.ts (1)
release(17-36)
packages/rule-engine/src/rules/dependency-check-rule.ts (1)
packages/rule-engine/src/types.ts (4)
DeploymentResourceContext(33-38)Release(3-13)DeploymentResourceRule(57-63)DeploymentResourceRuleResult(43-46)
packages/db/src/schema/release.ts (4)
packages/db/src/schema/resource.ts (1)
resource(61-89)packages/db/src/schema/deployment.ts (1)
deployment(61-85)packages/db/src/schema/environment.ts (1)
environment(49-74)packages/db/src/schema/job.ts (1)
job(74-106)
packages/rule-engine/src/rule-engine.ts (2)
packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceSelectionResult(48-52)Release(3-13)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/version-metadata-validation-rule.ts (3)
packages/rule-engine/src/rules/index.ts (1)
VersionMetadataValidationRule(7-7)packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/utils/releases.ts (2)
packages/rule-engine/src/types.ts (2)
Release(3-13)DeploymentResourceContext(33-38)packages/db/src/schema/release.ts (1)
release(17-36)
🪛 LanguageTool
packages/rule-engine/README.md
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...**: Interface for all rules - name: Identifying the rule for logging/debugg...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~127-~127: Possible missing comma found.
Context: ...ndependence**: Each rule should operate independently without relying on the outcome of ot...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
packages/rule-engine/README.md
14-14: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (33)
packages/db/src/schema/index.ts (1)
25-25: Export addition for the new release schema looks good!The addition of the new export statement for the release schema aligns well with the existing pattern in this file.
apps/event-worker/package.json (2)
23-23: New rule-engine package dependency addition looks appropriate.This dependency is necessary for implementing the new policy engine mentioned in the PR objectives.
40-40: Redis-semaphore addition is good for handling concurrent operations.The redis-semaphore package is a good choice for implementing distributed locking mechanisms which will likely be needed for the policy engine to handle concurrent rule evaluations safely.
packages/rule-engine/src/evaluate.ts (1)
11-33: Implementation of versionCooldownRule with database query looks good.The implementation correctly queries the database for the last successful deployment and uses the policy's minimumReleaseInterval for the cooldown period. The SQL query is properly constructed with appropriate joins and conditions.
packages/db/src/schema/release.ts (3)
17-36: Release table structure looks appropriate.The table structure with foreign key relationships to version, resource, deployment, and environment tables is well-designed. The cascade delete behavior ensures data integrity.
38-50: ReleaseVariable table with unique constraint is well-designed.The unique index on (releaseId, key) prevents duplicate variable keys for the same release, which is the correct constraint for this type of data.
52-63: ReleaseJob table for tracking related jobs looks good.The table correctly associates jobs with releases and includes appropriate timestamps and foreign key constraints.
packages/validators/src/events/index.ts (2)
5-10: LGTM: Good addition to the Channel enumThe addition of
RuleEngineEvaluationto the Channel enum is well-structured and follows the established pattern in the file.
23-30: Well-structured event schemaThe event schema for rule engine evaluation is properly defined with appropriate UUID validation for all required fields (resourceId, deploymentId, and environmentId).
packages/db/src/schema/rule.ts (3)
6-21: LGTM: Well-structured rule table definitionThe rule table is properly defined with all necessary fields including primary key, name, description, workspace reference, and timestamps with appropriate constraints.
23-32: LGTM: Good rollout rule schemaThe rollout rule table properly defines parameters for gradual deployment with appropriate time window and deployment limit configurations.
34-42: LGTM: Well-structured approval rule schemaThe approval rule table correctly establishes the relationship between rules and environments that require approval, with proper foreign key constraints.
packages/rule-engine/src/rules/index.ts (1)
2-12: LGTM: Well-organized rule exportsThe export structure is clean and follows a consistent pattern, making all rule classes available through a single import location.
packages/db/src/schema/resource.ts (2)
53-57: LGTM: Good import additionsThe new imports are properly added for the tables referenced in the new schema.
412-434: LGTM: Well-designed resourceDesiredRelease tableThe resourceDesiredRelease table is well-structured with:
- Proper foreign key relationships and cascade behaviors
- A good unique constraint on the combination of resourceId, environmentId, and deploymentId
- Appropriate nullable field for desiredReleaseId with default NULL value
This design will work well for tracking desired releases per resource across environments and deployments.
apps/event-worker/src/rule-engine-evaluation/index.ts (2)
1-19: No further comments on imports.
21-54: The context lookup logic looks well-structured.
All necessary joins and filters are appropriately handled, and the missing record scenario is handled downstream by the null check.packages/rule-engine/src/rules/dependency-check-rule.ts (2)
80-88: Consider fallback behavior for missing dependencies.
This matches a past comment asking if a dependency name not found independencyChecks(or undefined responses) should implicitly pass or block the release for safety.
50-116: Dependency check logic is straightforward and robust.
Releases with no failed checks are allowed, and failing checks appropriately block a release with descriptive reasons.packages/rule-engine/src/rules/version-metadata-validation-rule.ts (1)
127-163: Metadata Validation Rule logic appears solid and aligns with the intended functionality.
Missing metadata handling and environment-specific lookups are clearly implemented, and the custom error message support is a nice touch.packages/rule-engine/src/rule-engine.ts (6)
1-8: Imports appear consistent and well-scoped
No issues found with these imports.
9-69: Documentation block is highly informative
The usage example and doc comments are comprehensive, providing clear guidance on how to use the RuleEngine.
70-82: Consider validating that function-based rules provide a name
When a rule is supplied as a function, ensure that the returned rule object includes a validnameproperty to avoid empty or undefined rule names used in error messages.Would you like a script to scan for all function-based rules and confirm they return a valid name property?
84-146: Sequential application of rules looks correct
The loop correctly applies each rule in turn, halting if there are no allowed releases left. The usage ofresult.allowedReleasesensures each subsequent rule receives the filtered set. This design is logically sound.
148-186: Straightforward final release selection
The selection strategy prioritizes sequential upgrades, followed by the desired release, and then defaults to the newest. This logic is clearly implemented and adheres to the documented behavior.
188-200: Metadata filtering for sequential upgrades is concise and clear
Filtering byrequiresSequentialUpgradeis an elegant approach, and the method returns aReleasescollection for easy chaining.packages/rule-engine/src/rules/version-cooldown-rule.ts (4)
1-35: Database and type definitions look consistent
The imports and type definitions align well with the rest of the codebase.
37-64: Efficient retrieval of last successful deployment
Using a single query with ordering and limiting is optimal for performance. No issues found.
80-88: Recurring concern about validating non-positive cooldown
This file still lacks validation for zero or negativecooldownMinutes. A previous comment already flagged it. Consider enforcing a positive integer check.
90-137: Cooldown logic is robust for positive intervals
The critical path from retrievinglastDeploymentTimeto computing remaining wait time appears correct. Returning a denial reason when the cooldown remains active is very user-friendly.packages/rule-engine/src/rules/webhook-check-rule.ts (3)
1-57: Initial definitions straightforward
Types and interfaces for webhook usage are well-defined, clearly describing the response format and caller function.
114-154: WebhookCheckRuleOptions are flexible
The options cater to different use cases (custom payloads, optional timeouts, etc.). The defaults seem reasonable.
176-190: Constructor parameters and defaults
The constructor elegantly initializes all options, providing a sensible default for delayed blocking behavior.
| const versionCooldownRule = () => | ||
| new VersionCooldownRule({ | ||
| cooldownMinutes: 1440, | ||
| getLastSuccessfulDeploymentTime: () => new Date(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the fixed return value for getLastSuccessfulDeploymentTime.
Currently, it always returns the current time, which can nullify the cooldown logic. Consider fetching the actual last successful deployment time to correctly enforce the cooldown period.
getLastSuccessfulDeploymentTime: () => new Date(),
+ // Suggestion:
+ // getLastSuccessfulDeploymentTime: async () => {
+ // // e.g., query DB for the actual last successful deployment
+ // return actualLastDeploymentTime;
+ // },📝 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.
| const versionCooldownRule = () => | |
| new VersionCooldownRule({ | |
| cooldownMinutes: 1440, | |
| getLastSuccessfulDeploymentTime: () => new Date(), | |
| }); | |
| const versionCooldownRule = () => | |
| new VersionCooldownRule({ | |
| cooldownMinutes: 1440, | |
| getLastSuccessfulDeploymentTime: () => new Date(), | |
| // Suggestion: | |
| // getLastSuccessfulDeploymentTime: async () => { | |
| // // e.g., query DB for the actual last successful deployment | |
| // return actualLastDeploymentTime; | |
| // }, | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rule-engine/src/evaluate.ts (1)
3-4: 🛠️ Refactor suggestionAdd missing return type import.
The
evaluatefunction returns the result fromruleEngine.evaluate(), which is of typeDeploymentResourceSelectionResult, but this type isn't imported.import type { DeploymentResourceContext } from "./types.js"; +import type { DeploymentResourceSelectionResult } from "./types.js"; import type { Releases } from "./utils/releases.js";
🧹 Nitpick comments (5)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (3)
89-92: Unused context parameter should be documentedThe context parameter is marked with an underscore to indicate it's not used, but there's no explanation why it's not needed.
Consider adding a comment explaining why the context isn't used or how it might be used in future implementations:
async filter( - _: DeploymentResourceContext, + // Context not currently used but required by interface + _context: DeploymentResourceContext, releases: Releases, ): Promise<DeploymentResourceRuleResult> {
138-147: Simplify best candidate selection logicThe current implementation of finding the best candidate is more complex than necessary and includes a redundant null coalescing operator.
Consider simplifying to:
- const bestCandidate = - disallowedReleases.length > 0 - ? disallowedReleases.reduce( - (best, current) => - best && current.count < best.count - ? current - : (best ?? current), - disallowedReleases[0], - ) - : null; + const bestCandidate = disallowedReleases.length > 0 + ? disallowedReleases.reduce( + (best, current) => current.count < best.count ? current : best, + disallowedReleases[0] + ) + : null;This simplification removes the redundant null check since
bestwill never be null due to the initialization withdisallowedReleases[0], and the entire expression only runs ifdisallowedReleases.length > 0.
129-131: Inconsistent release identification in error messagesThere's an inconsistency in how releases are identified in error messages. Line 130 uses
version.tagwhile line 149-150 usesrelease.id.For consistency, choose one identifier format for all error messages:
- reason = bestCandidate - ? `Gradual rollout limit reached for all release candidates. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.count}/${this.options.maxDeploymentsPerTimeWindow} deployments in the last ${this.options.timeWindowMinutes} minutes.` - : `Gradual rollout limit reached for all release candidates.`; + reason = bestCandidate + ? `Gradual rollout limit reached for all release candidates. Best candidate (${bestCandidate.release.version.tag}) has ${bestCandidate.count}/${this.options.maxDeploymentsPerTimeWindow} deployments in the last ${this.options.timeWindowMinutes} minutes.` + : `Gradual rollout limit reached for all release candidates.`;This makes both error messages use the version tag format, which is typically more meaningful to users than internal IDs.
Also applies to: 149-151
packages/db/src/schema/rule.ts (2)
128-137: Consider more precise time representation for TimeWindow.The PR comments describe time windows with specific hour precision (e.g., "Sa/03->Su/23"). While the current integer representation for hours works, consider if more precise time values (including minutes) might be needed for certain use cases.
export const ruleTimeWindow = pgTable("rule_time_window", { id: uuid("id").primaryKey().defaultRandom(), ruleId: uuid("rule_id") .notNull() .references(() => rule.id, { onDelete: "cascade" }), - startHour: integer("start_hour").notNull(), - endHour: integer("end_hour").notNull(), + startTime: text("start_time").notNull(), // Format: "HH:MM" + endTime: text("end_time").notNull(), // Format: "HH:MM" days: ruleTimeWindowDays("days").array().notNull(), timezone: text("timezone").notNull(), });
84-84: Remove or uncomment resourceSelector line.There's a commented out line for
resourceSelectorin the ruleResourceConcurrency table. Either remove it if it's not needed or uncomment it if it should be part of the schema.- // resourceSelector: json("resource_selector").default(sql`NULL`), + resourceSelector: json("resource_selector").default(sql`NULL`),Or simply remove the line if it's not needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/db/src/schema/index.ts(1 hunks)packages/db/src/schema/rule.ts(1 hunks)packages/rule-engine/src/evaluate.ts(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/gradual-version-rollout-rule.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/evaluate.tspackages/db/src/schema/index.tspackages/rule-engine/src/rule-engine.tspackages/rule-engine/src/rules/gradual-version-rollout-rule.tspackages/db/src/schema/rule.ts
🧬 Code Definitions (1)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (2)
packages/rule-engine/src/types.ts (4)
Release(3-13)DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/utils/releases.ts (1)
Releases(18-302)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (6)
1-11: Good job with import organization and type importsThe file is well-structured with clear separation between external dependencies and internal imports. Using named imports helps with clarity and tree-shaking.
16-39: Well-structured types with good documentationThe types are clearly defined with thorough JSDoc comments explaining their purpose and parameters. The naming is descriptive and follows TypeScript best practices.
41-58: Implementation correctly uses the time window for filteringThe
getRecentDeploymentCountfunction properly uses the time window parameter to filter recent deployments, addressing the issue noted in the previous review.Great job implementing the time-based filtering with:
gte(schema.job.createdAt, new Date(Date.now() - timeWindowMs))This ensures only deployments within the specified time window are counted.
60-74: Excellent class documentation with example usageThe JSDoc comments thoroughly explain the purpose of the rule and include a practical example. This makes it easy for other developers to understand how to use this class.
95-110: Good use of parallel processing for efficiencyThe implementation efficiently processes all releases in parallel using
Promise.all, which is a good practice for performance when dealing with potentially many async operations.
155-162: Clean result object constructionThe return statement is well-structured, conditionally creating a new Releases instance or returning an empty one. The ternary operator is appropriate here for its conciseness.
packages/db/src/schema/index.ts (1)
25-26: New schema exports added correctly.The new exports for release and rule schemas are properly added to the index file, making them available throughout the application.
packages/rule-engine/src/evaluate.ts (1)
36-45: TimeWindow implementation follows requirements from PR comments.The implementation of the TimeWindow rule correctly uses the parameters described in the PR comments, including days, start/end hours, and timezone.
packages/rule-engine/src/rule-engine.ts (3)
9-69: Well-documented RuleEngine implementation with comprehensive examples.The class documentation and examples are excellent, making it clear how to use the RuleEngine and what to expect from it. The examples include multiple rule types and show how to handle both successful and failed evaluations.
112-144: Robust evaluation implementation with proper error handling.The evaluate method correctly:
- Applies rules sequentially
- Stops evaluation when any rule disqualifies all candidates
- Provides clear error messages with rule names
- Handles the final selection logic
The error messages include both the rule name and additional context, which will be valuable for debugging.
167-184: Selection strategy follows described requirements.The selection strategy correctly prioritizes sequential upgrades first, then the desired release if specified, and finally defaults to the newest release. This aligns with the PR comments about proper ordering and selection.
packages/db/src/schema/rule.ts (1)
118-126: Days enum aligns with TimeWindow requirements.The ruleTimeWindowDays enum properly includes all days of the week, which aligns with the PR requirements for DayOfWeek pairs in time windows.
| export const evaluate = ( | ||
| rule: Rule, | ||
| releases: Releases, | ||
| context: DeploymentResourceContext, | ||
| ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type annotation to the evaluate function.
The function is missing an explicit return type annotation, which should be Promise<DeploymentResourceSelectionResult> to match the return type of ruleEngine.evaluate().
export const evaluate = (
rule: Rule,
releases: Releases,
context: DeploymentResourceContext,
-) => {
+): Promise<DeploymentResourceSelectionResult> => {
const ruleEngine = new RuleEngine([📝 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 evaluate = ( | |
| rule: Rule, | |
| releases: Releases, | |
| context: DeploymentResourceContext, | |
| ) => { | |
| export const evaluate = ( | |
| rule: Rule, | |
| releases: Releases, | |
| context: DeploymentResourceContext, | |
| ): Promise<DeploymentResourceSelectionResult> => { | |
| const ruleEngine = new RuleEngine([ | |
| // ... further implementation | |
| ]); | |
| // ... further implementation | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
25-29: 🛠️ Refactor suggestionRevisit non-positive cooldown acceptance.
A zero or negative
cooldownMinutesallows immediate or past-time deployments, which may be unintended. A past review comment raised this concern, and it remains unresolved. Consider validating thatcooldownMinutesis strictly positive.#!/bin/bash # Checking for usage of zero or negative cooldownMinutes in the codebase rg -A 5 'cooldownMinutes\s*:\s*0' rg -A 5 'cooldownMinutes\s*:\s*-[0-9]'packages/rule-engine/src/rules/previous-deploy-status-rule.ts (2)
152-156:⚠️ Potential issueProtect against empty releaseChecks array
The code assumes that
releaseChecksis not empty when finding the best candidate withreduce. If there are no candidates, this would cause an error.Add a check before reducing:
// If no releases allowed, find best candidate and return reason -const bestCandidate = releaseChecks.reduce((best, current) => - current.successfulDeployments > best.successfulDeployments - ? current - : best, -); +const bestCandidate = releaseChecks.length > 0 + ? releaseChecks.reduce((best, current) => + current.successfulDeployments > best.successfulDeployments + ? current + : best, + ) + : { release: { id: "none" }, successfulDeployments: 0 };
125-127: 💡 Verification agent❓ Verification inconclusive
Handle edge case when totalResources is 0
If
requireAllResourcesis true but the dependent environments have no resources,requiredDeploymentswill be 0, which would always pass validation.Add a check to handle this edge case:
const requiredDeployments = requireAllResources ? await this.getResourceCount(dependentEnvironments.map(({ id }) => id)) : (minSuccessfulDeployments ?? 0); + // If requiring all resources but none exist, we should fail or alert + if (requireAllResources && requiredDeployments === 0) { + const envNames = dependentEnvironments.map(({ name }) => name).join(", "); + return { + allowedReleases: Releases.empty(), + reason: `No resources found in dependent environments (${envNames}). Cannot validate deployment requirements.` + }; + }Let's verify if this check exists in the current codebase:
🏁 Script executed:
#!/bin/bash # Check if there's existing code that handles the zero resources edge case rg -A 5 "requiredDeployments === 0" --type ts packages/rule-engine/src/Length of output: 73
Action Required: Missing Edge-Case Handling for Zero Resources
After re-running the search for the expected check in the file, no evidence was found that the logic handling the case where no resources exist (i.e. when
requiredDeployments === 0underrequireAllResources) has been implemented. Please ensure the following diff is applied inpackages/rule-engine/src/rules/previous-deploy-status-rule.ts(around lines 125–127):const requiredDeployments = requireAllResources ? await this.getResourceCount(dependentEnvironments.map(({ id }) => id)) : (minSuccessfulDeployments ?? 0); + // If requiring all resources but none exist, we should fail or alert accordingly. + if (requireAllResources && requiredDeployments === 0) { + const envNames = dependentEnvironments.map(({ name }) => name).join(", "); + return { + allowedReleases: Releases.empty(), + reason: `No resources found in dependent environments (${envNames}). Cannot validate deployment requirements.` + }; + }Please update the code to properly handle this edge case so that deployments without any available resources do not erroneously pass validation.
🧹 Nitpick comments (16)
packages/rule-engine/src/evaluate.ts (1)
52-64: Consider adding an explicit return type to theevaluatefunction.While TypeScript can infer the return type from
ruleEngine.evaluate(...), providing an explicit return type (e.g.,Promise<DeploymentResourceRuleResult>) can improve readability and maintainability.-export const evaluate = ( +export const evaluate = async ( rule: Rule, releases: Releases, context: DeploymentResourceContext, ): Promise<DeploymentResourceRuleResult> => { const ruleEngine = new RuleEngine([ maintenanceWindows(rule), ...versionMetadataValidation(rule), ...timeWindow(rule), ...gradualVersionRollout(rule), ]); return ruleEngine.evaluate(releases, context); };packages/rule-engine/src/rules/maintenance-window-rule.ts (1)
46-81: Add edge-case test coverage.The logic correctly blocks deployments within active windows. However, consider adding test scenarios for:
- Overlapping maintenance windows
- Zero-duration windows (
start === end)
This ensures robust handling of boundary conditions.packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (1)
89-108: Consider handling potential Promise.all rejectionWhen using
Promise.all, if any single promise is rejected, the entire operation fails. You should consider adding error handling for individual promises to prevent one failed deployment count check from causing the entire rule evaluation to fail.- const releaseChecks = await Promise.all( - releases.getAll().map(async (release) => { - const recentDeployments = await this.options.getRecentDeploymentCount( - release, - timeWindowMs, - ); + const releaseChecks = await Promise.allSettled( + releases.getAll().map(async (release) => { + try { + const recentDeployments = await this.options.getRecentDeploymentCount( + release, + timeWindowMs, + ); + + return { + release, + recentDeployments, + isAllowed: + recentDeployments < this.options.maxDeploymentsPerTimeWindow, + }; + } catch (error) { + console.error(`Error checking deployment count for release ${release.id}:`, error); + // Default to not allowed if we can't verify the count + return { + release, + recentDeployments: this.options.maxDeploymentsPerTimeWindow, + isAllowed: false, + error: String(error), + }; + } + }), + ); + + // Process the results, filtering out rejected promises + const processedChecks = releaseChecks + .filter((result): result is PromiseFulfilledResult<any> => result.status === 'fulfilled') + .map(result => result.value);packages/rule-engine/src/types.ts (2)
3-13: Consider stronger typing for config and variablesThe
configandvariablesproperties useRecord<string, any>andRecord<string, unknown>, which are very generic. Consider using more specific types if the structure of these objects is known, or using TypeScript's utility types to improve type safety.export type Release = { id: string; createdAt: Date; version: { id: string; tag: string; - config: Record<string, any>; + config: Record<string, unknown>; metadata: Record<string, string>; }; variables: Record<string, unknown>; };
15-20: Be more specific with selector object typesThe
resourceSelectorandversionSelectorproperties are typed asobject, which is too generic. Consider using a more specific type or interface that describes the expected shape of these selectors.export type Deployment = { id: string; name: string; - resourceSelector?: object; - versionSelector?: object; + resourceSelector?: Record<string, unknown>; + versionSelector?: Record<string, unknown>; };packages/rule-engine/src/rules/version-metadata-validation-rule.ts (4)
91-95: Initialization in constructor could be simplifiedThe constructor initializes default values, but could be simplified using parameter defaults.
- constructor(private options: MetadataValidationRuleOptions) { - this.options.allowMissingMetadata = options.allowMissingMetadata ?? false; - this.options.checkEnvironmentSpecificValues = - options.checkEnvironmentSpecificValues ?? false; - } + constructor(private options: MetadataValidationRuleOptions) { + // Set defaults directly through destructuring and reassignment + this.options = { + ...options, + allowMissingMetadata: options.allowMissingMetadata ?? false, + checkEnvironmentSpecificValues: options.checkEnvironmentSpecificValues ?? false, + }; + }
115-125: Environment pattern matching could be more preciseThe current implementation uses
includes()for pattern matching, which could match substrings unintentionally. For example, "prod" would match "production" and also "reproductionapi".Consider using more precise pattern matching like regex or exact matches:
- const shouldApply = environmentPatterns.some((pattern) => - envName.includes(pattern.toLowerCase()), - ); + // More precise matching using regex boundary or exact matches + const shouldApply = environmentPatterns.some((pattern) => { + const lowerPattern = pattern.toLowerCase(); + // Match exact environment name or pattern with word boundaries + return envName === lowerPattern || + new RegExp(`\\b${lowerPattern}\\b`).test(envName); + });
127-143: Consider a more functional approach for filter logicThe filter logic is correct but could be refactored for better readability using a more declarative approach.
- const allowedReleases: Releases = releases.filter((release) => { - // If we're checking environment-specific values, try that first - if (checkEnvironmentSpecificValues) { - const envSpecificKey = `${metadataKey}.${context.environment.name.toLowerCase()}`; - const envSpecificValue = release.version.metadata[envSpecificKey]; - - if (envSpecificValue !== undefined) { - // If an environment-specific value exists, use it - return envSpecificValue === requiredValue; - } - } - - const metadataValue = release.version.metadata[metadataKey]; - return metadataValue == null - ? (allowMissingMetadata ?? false) - : metadataValue === requiredValue; - }); + const allowedReleases: Releases = releases.filter((release) => { + // Check for environment-specific metadata value + if (checkEnvironmentSpecificValues) { + const envSpecificKey = `${metadataKey}.${context.environment.name.toLowerCase()}`; + const envSpecificValue = release.version.metadata[envSpecificKey]; + + // If environment-specific value exists, use it exclusively + if (envSpecificValue !== undefined) { + return envSpecificValue === requiredValue; + } + } + + // Fall back to standard metadata check + const metadataValue = release.version.metadata[metadataKey]; + + // Allow missing metadata if configured, otherwise require matching value + return metadataValue == null + ? allowMissingMetadata + : metadataValue === requiredValue; + });
145-160: Handle string templating securelyThe code uses string replacement for custom error messages, which is fine but could be more robust. Ensure that the values being substituted don't contain special characters that could affect the message formatting.
- if (customErrorMessage) { - reason = customErrorMessage - .replace("{key}", metadataKey) - .replace("{value}", requiredValue); + if (customErrorMessage) { + // Safely replace placeholders with values + reason = customErrorMessage + .replace(/\{key\}/g, metadataKey) + .replace(/\{value\}/g, requiredValue);packages/rule-engine/src/rule-engine.ts (1)
195-198: Consider parameterizing the sequential upgrade metadata keyThe
findSequentialUpgradeReleasesmethod uses a hardcoded metadata key "requiresSequentialUpgrade". Consider making this configurable to allow for different metadata keys.+ private sequentialUpgradeMetadataKey = "requiresSequentialUpgrade"; + private findSequentialUpgradeReleases(releases: Releases): Releases { // Look for the standard metadata key used by SequentialUpgradeRule - return releases.filterByMetadata("requiresSequentialUpgrade", "true"); + return releases.filterByMetadata(this.sequentialUpgradeMetadataKey, "true"); }packages/rule-engine/src/rules/version-cooldown-rule.ts (1)
17-20: Unify the return type for consistency.Currently, the type can return either a Promise or a direct Date/null, which can be confusing to callers. Ensuring a consistently asynchronous or synchronous return type in
GetLastSuccessfulDeploymentTimewould improve predictability.packages/rule-engine/src/releases.ts (1)
18-18: Consider aligning the class name with the documentation.The doc comments mention “CandidateReleases,” while the class is named
Releases. Renaming it would improve clarity and consistency.-export class Releases { +export class CandidateReleases {packages/rule-engine/src/rules/previous-deploy-status-rule.ts (4)
141-149: Return types inconsistency with allowedReleasesIn one branch, the method returns
{ allowedReleases: releases }(line 123) using the originalReleasesobject, but in another branch, it returns{ allowedReleases: new Releases(allowedReleases) }(line 149) constructing a new instance.For consistency, consider always returning a new
Releasesinstance:if (!hasDependentEnvironments || !hasMinimumRequirement) - return { allowedReleases: releases }; + return { allowedReleases: new Releases(releases.getAll()) }; // ... if (allowedReleases.length > 0) return { allowedReleases: new Releases(allowedReleases) };
95-100: Consider more explicit default behaviorIf neither
requireAllResourcesnorminSuccessfulDeploymentsis provided, the code defaults tominSuccessfulDeployments = 0, which effectively disables the rule since any deployment would satisfy zero required deployments.Consider making this behavior more explicit:
// Set default values if ( this.options.requireAllResources == null && this.options.minSuccessfulDeployments == null ) { + // Default to no minimum requirements if not specified this.options.minSuccessfulDeployments = 0; }
158-161: Improve error message formatting for readabilityThe error messages contain a lot of information which can be hard to parse quickly. Consider formatting them for better readability.
Format the error messages for easier scanning:
const reasonMessage = this.options.requireAllResources - ? `Not all resources in ${envNames} have been successfully deployed for any release candidate. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.successfulDeployments}/${requiredDeployments} deployments.` - : `Minimum deployment requirement not met for any release candidate. Need at least ${requiredDeployments} successful deployments in ${envNames}. Best candidate (${bestCandidate.release.id}) has ${bestCandidate.successfulDeployments} deployments.`; + ? `Not all resources in ${envNames} have been successfully deployed for any release candidate. + Best candidate: ${bestCandidate.release.id} + Current status: ${bestCandidate.successfulDeployments}/${requiredDeployments} deployments` + : `Minimum deployment requirement not met for any release candidate. + Required: At least ${requiredDeployments} successful deployments in ${envNames} + Best candidate: ${bestCandidate.release.id} (${bestCandidate.successfulDeployments} deployments)`;
26-51: Enhance documentation with additional examplesThe options type is well documented, but considering the complexity of the rule, additional examples might be helpful.
Consider adding examples of how to provide custom implementations for the resource and deployment counting functions:
/** * Options for configuring the PreviousDeployStatusRule * * @example * ```ts * // With custom resource counting * new PreviousDeployStatusRule({ * dependentEnvironments: [{ name: "staging", id: "staging" }], * requireAllResources: true, * getResourceCount: async (envIds) => { * // Custom implementation * return await db.countResourcesInEnvironments(envIds); * } * }); * ``` */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/rule-engine/src/evaluate.ts(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/releases.ts(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/__tests__/sequential-upgrade-multiple.test.ts(1 hunks)packages/rule-engine/src/rules/approval-required-rule.ts(1 hunks)packages/rule-engine/src/rules/gradual-version-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/maintenance-window-rule.ts(1 hunks)packages/rule-engine/src/rules/previous-deploy-status-rule.ts(1 hunks)packages/rule-engine/src/rules/resource-concurency-rule.ts(1 hunks)packages/rule-engine/src/rules/sequential-upgrade-rule.ts(1 hunks)packages/rule-engine/src/rules/time-window-rule.ts(1 hunks)packages/rule-engine/src/rules/version-cooldown-rule.ts(1 hunks)packages/rule-engine/src/rules/version-metadata-validation-rule.ts(1 hunks)packages/rule-engine/src/rules/webhook-check-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rule-engine/src/rules/tests/sequential-upgrade-multiple.test.ts
- packages/rule-engine/src/rules/index.ts
- packages/rule-engine/src/rules/approval-required-rule.ts
- packages/rule-engine/src/rules/sequential-upgrade-rule.ts
- packages/rule-engine/src/rules/time-window-rule.ts
- packages/rule-engine/src/rules/webhook-check-rule.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/evaluate.tspackages/rule-engine/src/rules/maintenance-window-rule.tspackages/rule-engine/src/rules/gradual-version-rollout-rule.tspackages/rule-engine/src/index.tspackages/rule-engine/src/rules/version-cooldown-rule.tspackages/rule-engine/src/types.tspackages/rule-engine/src/rule-engine.tspackages/rule-engine/src/rules/version-metadata-validation-rule.tspackages/rule-engine/src/rules/previous-deploy-status-rule.tspackages/rule-engine/src/rules/resource-concurency-rule.tspackages/rule-engine/src/releases.ts
🧬 Code Definitions (8)
packages/rule-engine/src/evaluate.ts (6)
packages/db/src/schema/rule.ts (8)
Rule(150-150)RuleRollout(152-152)RuleApproval(153-153)RuleMaintenanceWindow(154-154)RuleResourceConcurrency(155-156)RuleVersionMetadataValidation(159-160)RuleTimeWindow(161-161)rule(15-32)packages/rule-engine/src/rules/maintenance-window-rule.ts (1)
MaintenanceWindowRule(46-81)packages/rule-engine/src/rules/version-metadata-validation-rule.ts (1)
VersionMetadataValidationRule(88-164)packages/rule-engine/src/rules/time-window-rule.ts (1)
TimeWindowRule(58-122)packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (1)
GradualVersionRolloutRule(75-163)packages/rule-engine/src/types.ts (1)
DeploymentResourceContext(33-38)
packages/rule-engine/src/rules/maintenance-window-rule.ts (4)
packages/rule-engine/src/rules/index.ts (1)
MaintenanceWindowRule(5-5)packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/index.ts (1)
Releases(8-8)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (2)
packages/rule-engine/src/types.ts (4)
Release(3-13)DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/types.ts (3)
packages/db/src/schema/resource.ts (1)
Resource(106-106)packages/rule-engine/src/index.ts (1)
Releases(8-8)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rule-engine.ts (3)
packages/rule-engine/src/index.ts (2)
RuleEngine(2-2)Releases(8-8)packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceSelectionResult(48-52)Release(3-13)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/version-metadata-validation-rule.ts (4)
packages/rule-engine/src/rules/index.ts (1)
VersionMetadataValidationRule(6-6)packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/index.ts (1)
Releases(8-8)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/previous-deploy-status-rule.ts (4)
packages/rule-engine/src/rules/index.ts (1)
PreviousDeployStatusRule(7-7)packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/index.ts (1)
Releases(8-8)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/resource-concurency-rule.ts (2)
packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(57-63)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(43-46)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (26)
packages/rule-engine/src/index.ts (1)
1-8: Barrel exports look good.Exporting from multiple modules in this barrel-style file is clear and maintainable. No concerns noted.
packages/rule-engine/src/rules/resource-concurency-rule.ts (2)
31-35: Double-check ID matching for resource concurrency.A past review comment indicated that
releaseJobTrigger.idis the primary key for the trigger record, not necessarily the resource ID. If you want to filter by resource, consider usingreleaseJobTrigger.resourceIdinstead:.where( and( - eq(schema.releaseJobTrigger.id, resourceId), + eq(schema.releaseJobTrigger.resourceId, resourceId), inArray(schema.job.status, [JobStatus.InProgress, JobStatus.Pending]), ), )
55-60: Validate concurrency limit on construction.A previous comment suggested throwing an error if
concurrencyLimitis zero or negative to avoid unintentionally blocking all deployments:constructor(private options: ResourceConcurrencyRuleOptions = { concurrencyLimit: 1, getRunningCount, }) { + if (this.options.concurrencyLimit < 1) { + throw new Error("Concurrency limit must be a positive integer."); + } }packages/rule-engine/src/rules/gradual-version-rollout-rule.ts (9)
1-12: Good organization of imports and typesThe imports and type definitions are well-organized. I especially like the clear separation between external dependencies and internal imports.
16-19: LGTM! Well-designed function type signatureThe
GetRecentDeploymentCountFunctiontype is well-defined with clear parameter types and return type flexibility.
24-39: Well-documented options typeThe
GradualVersionRolloutRuleOptionstype has excellent JSDoc comments explaining each option's purpose and behavior.
41-58: Implementation correctly uses time window filteringYour implementation of
getRecentDeploymentCountcorrectly filters jobs by creation time using the provided time window, which matches the past review comment that was addressed.
60-74: Excellent documentation with example codeThe class-level JSDoc documentation is thorough and includes a practical example that shows how to use the rule.
75-78: Proper interface implementationThe class correctly implements the
DeploymentResourceRuleinterface with the required name property.
113-123: Good separation of concerns for clarityThe code separates filtering allowed and disallowed releases for clarity, which improves readability.
127-152: Comprehensive reason messages with detailed informationThe rule provides very informative reason messages, especially for when all releases are disallowed. The message includes specific release tags and limits, which will be helpful for debugging and user understanding.
155-162: Defensive programming with empty releases checkThe code properly handles the case when there are no allowed releases by using the
Releases.empty()factory method, which is a good practice.packages/rule-engine/src/types.ts (3)
22-31: LGTM! Clear and concise type definitionsThe
ResourceandEnvironmenttypes are well-defined with clear property names.
33-46: Good organization and documentationThe
DeploymentResourceContextandDeploymentResourceRuleResulttypes are well-organized with a clear comment explaining the purpose of the rule result type.
48-63: Well-defined interface and selection result typeThe
DeploymentResourceSelectionResulttype andDeploymentResourceRuleinterface provide a clean contract for rule implementation and selection results.packages/rule-engine/src/rules/version-metadata-validation-rule.ts (2)
11-47: Excellent documentation of options typeThe
MetadataValidationRuleOptionstype is very well-documented with clear explanations of each option, including default values and examples of usage patterns.
49-87: Comprehensive class documentation with multiple examplesThe class documentation is excellent, providing clear examples for different use cases which makes the API much easier to understand and use correctly.
packages/rule-engine/src/rule-engine.ts (3)
70-83: Well-designed constructor with flexible rule inputThe constructor accepts an array of rules that can be either direct instances or factory functions, which provides good flexibility for rule initialization.
133-144: Good null handling with clear error messageThe code properly handles the case when no release is chosen after applying all rules, providing a clear error message.
167-184: Well-implemented selection strategyThe
selectFinalReleasemethod implements a clear strategy for selecting the final release, with priority given to sequential upgrades, then desired release ID, then newest release.packages/rule-engine/src/rules/version-cooldown-rule.ts (2)
45-64: Database lookup logic is solid.The query using
orderBy(desc(...))andlimit(1)effectively retrieves the most recent successful job. This approach looks correct and straightforward.
83-138: Cooldown enforcement logic is robust.The flow correctly computes the earliest allowed time, checks the current time, and provides a clear message when blocking. This is well-implemented.
packages/rule-engine/src/releases.ts (2)
67-94: Well-implemented release age checks.
getOldest()andgetNewest()correctly reduce over the collection by comparing creation dates, ensuring accurate retrieval of boundary releases.
103-123: Great fallback logic for deployment targets.In
getEffectiveTarget(), returning the desired release when available or falling back to the newest release is both clear and intuitive.packages/rule-engine/src/rules/previous-deploy-status-rule.ts (2)
53-56: Replace placeholder implementation with error before productionThe default implementation of
getResourceCountsimply returns 0 with a TODO comment. This approach could lead to incorrect validation results if a proper implementation isn't provided.Consider throwing an explicit error instead to prevent silent failures:
const getResourceCount: GetResourceCountFunction = (_: string[]) => { - // TODO: Sum of all resources in the dependent environments - return 0; + // Implement actual resource counting logic + throw new Error("getResourceCount not implemented. Please provide an implementation."); };
58-64: Replace placeholder implementation with error before productionSimilarly, the default implementation of
getSuccessfulDeploymentsreturns 0 with a TODO comment, which could lead to false validation results.Consider throwing an explicit error instead:
const getSuccessfulDeployments: GetSuccessfulDeploymentsFunction = ( _: string, __: string[], ) => { - // TODO: Count of successful deployments in the dependent environments - return 0; + // Implement actual deployment counting logic + throw new Error("getSuccessfulDeployments not implemented. Please provide an implementation."); };
| async evaluate( | ||
| releases: Releases, | ||
| context: DeploymentResourceContext, | ||
| ): Promise<DeploymentResourceSelectionResult> { | ||
| // Apply each rule in sequence to filter candidate releases | ||
| for (const rule of this.rules) { | ||
| const result = await ( | ||
| typeof rule === "function" ? await rule() : rule | ||
| ).filter(context, releases); | ||
|
|
||
| // If the rule yields no candidates, we must stop. | ||
| if (result.allowedReleases.isEmpty()) { | ||
| return { | ||
| allowed: false, | ||
| reason: `${rule.name} disqualified all versions. Additional info: ${result.reason ?? ""}`, | ||
| }; | ||
| } | ||
|
|
||
| releases = result.allowedReleases; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix double await in rule initialization
There's a double await in the rule initialization logic which is unnecessary and could be simplified.
- const result = await (
- typeof rule === "function" ? await rule() : rule
- ).filter(context, releases);
+ // Resolve the rule (if it's a factory function) before calling filter
+ const ruleInstance = typeof rule === "function" ? await rule() : rule;
+ const result = await ruleInstance.filter(context, releases);📝 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.
| async evaluate( | |
| releases: Releases, | |
| context: DeploymentResourceContext, | |
| ): Promise<DeploymentResourceSelectionResult> { | |
| // Apply each rule in sequence to filter candidate releases | |
| for (const rule of this.rules) { | |
| const result = await ( | |
| typeof rule === "function" ? await rule() : rule | |
| ).filter(context, releases); | |
| // If the rule yields no candidates, we must stop. | |
| if (result.allowedReleases.isEmpty()) { | |
| return { | |
| allowed: false, | |
| reason: `${rule.name} disqualified all versions. Additional info: ${result.reason ?? ""}`, | |
| }; | |
| } | |
| releases = result.allowedReleases; | |
| } | |
| async evaluate( | |
| releases: Releases, | |
| context: DeploymentResourceContext, | |
| ): Promise<DeploymentResourceSelectionResult> { | |
| // Apply each rule in sequence to filter candidate releases | |
| for (const rule of this.rules) { | |
| // Resolve the rule (if it's a factory function) before calling filter | |
| const ruleInstance = typeof rule === "function" ? await rule() : rule; | |
| const result = await ruleInstance.filter(context, releases); | |
| // If the rule yields no candidates, we must stop. | |
| if (result.allowedReleases.isEmpty()) { | |
| return { | |
| allowed: false, | |
| reason: `${rule.name} disqualified all versions. Additional info: ${result.reason ?? ""}`, | |
| }; | |
| } | |
| releases = result.allowedReleases; | |
| } | |
| } |
| async filter( | ||
| _: DeploymentResourceContext, | ||
| releases: Releases, | ||
| ): Promise<DeploymentResourceRuleResult> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Context parameter is unused but potentially valuable
The filter method doesn't use the DeploymentResourceContext parameter (renamed to _). This context contains the desired release ID which could be important for validation.
Consider using the context parameter to verify if the desired release meets the requirements:
async filter(
- _: DeploymentResourceContext,
+ ctx: DeploymentResourceContext,
releases: Releases,
): Promise<DeploymentResourceRuleResult> {
// ... existing code ...
// Early validation for the desired release
+ if (ctx.desiredReleaseId) {
+ const desiredRelease = releases.getAll().find(r => r.id === ctx.desiredReleaseId);
+ if (!desiredRelease) {
+ return {
+ allowedReleases: releases,
+ reason: "Desired release not found in candidates"
+ };
+ }
+ }Let's verify how other rules use the context parameter:
🏁 Script executed:
#!/bin/bash
# Check how other rules use the DeploymentResourceContext parameter
rg -A 3 "context: DeploymentResourceContext" --type ts packages/rule-engine/src/Length of output: 8844
Enhance release validation by leveraging DeploymentResourceContext
The filter method in previous-deploy-status-rule.ts currently ignores its context parameter by renaming it to _. Given that other rules use this parameter to validate the desired release (via desiredReleaseId), please consider refactoring this method. For example, rename the parameter to ctx and add an early check to verify that the desired release exists:
async filter(
- _: DeploymentResourceContext,
+ ctx: DeploymentResourceContext,
releases: Releases,
): Promise<DeploymentResourceRuleResult> {
// ... existing code ...
// Early validation for the desired release
+ if (ctx.desiredReleaseId) {
+ const desiredRelease = releases.getAll().find(r => r.id === ctx.desiredReleaseId);
+ if (!desiredRelease) {
+ return {
+ allowedReleases: releases,
+ reason: "Desired release not found in candidates"
+ };
+ }
+ }This change aligns with the approach used in other rules within the codebase.
📝 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.
| async filter( | |
| _: DeploymentResourceContext, | |
| releases: Releases, | |
| ): Promise<DeploymentResourceRuleResult> { | |
| async filter( | |
| ctx: DeploymentResourceContext, | |
| releases: Releases, | |
| ): Promise<DeploymentResourceRuleResult> { | |
| // ... existing code ... | |
| // Early validation for the desired release | |
| if (ctx.desiredReleaseId) { | |
| const desiredRelease = releases.getAll().find(r => r.id === ctx.desiredReleaseId); | |
| if (!desiredRelease) { | |
| return { | |
| allowedReleases: releases, | |
| reason: "Desired release not found in candidates" | |
| }; | |
| } | |
| } | |
| // ... rest of the code ... | |
| } |
Summary by CodeRabbit
New Features
Tests
Documentation