chore: release target concurrency rule #708
Conversation
WalkthroughAdds target-scoped policy evaluation: new TargetScopedEvaluator interface and ReleaseTargetConcurrencyEvaluator, evaluator factory and manager methods to evaluate policies against a ReleaseTarget, integration into planner logic, and exposure of the targetDecision in the release-targets API response. Changes
Sequence Diagram(s)sequenceDiagram
participant API as Release Targets Endpoint
participant PM as PolicyManager
participant Factory as EvaluatorFactory
participant Eval as TargetScopedEvaluator
participant Store as Store
API->>PM: EvaluateTarget(ctx, releaseTarget, policies)
PM->>Factory: EvaluateTargetScopedPolicyRules(ctx, policy, releaseTarget)
Factory->>Factory: createTargetScopedEvaluator(rule)
Factory->>Eval: Evaluate(ctx, releaseTarget)
Eval->>Store: Query processing jobs for releaseTarget
alt processing jobs found
Eval-->>Factory: RuleEvaluation (Denied) with job details
else no processing jobs
Eval-->>Factory: RuleEvaluation (Allowed)
end
Factory-->>PM: []*RuleEvaluation
PM->>PM: Aggregate into DeployDecision (targetDecision)
PM-->>API: DeployDecision (includes targetDecision)
API-->>Client: return JSON with targetDecision
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
![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 (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency.go (1)
21-30: Code is correct; design suggestions are optional.The method signatures verify correctly. The implementation intentionally enforces a fixed concurrency limit of 1—this evaluator is not designed to be configurable. Unlike evaluators like
GradualRolloutEvaluatororAnyApprovalEvaluatorthat receive their specific rule types,ReleaseTargetConcurrencyEvaluatorhas no rule parameter and no configuration fields. The factory method deliberately ignores thePolicyRuleparameter when creating this evaluator. The name accurately describes what it does; renaming toSingleConcurrencyEvaluatoror adding configurability are optional design improvements, not requirements.
📜 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/releasetargetconcurrency/releasetargetconcurrency.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/factory.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/releasetargetconcurrency/releasetargetconcurrency.goapps/workspace-engine/pkg/workspace/releasemanager/policy/factory.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
Learning: In packages/rule-engine/src/rules/concurrency-rule.ts, the ConcurrencyRule should remain simple without additional validation since database and Zod schemas already handle concurrency validation. The user prefers this rule to be "dumb" and just perform the comparison check rather than duplicating validation logic.
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-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/factory.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/factory.go
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency.go (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
TargetScopedEvaluator(62-67)apps/workspace-engine/pkg/workspace/releasemanager/policy/results/rule.go (2)
NewDeniedResult(24-26)NewAllowedResult(29-33)
apps/workspace-engine/pkg/workspace/releasemanager/policy/factory.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (5)
Policy(312-325)ReleaseTarget(415-419)RuleEvaluation(478-496)PolicyRule(335-342)Id(52-52)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
TargetScopedEvaluator(62-67)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency.go (1)
NewReleaseTargetConcurrencyEvaluator(17-19)
⏰ 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: Typecheck
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: tests
- GitHub Check: workspace-engine-tests
🔇 Additional comments (3)
apps/workspace-engine/pkg/workspace/releasemanager/policy/factory.go (2)
120-142: LGTM! Method follows established patterns.The implementation correctly follows the same structure as other evaluation methods in this file, using the
evaluateRuleshelper and properly handling nil evaluators and error propagation.
215-221: Fix the misleading comment and investigate whether unconditional concurrency checks are intentional.The method does ignore the
ruleparameter and unconditionally returns aReleaseTargetConcurrencyEvaluatorfor all rules. However, the suggested fix in the original review is incorrect:PolicyRulehas no concurrency-related field (verified: onlyAnyApproval,EnvironmentProgression,GradualRollout,Id,PolicyId,CreatedAt).The concurrency evaluator checks global job state (whether processing jobs exist for a target) rather than rule-specific configuration, suggesting this may be intentional design—a global constraint applied uniformly. Unlike other evaluators that check rule-specific fields (
rule.GradualRollout,rule.AnyApproval), the concurrency evaluator has no config to gate on.Issues to address:
- The comment "Returns nil for unknown rule types" is misleading—it never returns nil or empty.
- The unused
ruleparameter suggests this was scaffolded from the pattern but may not need it, or this needs clarification if the behavior is intentional.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency.go (1)
11-19: LGTM! Clean implementation following Go best practices.The compile-time interface assertion and constructor pattern are appropriate and well-structured.
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.go (4)
47-51: Consider consistent error handling in the setup helper.Error checks are present for
Resources.Upsert(line 30) andReleases.Upsert(line 43), but missing forJobs.Upsert(line 50). While test failures would still surface later, adding the check would make debugging easier if job creation fails.Apply this diff for consistency:
// Add jobs for _, job := range jobs { job.ReleaseId = release.ID() - st.Jobs.Upsert(ctx, job) + if err := st.Jobs.Upsert(ctx, job); err != nil { + t.Fatalf("Failed to upsert job: %v", err) + } }
168-171: Inconsistent type assertion handling.The type assertion on line 168 doesn't check the
okvalue, while other tests do (e.g., line 124, line 602). Consider either checkingokconsistently or using_to explicitly ignore it throughout the test suite.Apply this diff for consistency:
- jobsMap, _ := result.Details["jobs"].(map[string]*oapi.Job) + jobsMap, ok := result.Details["jobs"].(map[string]*oapi.Job) + if !ok { + t.Fatal("expected jobs to be map[string]*oapi.Job") + } if len(jobsMap) != 1 { t.Errorf("expected 1 job in details, got %d", len(jobsMap)) }
205-208: Inconsistent variable naming and type assertion handling.The variable name
jobsMap2is inconsistent with other tests that usejobsMaporjobsInDetails. Additionally, the type assertion doesn't check theokvalue.Apply this diff for consistency:
- jobsMap2, _ := result.Details["jobs"].(map[string]*oapi.Job) - if len(jobsMap2) != 1 { - t.Errorf("expected 1 job in details, got %d", len(jobsMap2)) + jobsMap, ok := result.Details["jobs"].(map[string]*oapi.Job) + if !ok { + t.Fatal("expected jobs to be map[string]*oapi.Job") + } + if len(jobsMap) != 1 { + t.Errorf("expected 1 job in details, got %d", len(jobsMap)) }
426-426: Missing error check for consistency.The call to
st.Resources.Upsertdoesn't check for errors, while the setup helper function does (line 30-31). Consider adding an error check for consistency.Apply this diff:
- st.Resources.Upsert(context.Background(), resource2) + if _, err := st.Resources.Upsert(context.Background(), resource2); err != nil { + t.Fatalf("Failed to upsert resource2: %v", err) + }
📜 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 (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_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/releasetargetconcurrency/releasetargetconcurrency_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/releasetargetconcurrency/releasetargetconcurrency_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
Learning: In packages/rule-engine/src/rules/concurrency-rule.ts, the ConcurrencyRule should remain simple without additional validation since database and Zod schemas already handle concurrency validation. The user prefers this rule to be "dumb" and just perform the comparison check rather than duplicating validation logic.
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-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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_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/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_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/pkg/model/selector/**/*_test.go : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_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/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/releasetargetconcurrency/releasetargetconcurrency_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/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.go (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (16)
Id(52-52)Metadata(55-55)CreatedAt(50-50)Release(406-412)ReleaseId(56-56)Status(58-58)InProgress(39-39)Pending(42-42)ActionRequired(35-35)Successful(44-44)CompletedAt(49-49)Failure(38-38)Skipped(43-43)InvalidJobAgent(41-41)InvalidIntegration(40-40)ExternalRunNotFound(37-37)apps/workspace-engine/pkg/workspace/store/releases.go (1)
Releases(17-20)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency.go (1)
NewReleaseTargetConcurrencyEvaluator(17-19)
⏰ 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: Lint
- GitHub Check: Format
- GitHub Check: Typecheck
- GitHub Check: workspace-engine-tests
- GitHub Check: tests
🔇 Additional comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.go (1)
56-610: Excellent test coverage and structure.The test suite provides comprehensive coverage of the ReleaseTargetConcurrencyEvaluator:
- Edge cases: no jobs, nil release target, different release targets
- All processing states: Pending, InProgress, ActionRequired
- All terminal states: Successful, Failure, Cancelled, Skipped, and error states
- Mixed scenarios: completed jobs with processing jobs
- Result structure validation
The use of table-driven tests for state enumeration (lines 443-488, 490-536) combined with individual tests for complex scenarios follows Go testing best practices.
Summary by CodeRabbit
New Features
Tests