fix: if candidate version is already rolled out gradual rollout evaluator short circuits#1116
Conversation
…ator short circuits
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a version check capability to the gradual rollout evaluator. When evaluating whether a resource should be updated, the system now checks if the resource is already deployed on the candidate version; if so, it immediately allows the update without consulting the rollout curve timing gates. ChangesVersion Check Short-Circuit for Gradual Rollout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
Fixes an edge case in the workspace-engine gradual rollout policy evaluator where evaluating a candidate version against a release target that is already deployed on that version could return Pending (due to curve position) and trigger an unintended rollback to an older version during desired-release reconciliation.
Changes:
- Add
GetCurrentVersionID(...)to the gradual rollout getters interface and implement it in the Postgres-backed getters. - Short-circuit
GradualRolloutEvaluator.EvaluatetoAllowedwhen the current deployed version equals the candidate version. - Update controller/test mocks and add targeted tests covering the already-on-version behavior and ensuring normal curve gating still applies otherwise.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/test/controllers/harness/mocks.go | Extends desiredrelease test harness getter to support GetCurrentVersionID. |
| apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go | Updates reconcile test mock to satisfy new getter interface method. |
| apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.go | Updates policy-eval test mock to satisfy new getter interface method. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go | Adds “already on candidate version” short-circuit to prevent rollback behavior. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go | Adds tests for the new short-circuit behavior (and non-short-circuit cases). |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/getters.go | Introduces GetCurrentVersionID in interface + Postgres implementation (using existing current-release query). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| row, err := p.queries.GetCurrentReleaseByReleaseTarget( | ||
| ctx, | ||
| db.GetCurrentReleaseByReleaseTargetParams{ | ||
| ResourceID: resourceID, | ||
| EnvironmentID: environmentID, |
| currentVersionID, err := e.getters.GetCurrentVersionID(ctx, releaseTarget) | ||
| if err != nil { | ||
| return results. | ||
| NewDeniedResult(fmt.Sprintf("Failed to get current version: %v", err)). | ||
| WithDetail("error", err.Error()) |
| // Pretend the resource at position 4 (whose curve slot is at t+240s) is | ||
| // already on this version because it advanced during an override. |
| } | ||
| return resourceID, environmentID, deploymentID, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
This is just a nit, but this seems like a data clump, it might read nicer to have an object like, but this is also fine
type ReleaseTargetIDs struct {
EnvironmentID uuid.UUID
ResourceID uuid.UUID
DeploymentID uuid.UUID
}
Summary by CodeRabbit