chore: no targets passes progression rule#722
Conversation
WalkthroughEvaluator now returns allowed when a dependency environment has zero ReleaseTarget records (early return). Tests were updated to explicitly upsert ReleaseTarget fixtures before use, and ReleaseTargetJobTracker avoids an initial compute when no targets exist. Changes
Sequence Diagram(s)sequenceDiagram
participant Eval as ProgressionEvaluator
participant JT as ReleaseTargetJobTracker
rect rgb(200,220,255)
note right of Eval: Previous flow (deny on no targets)
Eval->>JT: queryReleaseTargets(env)
JT-->>Eval: count = 0
Eval->>Eval: deny progression (old)
end
rect rgb(220,255,220)
note right of Eval: New flow (allow on no targets)
Eval->>JT: queryReleaseTargets(env)
JT-->>Eval: count = 0
Eval->>Eval: return allowed (early exit)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)apps/workspace-engine/**/*.go📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Files:
apps/workspace-engine/**/*_test.go📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
📚 Learning: 2025-08-12T18:13:54.630ZApplied to files:
🧬 Code graph analysis (1)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.go (1)
⏰ 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)
🔇 Additional comments (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.go (1)
208-219: Inconsistent test setup: missing ReleaseTarget upsert.
TestSoakTimeEvaluator_NoSuccessfulJobscreatesrt1at line 208 but doesn't upsert it to the store before creatingrelease1at line 213. This is inconsistent with all other tests in this file and across the PR, which explicitly upsert ReleaseTarget entries before creating releases.Apply this diff to add the missing upsert:
rt1 := &oapi.ReleaseTarget{ ResourceId: "resource-1", EnvironmentId: "env-staging", DeploymentId: "deploy-1", } + st.ReleaseTargets.Upsert(ctx, rt1) release1 := &oapi.Release{ ReleaseTarget: *rt1, Version: *version,
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go (1)
286-288: Consider removing unreachable code.Lines 286-288 check for zero release targets and return a denial, but this code is now unreachable due to the early return at lines 276-278. While not harmful, removing it would improve code clarity.
Apply this diff to remove the dead code:
span.SetAttributes(attribute.Int("job_count", len(tracker.Jobs()))) span.SetAttributes(attribute.Int("release_target_count", len(tracker.ReleaseTargets))) if len(tracker.Jobs()) == 0 { return results.NewDeniedResult("No jobs found") } - - if len(tracker.ReleaseTargets) == 0 { - return results.NewDeniedResult("No release targets found") - } passRateResult := passRateEvaluator.Evaluate(ctx, scope)
📜 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 (5)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go(3 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.go(13 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.go(8 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/environmentprogression/jobtracker.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_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/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.go
🧠 Learnings (6)
📓 Common learnings
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 : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_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 : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_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/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_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/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_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/environmentprogression/soaktime_test.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.go (1)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go (3)
apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
ReleaseTargets(16-17)apps/workspace-engine/pkg/workspace/releasemanager/policy/results/rule.go (1)
NewAllowedResult(29-33)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go (4)
apps/workspace-engine/pkg/oapi/oapi.gen.go (5)
ReleaseTarget(569-573)DeploymentVersion(219-230)Id(75-75)EnvironmentProgressionRule(263-273)Environment(253-260)apps/workspace-engine/pkg/workspace/store/release_targets.go (1)
ReleaseTargets(23-27)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go (1)
NewEvaluator(28-39)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/evaulator.go (1)
EvaluatorScope(25-29)
⏰ 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: Lint
- GitHub Check: Typecheck
- GitHub Check: Format
🔇 Additional comments (7)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.go (1)
89-91: LGTM! Conditional initialization prevents unnecessary computation.Skipping the initial statistics computation when no release targets exist is a sensible optimization and aligns with the broader change to allow progression when dependency environments have no targets.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go (1)
276-278: LGTM! Core behavioral change allows progression when no targets exist.This change implements the intended behavior: when a dependency environment has no release targets, progression is allowed by default. This makes sense for scenarios where dependency environments are not yet populated or don't apply to the current deployment.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.go (1)
156-158: LGTM! Proper test setup with explicit ReleaseTarget upserts.The tests now consistently upsert ReleaseTarget entries before creating releases that reference them. This ensures targets exist in the store prior to lookups and associations, preventing potential test failures and making the test setup more explicit and reliable.
Also applies to: 238-239, 327-327, 381-382, 455-455, 514-514, 575-575, 629-630, 706-707, 781-781, 863-865, 1085-1086, 1169-1171
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.go (1)
84-84: LGTM! Consistent test setup with explicit ReleaseTarget upserts.The tests now properly upsert ReleaseTarget entries before creating releases, ensuring targets exist in the store for downstream lookups. This follows the pattern established across the PR and makes test setup more reliable.
Also applies to: 149-149, 271-271, 335-335, 432-432, 497-497, 561-561, 628-628
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go (3)
87-104: LGTM! Test fixture now creates ReleaseTarget entries.The setup function now explicitly creates and upserts ReleaseTarget entries for dev, staging, and prod environments. This ensures targets exist in the store before tests run, providing a more complete and realistic test fixture that aligns with the PR's focus on explicit ReleaseTarget management.
182-182: LGTM! Explicit ReleaseTarget upserts ensure target availability.These tests now explicitly upsert the staging ReleaseTarget before creating releases that reference it. While the setupTestStore already creates release targets, explicitly upserting ensures the target exists at the point of use, making the test setup more clear and defensive.
Also applies to: 944-944
996-1039: LGTM! New test validates no-targets-allowed behavior.This test properly verifies the core PR change: when a dependency environment has no release targets, progression is allowed. The test removes the staging release target and confirms that the evaluator returns an allowed result, validating the new default-to-allowed behavior.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.