From c4b65dcf78455176577f4946ecc12bd47d5a4b82 Mon Sep 17 00:00:00 2001 From: Shinnosuke Sawada-Dazai Date: Thu, 20 Feb 2025 09:57:45 +0900 Subject: [PATCH 1/4] Remove sdk.StageStatusCancelled because the piped handles the case as cancelled by the user without using the plugin's result. Signed-off-by: Shinnosuke Sawada-Dazai --- pkg/app/pipedv1/plugin/wait/wait.go | 5 ++++- pkg/app/pipedv1/plugin/wait/wait_test.go | 2 +- pkg/plugin/sdk/deployment.go | 7 ++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/app/pipedv1/plugin/wait/wait.go b/pkg/app/pipedv1/plugin/wait/wait.go index 0e71d5c2ac..022a3037bb 100644 --- a/pkg/app/pipedv1/plugin/wait/wait.go +++ b/pkg/app/pipedv1/plugin/wait/wait.go @@ -78,7 +78,10 @@ func wait(ctx context.Context, duration time.Duration, initialStart time.Time, s case <-ctx.Done(): // on cancelled slp.Info("Wait cancelled") - return sdk.StageStatusCancelled + // The piped handles this case as cancelled by the user without using the plugin's result. + // So we don't need to consider which status should be returned. + // We return the failure here. + return sdk.StageStatusFailure } } } diff --git a/pkg/app/pipedv1/plugin/wait/wait_test.go b/pkg/app/pipedv1/plugin/wait/wait_test.go index 60a7e3e86e..f2e6e99af1 100644 --- a/pkg/app/pipedv1/plugin/wait/wait_test.go +++ b/pkg/app/pipedv1/plugin/wait/wait_test.go @@ -69,7 +69,7 @@ func TestWait_Cancel(t *testing.T) { select { case result := <-resultCh: - assert.Equal(t, sdk.StageStatusCancelled, result) + assert.Equal(t, sdk.StageStatusFailure, result) case <-time.After(1 * time.Second): t.Error("wait() did not ended even after the context was canceled") } diff --git a/pkg/plugin/sdk/deployment.go b/pkg/plugin/sdk/deployment.go index 2831f14efd..09f4b08b89 100644 --- a/pkg/plugin/sdk/deployment.go +++ b/pkg/plugin/sdk/deployment.go @@ -455,9 +455,8 @@ type ExecuteStageResponse struct { type StageStatus int const ( - StageStatusSuccess StageStatus = 2 - StageStatusFailure StageStatus = 3 - StageStatusCancelled StageStatus = 4 + StageStatusSuccess StageStatus = 2 + StageStatusFailure StageStatus = 3 // StageStatusSkipped StageStatus = 5 // TODO: If SDK can handle whole skipping, this is unnecessary. @@ -472,8 +471,6 @@ func (o StageStatus) toModelEnum() model.StageStatus { return model.StageStatus_STAGE_SUCCESS case StageStatusFailure: return model.StageStatus_STAGE_FAILURE - case StageStatusCancelled: - return model.StageStatus_STAGE_CANCELLED case StageStatusExited: return model.StageStatus_STAGE_EXITED default: From 9a27453aac6e1cddaca40bf13bfa3a1699aaf9e3 Mon Sep 17 00:00:00 2001 From: Shinnosuke Sawada-Dazai Date: Fri, 21 Feb 2025 13:33:48 +0900 Subject: [PATCH 2/4] Handle context cancellation in stage execution without returning an error Signed-off-by: Shinnosuke Sawada-Dazai --- pkg/app/pipedv1/controller/scheduler.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/app/pipedv1/controller/scheduler.go b/pkg/app/pipedv1/controller/scheduler.go index 5d73c5676b..2d732fa7c4 100644 --- a/pkg/app/pipedv1/controller/scheduler.go +++ b/pkg/app/pipedv1/controller/scheduler.go @@ -553,7 +553,10 @@ func (s *scheduler) executeStage(sig StopSignal, ps *model.PipelineStage) (final TargetDeploymentSource: tds.ToPluginDeploySource(), }, }) - if err != nil { + // do not return error if the context is already canceled. + // this occurs when the stage is canceled. + // otherwise, return the error. + if err != nil && ctx.Err() == nil { s.logger.Error("failed to execute stage", zap.String("stage-name", ps.Name), zap.Error(err)) s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires) return model.StageStatus_STAGE_FAILURE From a755ab8a3859edf1cac522337c67648ca2707ced Mon Sep 17 00:00:00 2001 From: Shinnosuke Sawada-Dazai Date: Tue, 25 Feb 2025 09:12:01 +0900 Subject: [PATCH 3/4] Refactor StageStatus constants for clarity and maintainability Signed-off-by: Shinnosuke Sawada-Dazai --- pkg/plugin/sdk/deployment.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/plugin/sdk/deployment.go b/pkg/plugin/sdk/deployment.go index 09f4b08b89..394635a55d 100644 --- a/pkg/plugin/sdk/deployment.go +++ b/pkg/plugin/sdk/deployment.go @@ -455,16 +455,19 @@ type ExecuteStageResponse struct { type StageStatus int const ( - StageStatusSuccess StageStatus = 2 - StageStatusFailure StageStatus = 3 - - // StageStatusSkipped StageStatus = 5 // TODO: If SDK can handle whole skipping, this is unnecessary. - + _ StageStatus = iota + // StageStatusSuccess indicates that the stage succeeded. + StageStatusSuccess + // StageStatusFailure indicates that the stage failed. + StageStatusFailure // StageStatusExited can be used when the stage succeeded and exit the pipeline without executing the following stages. - StageStatusExited StageStatus = 6 + StageStatusExited + + // StageStatusSkipped // TODO: If SDK can handle whole skipping, this is unnecessary. ) // toModelEnum converts the StageStatus to the model.StageStatus. +// It returns model.StageStatus_STAGE_FAILURE if the given value is invalid. func (o StageStatus) toModelEnum() model.StageStatus { switch o { case StageStatusSuccess: From 4d96f03344e57e1da198a2e87d59aebbc10abace Mon Sep 17 00:00:00 2001 From: Shinnosuke Sawada-Dazai Date: Tue, 25 Feb 2025 17:47:42 +0900 Subject: [PATCH 4/4] Update comments on cancelled case Co-authored-by: Tetsuya KIKUCHI <97105818+t-kikuc@users.noreply.github.com> Signed-off-by: Shinnosuke Sawada-Dazai --- pkg/app/pipedv1/plugin/wait/wait.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/app/pipedv1/plugin/wait/wait.go b/pkg/app/pipedv1/plugin/wait/wait.go index 022a3037bb..3733c70f6c 100644 --- a/pkg/app/pipedv1/plugin/wait/wait.go +++ b/pkg/app/pipedv1/plugin/wait/wait.go @@ -78,9 +78,8 @@ func wait(ctx context.Context, duration time.Duration, initialStart time.Time, s case <-ctx.Done(): // on cancelled slp.Info("Wait cancelled") - // The piped handles this case as cancelled by the user without using the plugin's result. - // So we don't need to consider which status should be returned. - // We return the failure here. + // We can return any status here because the piped handles this case as cancelled by a user, + // ignoring the result from a plugin. return sdk.StageStatusFailure } }