From be2c1536c238e1504dba233f3b5c3f829c98468b Mon Sep 17 00:00:00 2001 From: knanao Date: Mon, 11 Apr 2022 10:26:10 +0900 Subject: [PATCH 1/4] Fix the deployment model method --- pkg/app/server/grpcapi/web_api.go | 4 +-- pkg/model/deployment.go | 10 ++++---- pkg/model/deployment_test.go | 41 +++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/pkg/app/server/grpcapi/web_api.go b/pkg/app/server/grpcapi/web_api.go index e3579fcb01..154b3c3481 100644 --- a/pkg/app/server/grpcapi/web_api.go +++ b/pkg/app/server/grpcapi/web_api.go @@ -1025,11 +1025,11 @@ func (a *WebAPI) ApproveStage(ctx context.Context, req *webservice.ApproveStageR if err := a.validateDeploymentBelongsToProject(ctx, req.DeploymentId, claims.Role.ProjectId); err != nil { return nil, err } - stage, ok := deployment.StageStatusMap()[req.StageId] + stage, ok := deployment.StageMap()[req.StageId] if !ok { return nil, status.Error(codes.FailedPrecondition, "The stage was not found in the deployment") } - if model.IsCompletedStage(stage) { + if model.IsCompletedStage(stage.Status) { return nil, status.Errorf(codes.FailedPrecondition, "Could not approve the stage because it was already completed") } diff --git a/pkg/model/deployment.go b/pkg/model/deployment.go index 3761bdfa59..4eff4bf708 100644 --- a/pkg/model/deployment.go +++ b/pkg/model/deployment.go @@ -113,13 +113,13 @@ func CanUpdateStageStatus(cur, next StageStatus) bool { return false } -// StageStatusMap returns the map from id to status of all stages. -func (d *Deployment) StageStatusMap() map[string]StageStatus { - statuses := make(map[string]StageStatus, len(d.Stages)) +// StageMap returns the map of id and the stage. +func (d *Deployment) StageMap() map[string]*PipelineStage { + stage := make(map[string]*PipelineStage, len(d.Stages)) for _, s := range d.Stages { - statuses[s.Id] = s.Status + stage[s.Id] = s } - return statuses + return stage } // CommitHash returns the hash value of trigger commit. diff --git a/pkg/model/deployment_test.go b/pkg/model/deployment_test.go index 223625db77..3d707de581 100644 --- a/pkg/model/deployment_test.go +++ b/pkg/model/deployment_test.go @@ -69,3 +69,44 @@ func TestDeployment_ContainTags(t *testing.T) { }) } } + +func TestDeployment_StageMap(t *testing.T) { + testcases := []struct { + name string + deployment *Deployment + want map[string]*PipelineStage + }{ + { + name: "ok", + deployment: &Deployment{ + Stages: []*PipelineStage{ + { + Id: "stage1", + }, + { + Id: "stage2", + }, + }, + }, + want: map[string]*PipelineStage{ + "stage1": &PipelineStage{ + Id: "stage1", + }, + "stage2": &PipelineStage{ + Id: "stage2", + }, + }, + }, + { + name: "no stages", + deployment: &Deployment{}, + want: map[string]*PipelineStage{}, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + got := tc.deployment.StageMap() + assert.Equal(t, tc.want, got) + }) + } +} From 50dab9323395ef1a33b0c4cf5ca4d38df077eb72 Mon Sep 17 00:00:00 2001 From: pipecd-bot Date: Mon, 11 Apr 2022 01:47:12 +0000 Subject: [PATCH 2/4] Format unfmt-ed files --- pkg/model/deployment_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/model/deployment_test.go b/pkg/model/deployment_test.go index 3d707de581..3d8359f719 100644 --- a/pkg/model/deployment_test.go +++ b/pkg/model/deployment_test.go @@ -89,10 +89,10 @@ func TestDeployment_StageMap(t *testing.T) { }, }, want: map[string]*PipelineStage{ - "stage1": &PipelineStage{ + "stage1": { Id: "stage1", }, - "stage2": &PipelineStage{ + "stage2": { Id: "stage2", }, }, From 1aa337a280896fa6641739a9cedd3365edf476f8 Mon Sep 17 00:00:00 2001 From: knanao Date: Mon, 11 Apr 2022 11:18:39 +0900 Subject: [PATCH 3/4] Fix IsCompletedStage to IsCompleted method --- pkg/app/piped/controller/scheduler.go | 2 +- pkg/app/server/grpcapi/web_api.go | 2 +- pkg/model/deployment.go | 21 ++++++--------------- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 1a7ba4fa0f..0133de7c73 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -419,7 +419,7 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage defer func() { // When the piped has been terminated (PS kill) while the stage is still running // we should not mark the log persister as completed. - if !model.IsCompletedStage(finalStatus) && sig.Terminated() { + if !finalStatus.IsCompleted() && sig.Terminated() { return } lp.Complete(time.Minute) diff --git a/pkg/app/server/grpcapi/web_api.go b/pkg/app/server/grpcapi/web_api.go index 154b3c3481..5937fb2f1e 100644 --- a/pkg/app/server/grpcapi/web_api.go +++ b/pkg/app/server/grpcapi/web_api.go @@ -1029,7 +1029,7 @@ func (a *WebAPI) ApproveStage(ctx context.Context, req *webservice.ApproveStageR if !ok { return nil, status.Error(codes.FailedPrecondition, "The stage was not found in the deployment") } - if model.IsCompletedStage(stage.Status) { + if stage.Status.IsCompleted() { return nil, status.Errorf(codes.FailedPrecondition, "Could not approve the stage because it was already completed") } diff --git a/pkg/model/deployment.go b/pkg/model/deployment.go index 4eff4bf708..e9ed27dc8a 100644 --- a/pkg/model/deployment.go +++ b/pkg/model/deployment.go @@ -53,21 +53,12 @@ func IsSuccessfullyCompletedDeployment(status DeploymentStatus) bool { return status == DeploymentStatus_DEPLOYMENT_SUCCESS } -// IsCompletedStage checks whether the stage is at a completion state. -func IsCompletedStage(status StageStatus) bool { - if status.String() == "" { - return false - } - - switch status { - case StageStatus_STAGE_SUCCESS: - return true - case StageStatus_STAGE_FAILURE: - return true - case StageStatus_STAGE_CANCELLED: - return true - } - return false +// IsCompleted checks whether the stage is at a completion state. +func (s StageStatus) IsCompleted() bool { + return s == StageStatus_STAGE_SUCCESS || + s == StageStatus_STAGE_FAILURE || + s == StageStatus_STAGE_CANCELLED || + s == StageStatus_STAGE_SKIPPED } // CanUpdateDeploymentStatus checks whether the deployment can transit to the given status. From da4fc65bdf696355804dcc1773f8e0233161b0fe Mon Sep 17 00:00:00 2001 From: knanao Date: Mon, 11 Apr 2022 16:56:57 +0900 Subject: [PATCH 4/4] Add test --- pkg/model/deployment_test.go | 40 ++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/pkg/model/deployment_test.go b/pkg/model/deployment_test.go index 3d8359f719..33e3f561e8 100644 --- a/pkg/model/deployment_test.go +++ b/pkg/model/deployment_test.go @@ -110,3 +110,43 @@ func TestDeployment_StageMap(t *testing.T) { }) } } + +func TestStageStatus_IsCompleted(t *testing.T) { + testcases := []struct { + name string + status StageStatus + want bool + }{ + { + name: "running", + status: StageStatus_STAGE_RUNNING, + want: false, + }, + { + name: "success", + status: StageStatus_STAGE_SUCCESS, + want: true, + }, + { + name: "failure", + status: StageStatus_STAGE_FAILURE, + want: true, + }, + { + name: "cancelled", + status: StageStatus_STAGE_CANCELLED, + want: true, + }, + { + name: "skipped", + status: StageStatus_STAGE_SKIPPED, + want: true, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + got := tc.status.IsCompleted() + assert.Equal(t, tc.want, got) + }) + } +}