chore: gradual rollout respects policy skips #726
Conversation
WalkthroughRefactors gradual rollout evaluator's start-time calculation by extracting two new helper methods for approval and environment progression rules that incorporate skip-aware logic. The main function now aggregates policy skips and delegates per-rule start-time computations to these helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Key areas requiring attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
89-163: Consider extracting shared skip-handling logic to reduce duplication.Both
getStartTimeFromApprovalRuleandgetStartTimeFromEnvironmentProgressionRuleshare identical skip-filtering and time-calculation logic (lines 90-112 and 128-150). The only difference is the evaluator instantiation.A possible refactor could extract the common logic into a helper that accepts a function parameter for creating the evaluator:
+func (e *GradualRolloutEvaluator) getStartTimeFromSkipsOrEvaluator( + ctx context.Context, + rule *oapi.PolicyRule, + scope evaluator.EvaluatorScope, + allSkips []*oapi.PolicySkip, + createEvaluator func() evaluator.Evaluator, +) *time.Time { + skips := make([]*oapi.PolicySkip, 0) + for _, skip := range allSkips { + if skip.RuleId == rule.Id { + skips = append(skips, skip) + } + } + + if len(skips) > 0 { + sort.Slice(skips, func(i, j int) bool { + return skips[i].CreatedAt.After(skips[j].CreatedAt) + }) + latestSkipCreatedAt := skips[0].CreatedAt + if latestSkipCreatedAt.After(scope.Version.CreatedAt) { + return &latestSkipCreatedAt + } + return &scope.Version.CreatedAt + } + + eval := createEvaluator() + if eval == nil { + return nil + } + result := eval.Evaluate(ctx, scope) + if !result.Allowed || result.SatisfiedAt == nil { + return nil + } + return result.SatisfiedAt +}This would simplify both helpers to one-liners calling the shared function with different evaluator factories.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go (1)
616-843: Consider adding a test for skip created after version.The current tests cover the case where skip and version are created at the same time. Consider adding a test for the scenario where a skip is created after the version already exists - in this case, the rollout should start at the skip's
CreatedAttime, not the version's.Example test scenario:
- Version created at
baseTime- Skip created at
baseTime + 1 hour- Expected
rollout_start_timeshould bebaseTime + 1 hourThis would verify the
latestSkipCreatedAt.After(scope.Version.CreatedAt)branch in the implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go(4 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 658
File: apps/workspace-engine/pkg/engine/policy/rules/environment-version-rollout/rollout_start_time.go:65-80
Timestamp: 2025-08-19T22:58:15.170Z
Learning: In the environment-version-rollout rule's rollout start time logic, the code intentionally returns the latest ApprovedAt time (first non-nil record from records ordered by updatedAt) rather than the earliest. This is because the goal is to find the latest approval that caused all the rules to pass. While the complete logic for determining "the latest that caused all rules to pass" would be too complex, this approximation works well given that the UI prevents approving if all rules are already passing.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
📚 Learning: 2025-08-19T22:58:15.170Z
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 658
File: apps/workspace-engine/pkg/engine/policy/rules/environment-version-rollout/rollout_start_time.go:65-80
Timestamp: 2025-08-19T22:58:15.170Z
Learning: In the environment-version-rollout rule's rollout start time logic, the code intentionally returns the latest ApprovedAt time (first non-nil record from records ordered by updatedAt) rather than the earliest. This is because the goal is to find the latest approval that caused all the rules to pass. While the complete logic for determining "the latest that caused all rules to pass" would be too complex, this approximation works well given that the UI prevents approving if all rules are already passing.
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*.go : Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (7)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
PolicyRule(471-481)PolicySkip(484-514)Id(80-80)CreatedAt(78-78)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/evaulator.go (1)
EvaluatorScope(25-29)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go (1)
NewEvaluator(29-39)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.go (1)
NewEvaluator(24-34)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/approval.go (1)
NewEvaluator(21-30)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go (1)
NewEvaluator(29-41)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.go (1)
NewEvaluator(32-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (2)
7-7: LGTM!The
sortimport is correctly added and used for sorting policy skips byCreatedAt.
183-209: LGTM!The integration of policy skips into
getRolloutStartTimeis well-structured:
- Fetches all relevant skips once and passes them to helper methods
- Correctly delegates per-rule start-time calculation to the new helpers
- Preserves the existing logic for aggregating the latest satisfied time across rules
The skip-aware behavior aligns with the PR objective of respecting policy skips in gradual rollout.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go (2)
616-724: LGTM!The test correctly verifies that when an approval policy is skipped:
- The rollout starts at
baseTime(version creation time)- Subsequent positions follow the expected linear rollout schedule (60s intervals)
The test follows the existing file structure and naming conventions.
726-843: LGTM!The test correctly verifies that when an environment progression policy is skipped:
- The rollout starts immediately at
baseTime- The rollout schedule follows the expected linear intervals
Note: The test doesn't need to create the staging environment because the skip bypasses rule evaluation entirely, which is the intended behavior.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.