chore: only taint target on successful job #692
Conversation
WalkthroughThe skipdeployed evaluator's job status filtering has been removed. Previously, the evaluator only considered jobs in Pending, InProgress, or Successful states when determining deployment eligibility. The evaluator now considers all job statuses, including Failed, Cancelled, and Skipped, when checking if a release can be redeployed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The change is straightforward: removal of a filtering condition and corresponding test updates. The scope is limited to two related files with consistent, repetitive patterns across tests reflecting the new behavior. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/targets/taint.go(1 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/targets/taint.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go (2)
apps/workspace-engine/pkg/oapi/type_conversion.go (1)
ConvertToOapiJob(43-49)apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Status(57-57)Successful(43-43)
| oapiJob, ok := oapi.ConvertToOapiJob(entity) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| if oapiJob.Status != oapi.Successful { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant type conversion; status check logic is correct.
The ConvertToOapiJob call on line 93 is unnecessary because the case *oapi.Job: statement on line 92 already guarantees that entity is of type *oapi.Job. The conversion will always succeed, making the check on lines 94-96 dead code that can never execute.
The status check on lines 98-100 correctly implements the PR objective to only taint targets for successful jobs.
Apply this diff to remove the redundant conversion:
case *oapi.Job:
- oapiJob, ok := oapi.ConvertToOapiJob(entity)
- if !ok {
- continue
- }
-
- if oapiJob.Status != oapi.Successful {
+ if entity.Status != oapi.Successful {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oapiJob, ok := oapi.ConvertToOapiJob(entity) | |
| if !ok { | |
| continue | |
| } | |
| if oapiJob.Status != oapi.Successful { | |
| continue | |
| } | |
| if entity.Status != oapi.Successful { | |
| continue | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go around
lines 93 to 100, remove the redundant call to oapi.ConvertToOapiJob and its
associated ok check (the lines that perform the conversion and continue on !ok)
because the preceding case *oapi.Job: already guarantees the type; instead, use
the entity value asserted as the job (or the existing variable from the case)
and keep the existing status check for oapi.Successful unchanged.
📊 Code Coverage Reportworkspace-engine coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeployed_test.go (5)
104-119: Policy check: should failures block same‑release retry? Also avoid impl-coupled comment.This test now denies retry after a failed job and mentions “validJobStatuses filter removed.” That couples the test to an internal detail and, more importantly, changes behavior that could prevent remediation. Please confirm the product policy: do failed jobs block re‑deploy of the same release, or should only pending/inProgress/successful block? The PR title (“only taint target on successful job”) suggests the latter.
If the intent is “only successful blocks; failures/cancellations allow retry,” update the assertions accordingly and drop the impl note.
Example adjustment if retries after failure should be allowed:
- // Assert: Should DENY retry because failed jobs are now considered (validJobStatuses filter removed) + // Assert: Should ALLOW retry; previous failure should not block re-deploy of the same release - if result.Allowed { - t.Errorf("expected denied for retry after failure (failed jobs are now tracked), got allowed: %s", result.Message) - } + if !result.Allowed { + t.Errorf("expected allowed for retry after failure, got denied: %s", result.Message) + }As per coding guidelines.
334-349: Same concern for cancellations: confirm whether cancelled should block retry; remove impl detail in comment.This test now denies retry after a cancelled job and references “validJobStatuses filter removed.” Please verify policy. Typically, cancellation shouldn’t permanently block redeploy of the same release.
If the policy is to permit retry after cancellation, flip the assertion and remove the impl-coupled comment.
Example if cancellation should allow retry:
- // Assert: Should DENY retry because cancelled jobs are now considered (validJobStatuses filter removed) + // Assert: Should ALLOW retry; cancellation should not block re-deploy of the same release - if result.Allowed { - t.Errorf("expected denied for retry after cancellation (cancelled jobs are now tracked), got allowed: %s", result.Message) - } + if !result.Allowed { + t.Errorf("expected allowed for retry after cancellation, got denied: %s", result.Message) + }As per coding guidelines.
675-699: Stabilize time ordering with a fixed base time.Multiple time.Now() calls can introduce flakiness if execution spans a clock tick. Use a single base time and derive relative times for CreatedAt/CompletedAt.
Example:
- completedAt := time.Now() + base := time.Date(2025, 1, 1, 10, 0, 0, 0, time.UTC) + completedAt := base st.Jobs.Upsert(ctx, &oapi.Job{ Id: "job-failed", ReleaseId: oldRelease.ID(), Status: oapi.Failure, - CreatedAt: time.Now().Add(-3 * time.Hour), + CreatedAt: base.Add(-3 * time.Hour), CompletedAt: &completedAt, }) ... st.Jobs.Upsert(ctx, &oapi.Job{ Id: "job-skipped", ReleaseId: oldRelease.ID(), Status: oapi.Skipped, - CreatedAt: time.Now().Add(-1 * time.Hour), // Most recent + CreatedAt: base.Add(-1 * time.Hour), // Most recent CompletedAt: &completedAt, }) ``` <!-- review_comment_end --> --- `715-719`: **Strengthen assertion: also assert most recent job status.** We assert previous_release_id but not the status of that most recent job. Add a check to ensure we actually selected the skipped job. ```diff if result.Details["previous_release_id"] != oldRelease.ID() { t.Errorf("expected previous_release_id=%s (from most recent job), got %v", oldRelease.ID(), result.Details["previous_release_id"]) } +if result.Details["job_status"] != string(oapi.Skipped) { + t.Errorf("expected job_status=%s, got %v", oapi.Skipped, result.Details["job_status"]) +} ``` <!-- review_comment_end --> --- `721-791`: **Broad “all statuses block” may hinder remediation; consider scoping blockers.** This parametrized test asserts that ANY status prevents re‑deploying the same release. That would block recovery from failure/cancelled/skipped/misconfig states. Please confirm intended contract. A common policy is: - Blockers: pending, inProgress (to prevent duplicates), successful (to avoid redeploying identical release). - Non‑blockers (allow retry): failure, cancelled, skipped, invalidIntegration, invalidJobAgent, actionRequired, externalRunNotFound. If that policy is desired, split the slice into two sets and invert the expectations for the non‑blockers. Illustrative refactor: ```diff - statuses := []oapi.JobStatus{ - oapi.Pending, oapi.InProgress, oapi.Successful, oapi.Failure, oapi.Cancelled, oapi.Skipped, - oapi.ActionRequired, oapi.ExternalRunNotFound, oapi.InvalidIntegration, oapi.InvalidJobAgent, - } + blockers := []oapi.JobStatus{oapi.Pending, oapi.InProgress, oapi.Successful} + nonBlockers := []oapi.JobStatus{ + oapi.Failure, oapi.Cancelled, oapi.Skipped, oapi.ActionRequired, + oapi.ExternalRunNotFound, oapi.InvalidIntegration, oapi.InvalidJobAgent, + } - for _, status := range statuses { + for _, status := range blockers { t.Run(string(status), func(t *testing.T) { ... - // Assert: Should DENY regardless of job status + // Assert: Should DENY for blocker statuses if err != nil { ... } if result.Allowed { ... } ... }) } + + for _, status := range nonBlockers { + t.Run("nonBlocker_"+string(status), func(t *testing.T) { + ... + // Assert: Should ALLOW for non‑blocker statuses + if err != nil { ... } + if !result.Allowed { + t.Errorf("expected allowed for retry with status %s, got denied: %s", status, result.Message) + } + }) + }As per coding guidelines.
📜 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/skipdeployed/skipdeployed_test.go(7 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go(0 hunks)
💤 Files with no reviewable changes (1)
- apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go
🧰 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/skipdeployed/skipdeployed_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/skipdeployed/skipdeployed_test.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeployed_test.go (4)
apps/workspace-engine/pkg/oapi/oapi.gen.go (20)
Failure(37-37)Cancelled(35-35)CreatedAt(49-49)JobStatus(237-237)Pending(41-41)InProgress(38-38)Successful(43-43)Skipped(42-42)ActionRequired(34-34)ExternalRunNotFound(36-36)InvalidIntegration(39-39)InvalidJobAgent(40-40)ReleaseTarget(388-392)Release(379-385)DeploymentVersion(152-162)Id(51-51)Job(213-225)ReleaseId(55-55)Status(57-57)CompletedAt(48-48)apps/workspace-engine/pkg/workspace/store/releases.go (1)
Releases(16-18)apps/workspace-engine/pkg/workspace/store/jobs.go (1)
Jobs(16-18)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go (1)
NewSkipDeployedEvaluator(21-25)
⏰ 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). (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
Summary by CodeRabbit