init rate rollouts #459
init rate rollouts #459
Conversation
WalkthroughThis pull request introduces a new rollout rule for deployments by implementing the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Rule as RateRolloutRule
participant Clock
Client->>Rule: filter(context, releases)
Rule->>Clock: Retrieve current time
Clock-->>Rule: Return fixed/current time
Rule->>Rule: Iterate releases, calculate rollout %
Rule->>Client: Return allowed releases & rejection reasons
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 (3)
packages/rule-engine/src/rules/rate-rollout-rule.ts (2)
41-91: Consider clamping negative ages for safety.If a release has a future timestamp,
differenceInSecondsmight yield a negative value. While unlikely, it could impact the rollout logic. Consider clamping to zero:-const releaseAge = differenceInSeconds(now, releaseCreatedAt); +const releaseAge = Math.max(0, differenceInSeconds(now, releaseCreatedAt));This ensures the rollout process never calculates a negative percentage.
93-99: Optional fallback torelease.createdAtif version date is absent.Currently,
getVersionCreatedAtalways returnsrelease.version.createdAt. If there's any chanceversion.createdAtmight be undefined, consider a fallback:-private getVersionCreatedAt(release: ResolvedRelease): Date { - return release.version.createdAt; +private getVersionCreatedAt(release: ResolvedRelease): Date { + return release.version.createdAt ?? release.createdAt; }packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (1)
41-59: Well-designed helper function for creating test dataThe
createMockReleasesfunction nicely abstracts the creation of test data, making the test cases cleaner and more readable.Consider adding a comment explaining why both the release and version have the same creation date, and you might be able to remove the type assertion
as ResolvedReleaseif the object literal is properly typed.const createMockReleases = (releaseTimes: number[]): Releases => { const releases = releaseTimes.map((minutesAgo, index) => { const createdAt = new Date(baseDate.getTime() - minutesAgo * 60 * 1000); + // Both release and version share the same creation timestamp for testing simplicity return { id: `release-${index}`, createdAt, version: { id: `version-${index}`, tag: `v0.${index}.0`, config: {}, metadata: {}, createdAt, }, variables: {}, - } as ResolvedRelease; + }; }); return new Releases(releases); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/rules/rate-rollout-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/types.tspackages/rule-engine/src/rules/rate-rollout-rule.tspackages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.tspackages/rule-engine/src/rules/index.ts
🧬 Code Definitions (2)
packages/rule-engine/src/rules/rate-rollout-rule.ts (1)
packages/rule-engine/src/types.ts (4)
DeploymentResourceRule(59-65)DeploymentResourceContext(38-43)DeploymentResourceRuleResult(45-48)ResolvedRelease(7-18)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (2)
packages/rule-engine/src/types.ts (1)
ResolvedRelease(7-18)packages/rule-engine/src/rules/rate-rollout-rule.ts (1)
RateRolloutRule(23-113)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
packages/rule-engine/src/rules/index.ts (1)
2-2: Exporting the new rate rollout rule.This addition looks good. It cleanly exposes the new
RateRolloutRulefor external usage.packages/rule-engine/src/types.ts (1)
15-15:❓ Verification inconclusive
Verify the rationale for adding another
createdAt.Both the
ResolvedReleaseand itsversionobject now have acreatedAtproperty. If these timestamps can differ, this is valid. Otherwise, consider unifying to avoid confusion.Below is a script to locate all references to
createdAtin the codebase so you can confirm consistent usage:
🏁 Script executed:
#!/bin/bash rg -A 5 'createdAt'Length of output: 74147
Clarify the Distinct Roles for the Two
createdAtPropertiesIt appears that in
packages/rule-engine/src/types.tsthe release object has its owncreatedAtproperty while the nestedversionobject also includes acreatedAt. Notably, functions such as the one inpackages/rule-engine/src/rules/rate-rollout-rule.tsexplicitly referenceversion.createdAt, suggesting that the version timestamp may represent a different event (for example, the time the version was defined) compared to the overall release creation time.
- Release vs. Version Timestamp:
- The top-level
createdAtlikely indicates when the release record was created.- The nested
version.createdAtappears to capture when the version itself was established, and is actively used in the rate rollout logic.Please verify that this distinction is intentional. If these two timestamps can indeed differ, the current implementation is valid. If they are intended to be identical, consider unifying them to avoid potential confusion.
packages/rule-engine/src/rules/rate-rollout-rule.ts (6)
1-2: Use ofdate-fnslibrary is appropriate.
differenceInSecondsfromdate-fnsis a clean approach for time-based calculations, and it keeps the code readable.
3-9: Proper import of type definitions.The necessary interfaces and types are being imported to implement the
DeploymentResourceRule. Everything looks consistent.
11-21: Clear interface definition for the rollout rule.The
rolloutDurationSecondsanddenyReasonproperties provide flexibility. The documentation comments are concise and helpful.
23-34: Constructor sets up essential parameters.Storing the rollout duration and the custom deny reason is straightforward. The default deny reason is a good fallback for cases when one isn't provided.
36-40: Good testability design withgetCurrentTime.Providing a dedicated method for retrieving the current time improves test control and reliability. No issues here.
100-112: Simple hash-based rollout logic.This deterministic hashing approach is a quick way to generate a 0-100 range. For small IDs, collisions are unlikely problematic. No major concerns.
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (6)
1-6: Imports are well organized and appropriateThe test file correctly imports the necessary utilities from Vitest and the required components from the codebase.
7-22: Well-structured test contextThe mock deployment context provides all the necessary properties that would be used by the
RateRolloutRuleclass during testing, matching what would be expected by the rule's filter method.
24-39: Good time-based testing setupThe date mocking implementation is well-executed:
- Using a fixed base date ensures consistent test results
- Proper test hygiene with reset and restore operations in the appropriate hooks
- The spying approach on the Date constructor is correct for controlling time during tests
61-79: Complete rollout test is well implementedThe test for complete rollout is thorough:
- It correctly tests that releases created long ago (beyond the rollout period) are allowed
- The hash value mocking ensures deterministic test results
- Both allowed releases and rejection reasons are verified
Note: The test expects
result.rejectionReasonsto beundefinedwhen all releases are allowed rather than an empty collection. This appears to be the intended behavior of the implementation.
81-129: Partial rollout test is comprehensiveThis test thoroughly validates the partial rollout scenario:
- It correctly simulates different release ages relative to the rollout window
- The deterministic hash values are well-chosen to test boundary conditions
- It properly verifies both allowed and rejected releases
- The test correctly checks that rejection reasons exist for denied releases
131-159: Rejection reason test provides good coverageThis test effectively validates the format and content of rejection reasons:
- It correctly forces a release to be denied with a high hash value
- It verifies both the percentage completion and time remaining components
- The regex pattern appropriately matches the expected time format
The test ensures users will receive clear, informative rejection messages with accurate timing estimates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (2)
41-59: Add test coverage for version creation date edge casesConsider adding tests for scenarios where
version.createdAtandrelease.createdAtdiffer, since the implementation inrate-rollout-rule.tsspecifically usesversion.createdAtfor age calculation.it("should use version.createdAt instead of release.createdAt for age calculation", () => { // Mock the Date constructor to return a fixed "now" time const now = new Date("2025-01-01T01:00:00Z"); // 1 hour from base dateSpy.mockImplementation(() => now); // Create a rule with a 2-hour rollout period const rule = new RateRolloutRule({ rolloutDurationSeconds: 7200, // 2 hours }); // Create a release where version.createdAt and release.createdAt differ const release = { id: "release-diff-dates", createdAt: new Date(now.getTime() - 30 * 60 * 1000), // 30 min ago (25%) version: { id: "version-diff-dates", tag: "v1.0.0", config: {}, metadata: {}, createdAt: new Date(now.getTime() - 90 * 60 * 1000), // 90 min ago (75%) }, variables: {}, } as ResolvedRelease; const releases = new Releases([release]); // Mock hash value to be between 25% and 75% vi.spyOn(rule as any, "getHashValue").mockReturnValue(50); const result = rule.filter(mockDeploymentContext, releases); // Should be allowed based on version.createdAt (75% > 50%), not release.createdAt expect(result.allowedReleases.length).toBe(1); });
92-94: Test custom deny reason parameterConsider adding a test case that verifies the custom deny reason parameter works correctly. The current tests only use the default deny reason.
it("should use custom deny reason when provided", () => { const now = new Date("2025-01-01T00:30:00Z"); dateSpy.mockImplementation(() => now); const customDenyReason = "Custom deny reason for testing"; const rule = new RateRolloutRule({ rolloutDurationSeconds: 7200, denyReason: customDenyReason, }); const releases = createMockReleases([10]); vi.spyOn(rule as any, "getHashValue").mockReturnValue(100); const result = rule.filter(mockDeploymentContext, releases); const rejectionReason = result.rejectionReasons?.get("release-0"); expect(rejectionReason).toContain(customDenyReason); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(2 hunks)packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rule-engine/src/rules/tests/deployment-deny-rule.test.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__/rate-rollout-rule.test.ts
🧬 Code Definitions (1)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (2)
packages/rule-engine/src/types.ts (1)
ResolvedRelease(7-18)packages/rule-engine/src/rules/rate-rollout-rule.ts (1)
RateRolloutRule(23-113)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (7)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (7)
1-6: LGTM: Appropriate imports for testing the RateRolloutRuleThe imports are well-organized, including necessary test utilities from Vitest and the required types and classes from the project.
7-22: LGTM: Well-structured mock deployment contextThe mock deployment context provides all the necessary properties for testing the rule's filter method with a realistic context object.
24-39: LGTM: Effective date mocking setupGood approach for ensuring deterministic date behavior in tests by:
- Setting a fixed base date
- Spying on the Date constructor
- Properly resetting mocks between tests
- Restoring all mocks after tests
This ensures reliable and repeatable test results for time-based functionality.
41-59: LGTM: Useful mock release creation helperThe
createMockReleaseshelper function is well-designed:
- Takes an array of "minutes ago" values for flexible test data creation
- Generates proper timestamps relative to the base date
- Creates complete release objects with all required properties
- Returns a properly instantiated Releases collection
This approach reduces code duplication and makes tests more maintainable.
61-84: LGTM: Comprehensive test for complete rollout scenarioThis test effectively verifies that releases beyond the rollout period are allowed:
- Creates releases with timestamps far in the past
- Sets up a short rollout period (10 minutes)
- Mocks hash values deterministically
- Properly verifies that all releases are allowed
The test covers an essential use case for the rollout rule.
86-146: LGTM: Thorough test for partial rollout behaviorThis test case effectively validates the core functionality of the rule:
- Sets up a specific "now" time one hour after the base date
- Creates a 2-hour rollout period for testing partial rollout
- Creates releases at various points in the rollout window (25%, 50%, 75%, 100%)
- Mocks hash values to create deterministic test conditions
- Verifies each release is correctly allowed or rejected based on its age and hash
The detailed assertions for each release ensure the rule's behavior is thoroughly tested.
148-176: LGTM: Appropriate test for rejection reasoningThis test effectively verifies the rejection reason formatting:
- Creates a test environment with a specific "now" time
- Forces a rejection by setting a high hash value
- Verifies that the rejection reason includes the correct percentage (8%)
- Checks that the remaining time is properly formatted (~1h Xm)
This ensures the rule provides helpful information to users when releases are rejected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (2)
79-100: Rejection test case could be enhanced with a more explicit expectationThe rejection test correctly verifies the behavior when a release should be rejected because its hash value exceeds the current rollout percentage. However, the calculation of the expected rejection reason is a bit complex inline.
Consider making the expected message calculation more explicit for better readability:
- const expectedRejectionReason = `Release denied due to rate-based rollout restrictions (${Math.round( - (nowSecondsAfterBase / rolloutDurationSeconds) * 100, - )}% complete, eligible in ~5m)`; + const rolloutPercentage = Math.round((nowSecondsAfterBase / rolloutDurationSeconds) * 100); + const expectedRejectionReason = `Release denied due to rate-based rollout restrictions (${rolloutPercentage}% complete, eligible in ~5m)`;
8-101: Consider adding tests for additional edge casesThe current tests cover the main scenarios well, but there are some edge cases that could be tested for completeness.
Consider adding tests for:
- Zero rollout percentage (just started rollout period)
- Multiple releases with different creation dates
- Edge case where hash is 100 (maximum) with rollout percentage less than 100%
Example for testing zero rollout percentage:
it("should reject a release at the start of the rollout period", () => { const rule = new RateRolloutRule({ rolloutDurationSeconds: 600 }); // Set current time equal to the base date (0% rollout) vi.spyOn(rule as any, "getCurrentTime").mockReturnValue(baseDate); // Any hash value > 0 should cause rejection vi.spyOn(rule as any, "getHashValue").mockReturnValue(1); const result = rule.filter( mockDeploymentContext, new Releases([mockRelease]), ); expect(result.allowedReleases.getAll().length).toBe(0); expect(result.rejectionReasons.get(mockRelease.id)).toContain("0% complete"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts(1 hunks)packages/rule-engine/src/rules/rate-rollout-rule.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rule-engine/src/rules/rate-rollout-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/rules/__tests__/rate-rollout-rule.test.ts
🧬 Code Definitions (1)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (3)
packages/rule-engine/src/types.ts (1)
ResolvedRelease(7-18)packages/rule-engine/src/rules/rate-rollout-rule.ts (1)
RateRolloutRule(23-112)packages/rule-engine/src/releases.ts (1)
Releases(18-306)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (4)
packages/rule-engine/src/rules/__tests__/rate-rollout-rule.test.ts (4)
1-7: Imports are well-organizedThe file properly imports utilities from date-fns and vitest for testing, along with the necessary types and classes from the application codebase.
8-45: Test fixtures are well-definedThe test setup includes a clear mock deployment context, a fixed base date for deterministic testing, and proper test lifecycle hooks to reset mocks. The mock release object correctly matches the expected structure defined in the types.
47-61: Complete rollout test case looks goodThis test correctly verifies that a release is allowed when its rollout period is complete (100% rollout). The mocking approach for
getCurrentTimeandgetHashValueensures deterministic results.
63-77: Partial rollout acceptance test is properly implementedThis test correctly verifies that a release is allowed when the hash value (50) is equal to the rollout percentage (50%) at the halfway point of the rollout period.
Summary by CodeRabbit
New Features
Enhancements
rate-rollout-ruleaccessible from the main index.