From 0dff19018492162e2451ec94f8af2451f9d0135a Mon Sep 17 00:00:00 2001 From: Aditya Choudhari Date: Mon, 20 Oct 2025 20:21:40 -0700 Subject: [PATCH 1/2] chore: only taint target on successful job --- .../pkg/workspace/releasemanager/targets/taint.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go b/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go index 7818eacf9..b41a9886d 100644 --- a/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go +++ b/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go @@ -90,6 +90,15 @@ func (tp *TaintProcessor) processChanges(changeSet *changeset.ChangeSet[any]) { tp.taintByResourceId(entity.Id) case *oapi.Job: + oapiJob, ok := oapi.ConvertToOapiJob(entity) + if !ok { + continue + } + + if oapiJob.Status != oapi.Successful { + continue + } + rel, ok := tp.store.Releases.Get(entity.ReleaseId) if !ok { continue From 06ed6bedf233530bfdcf14d7d0504361a93fa5b9 Mon Sep 17 00:00:00 2001 From: Aditya Choudhari Date: Mon, 20 Oct 2025 20:40:29 -0700 Subject: [PATCH 2/2] fix --- .../skipdeployed/skipdeployed_test.go | 121 +++++++++++++++--- .../evaluator/skipdeployed/skipdeplyed.go | 11 -- .../workspace/releasemanager/targets/taint.go | 9 -- 3 files changed, 102 insertions(+), 39 deletions(-) diff --git a/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeployed_test.go b/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeployed_test.go index 2f3e68ac4..29ab55fee 100644 --- a/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeployed_test.go +++ b/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeployed_test.go @@ -101,17 +101,21 @@ func TestSkipDeployedEvaluator_PreviousDeploymentFailed(t *testing.T) { // Act: Try to deploy same release again result, err := evaluator.Evaluate(ctx, previousRelease) - // Assert: Should ALLOW retry because failed jobs are ignored by validJobStatuses filter + // Assert: Should DENY retry because failed jobs are now considered (validJobStatuses filter removed) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !result.Allowed { - t.Errorf("expected allowed for retry after failure (failed jobs are ignored), got denied: %s", result.Message) + if result.Allowed { + t.Errorf("expected denied for retry after failure (failed jobs are now tracked), got allowed: %s", result.Message) } - if result.Message != "No previous deployment found" { - t.Errorf("expected 'No previous deployment found' since failed jobs are filtered out, got '%s'", result.Message) + if result.Details["existing_job_id"] != "job-1" { + t.Errorf("expected existing_job_id=job-1, got %v", result.Details["existing_job_id"]) + } + + if result.Details["job_status"] != string(oapi.Failure) { + t.Errorf("expected job_status=failure, got %v", result.Details["job_status"]) } } @@ -290,7 +294,7 @@ func TestSkipDeployedEvaluator_JobInProgressNotSuccessful(t *testing.T) { } } -func TestSkipDeployedEvaluator_CancelledJobAllowsRedeploy(t *testing.T) { +func TestSkipDeployedEvaluator_CancelledJobPreventsRedeploy(t *testing.T) { // Setup: Previous job was cancelled st := setupStoreWithResource(t, "resource-1") ctx := context.Background() @@ -327,17 +331,21 @@ func TestSkipDeployedEvaluator_CancelledJobAllowsRedeploy(t *testing.T) { // Act: Try to deploy same release again result, err := evaluator.Evaluate(ctx, release) - // Assert: Should ALLOW retry because cancelled jobs are ignored by validJobStatuses filter + // Assert: Should DENY retry because cancelled jobs are now considered (validJobStatuses filter removed) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !result.Allowed { - t.Errorf("expected allowed for retry after cancellation (cancelled jobs are ignored), got denied: %s", result.Message) + if result.Allowed { + t.Errorf("expected denied for retry after cancellation (cancelled jobs are now tracked), got allowed: %s", result.Message) } - if result.Message != "No previous deployment found" { - t.Errorf("expected 'No previous deployment found' since cancelled jobs are filtered out, got '%s'", result.Message) + if result.Details["existing_job_id"] != "job-1" { + t.Errorf("expected existing_job_id=job-1, got %v", result.Details["existing_job_id"]) + } + + if result.Details["job_status"] != string(oapi.Cancelled) { + t.Errorf("expected job_status=cancelled, got %v", result.Details["job_status"]) } } @@ -630,8 +638,8 @@ func TestSkipDeployedEvaluator_PendingJobPreventsRedeploy(t *testing.T) { } } -func TestSkipDeployedEvaluator_IgnoresInvalidJobStatuses(t *testing.T) { - // Test that jobs with invalid statuses (failed, cancelled, etc.) are ignored +func TestSkipDeployedEvaluator_ConsidersAllJobStatuses(t *testing.T) { + // Test that jobs with all statuses (failed, cancelled, skipped, etc.) are now considered st := setupStoreWithResource(t, "resource-1") ctx := context.Background() @@ -664,7 +672,7 @@ func TestSkipDeployedEvaluator_IgnoresInvalidJobStatuses(t *testing.T) { t.Fatalf("Failed to upsert new release: %v", err) } - // Create jobs with various invalid statuses + // Create jobs with various statuses - the most recent is skipped completedAt := time.Now() st.Jobs.Upsert(ctx, &oapi.Job{ Id: "job-failed", @@ -686,7 +694,7 @@ func TestSkipDeployedEvaluator_IgnoresInvalidJobStatuses(t *testing.T) { Id: "job-skipped", ReleaseId: oldRelease.ID(), Status: oapi.Skipped, - CreatedAt: time.Now().Add(-1 * time.Hour), + CreatedAt: time.Now().Add(-1 * time.Hour), // Most recent CompletedAt: &completedAt, }) @@ -695,16 +703,91 @@ func TestSkipDeployedEvaluator_IgnoresInvalidJobStatuses(t *testing.T) { // Act: Try to deploy new release result, err := evaluator.Evaluate(ctx, newRelease) - // Assert: Should ALLOW because all previous jobs have invalid statuses and are ignored + // Assert: Should ALLOW because it's a different release, but should recognize the previous job if err != nil { t.Fatalf("unexpected error: %v", err) } if !result.Allowed { - t.Errorf("expected allowed when only invalid status jobs exist, got denied: %s", result.Message) + t.Errorf("expected allowed for different release, got denied: %s", result.Message) } - if result.Message != "No previous deployment found" { - t.Errorf("expected 'No previous deployment found' since invalid jobs are filtered out, got '%s'", result.Message) + // The most recent job should be recognized (job-skipped with oldRelease) + 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"]) + } +} + +func TestSkipDeployedEvaluator_AllJobStatusesPreventRedeployOfSameRelease(t *testing.T) { + // Test that ANY job status prevents re-deploying the same release + ctx := context.Background() + + statuses := []oapi.JobStatus{ + oapi.Pending, + oapi.InProgress, + oapi.Successful, + oapi.Failure, + oapi.Cancelled, + oapi.Skipped, + oapi.ActionRequired, + oapi.ExternalRunNotFound, + oapi.InvalidIntegration, + oapi.InvalidJobAgent, + } + + for _, status := range statuses { + t.Run(string(status), func(t *testing.T) { + st := setupStoreWithResource(t, "resource-1") + + releaseTarget := &oapi.ReleaseTarget{ + DeploymentId: "deployment-1", + EnvironmentId: "env-1", + ResourceId: "resource-1", + } + + release := &oapi.Release{ + ReleaseTarget: *releaseTarget, + Version: oapi.DeploymentVersion{ + Id: "version-1", + Tag: "v1.0.0", + }, + } + + if err := st.Releases.Upsert(ctx, release); err != nil { + t.Fatalf("Failed to upsert release: %v", err) + } + + // Create job with the given status + completedAt := time.Now() + st.Jobs.Upsert(ctx, &oapi.Job{ + Id: "job-1", + ReleaseId: release.ID(), + Status: status, + CreatedAt: time.Now().Add(-1 * time.Hour), + CompletedAt: &completedAt, + }) + + evaluator := NewSkipDeployedEvaluator(st) + + // Act: Try to deploy the same release again + result, err := evaluator.Evaluate(ctx, release) + + // Assert: Should DENY regardless of job status + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if result.Allowed { + t.Errorf("expected denied for retry with status %s, got allowed: %s", status, result.Message) + } + + if result.Details["existing_job_id"] != "job-1" { + t.Errorf("expected existing_job_id=job-1, got %v", result.Details["existing_job_id"]) + } + + if result.Details["job_status"] != string(status) { + t.Errorf("expected job_status=%s, got %v", status, result.Details["job_status"]) + } + }) } } diff --git a/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go b/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go index 1953a3cea..1adae7899 100644 --- a/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go +++ b/apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/skipdeployed/skipdeplyed.go @@ -3,7 +3,6 @@ package skipdeployed import ( "context" "fmt" - "slices" "time" "workspace-engine/pkg/oapi" "workspace-engine/pkg/workspace/releasemanager/policy/evaluator" @@ -25,12 +24,6 @@ func NewSkipDeployedEvaluator(store *store.Store) *SkipDeployedEvaluator { } } -var validJobStatuses = []oapi.JobStatus{ - oapi.Pending, - oapi.InProgress, - oapi.Successful, -} - // Evaluate checks if the release has already been attempted. // Returns: // - Denied: If the most recent job is for this exact release (regardless of status) @@ -59,10 +52,6 @@ func (e *SkipDeployedEvaluator) Evaluate( continue } - if !slices.Contains(validJobStatuses, job.Status) { - continue - } - // Try to get completion time first jobTime := job.CreatedAt diff --git a/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go b/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go index b41a9886d..7818eacf9 100644 --- a/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go +++ b/apps/workspace-engine/pkg/workspace/releasemanager/targets/taint.go @@ -90,15 +90,6 @@ func (tp *TaintProcessor) processChanges(changeSet *changeset.ChangeSet[any]) { tp.taintByResourceId(entity.Id) case *oapi.Job: - oapiJob, ok := oapi.ConvertToOapiJob(entity) - if !ok { - continue - } - - if oapiJob.Status != oapi.Successful { - continue - } - rel, ok := tp.store.Releases.Get(entity.ReleaseId) if !ok { continue