From 34495f504c733ac01c09bca27425c1519b5ebc79 Mon Sep 17 00:00:00 2001 From: Priti Desai Date: Tue, 5 May 2020 22:57:02 -0700 Subject: [PATCH] adding finally type at the pipeline level these changes are adding finally type and its validation, does not implement this new functionality. --- docs/pipelines.md | 29 + .../pipeline/v1alpha1/conversion_error.go | 2 + .../pipeline/v1alpha1/pipeline_conversion.go | 7 + .../v1alpha1/pipeline_conversion_test.go | 119 ++- .../pipeline/v1beta1/pipeline_defaults.go | 10 + .../v1beta1/pipeline_defaults_test.go | 136 ++++ pkg/apis/pipeline/v1beta1/pipeline_types.go | 4 + .../pipeline/v1beta1/pipeline_validation.go | 114 ++- .../v1beta1/pipeline_validation_test.go | 706 +++++++++++++----- .../pipeline/v1beta1/zz_generated.deepcopy.go | 7 + 10 files changed, 907 insertions(+), 227 deletions(-) create mode 100644 pkg/apis/pipeline/v1beta1/pipeline_defaults_test.go diff --git a/docs/pipelines.md b/docs/pipelines.md index 6b9f2716f6b..c5e8dd9d897 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -21,6 +21,7 @@ weight: 3 - [Configuring execution results at the `Pipeline` level](#configuring-execution-results-at-the-pipeline-level) - [Configuring the `Task` execution order](#configuring-the-task-execution-order) - [Adding a description](#adding-a-description) + - [Adding `Finally` to the `Pipeline` (Preview)](#adding-finally-to-the-pipeline-preview) - [Code examples](#code-examples) ## Overview @@ -529,6 +530,34 @@ In particular: The `description` field is an optional field and can be used to provide description of the `Pipeline`. +## Adding `Finally` to the `Pipeline` (Preview) + +_Finally type is available in the `Pipeline` but functionality is in progress. Final tasks are can be specified and +are validated but not executed yet._ + +You can specify a list of one or more final tasks under `finally` section. Final tasks are guaranteed to be executed +in parallel after all `PipelineTasks` under `tasks` have completed regardless of success or error. Final tasks are very +similar to `PipelineTasks` under `tasks` section and follow the same syntax. Each final task must have a +[valid](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) `name` and a [taskRef or +taskSpec](taskruns.md#specifying-the-target-task). For example: + +```yaml +spec: + tasks: + - name: tests + taskRef: + Name: integration-test + finally: + - name: cleanup-test + taskRef: + Name: cleanup +``` + +_[PR #2661](https://github.com/tektoncd/pipeline/pull/2661) is implementing this new functionality by adding support to enable +final tasks along with workspaces and parameters. `PipelineRun` status is being updated to include execution status of +final tasks i.e. `PipelineRun` status is set to success or failure depending on execution of `PipelineTasks`, this status +remains same when all final tasks finishes successfully but is set to failure if any of the final tasks fail._ + ## Code examples For a better understanding of `Pipelines`, study [our code examples](https://github.com/tektoncd/pipeline/tree/master/examples). diff --git a/pkg/apis/pipeline/v1alpha1/conversion_error.go b/pkg/apis/pipeline/v1alpha1/conversion_error.go index 990b34d9b95..5c8a01e588e 100644 --- a/pkg/apis/pipeline/v1alpha1/conversion_error.go +++ b/pkg/apis/pipeline/v1alpha1/conversion_error.go @@ -26,6 +26,8 @@ const ( // resources when they cannot be converted to warn of a forthcoming // breakage. ConditionTypeConvertible apis.ConditionType = v1beta1.ConditionTypeConvertible + // Conversion Error message for a field not available in v1alpha1 + ConversionErrorFieldNotAvailableMsg = "the specified field/section is not available in v1alpha1" ) // CannotConvertError is returned when a field cannot be converted. diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go b/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go index 26861e27a5b..787fd3a7827 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go @@ -25,6 +25,8 @@ import ( "knative.dev/pkg/apis" ) +const FinallyFieldName = "finally" + var _ apis.Convertible = (*Pipeline)(nil) // ConvertTo implements api.Convertible @@ -51,6 +53,7 @@ func (source *PipelineSpec) ConvertTo(ctx context.Context, sink *v1beta1.Pipelin } } } + sink.Finally = nil return nil } @@ -97,6 +100,10 @@ func (sink *PipelineSpec) ConvertFrom(ctx context.Context, source v1beta1.Pipeli } } } + // finally clause was introduced in v1beta1 and not available in v1alpha1 + if len(source.Finally) > 0 { + return ConvertErrorf(FinallyFieldName, ConversionErrorFieldNotAvailableMsg) + } return nil } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go index f4d54a41cda..ab78a966927 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go @@ -42,13 +42,12 @@ func TestPipelineConversionBadType(t *testing.T) { } } -func TestPipelineConversion(t *testing.T) { +func TestPipelineConversion_Success(t *testing.T) { versions := []apis.Convertible{&v1beta1.Pipeline{}} tests := []struct { - name string - in *Pipeline - wantErr bool + name string + in *Pipeline }{{ name: "simple conversion", in: &Pipeline{ @@ -114,7 +113,38 @@ func TestPipelineConversion(t *testing.T) { }}, }, }, - }, { + }} + + for _, test := range tests { + for _, version := range versions { + t.Run(test.name, func(t *testing.T) { + ver := version + // convert v1alpha1 Pipeline to v1beta1 Pipeline + if err := test.in.ConvertTo(context.Background(), ver); err != nil { + t.Errorf("ConvertTo() = %v", err) + } + got := &Pipeline{} + // converting it back to v1alpha1 pipeline and storing it in got variable to compare with original input + if err := got.ConvertFrom(context.Background(), ver); err != nil { + t.Errorf("ConvertFrom() = %v", err) + } + // compare origin input and roundtrip Pipeline i.e. v1alpha1 pipeline converted to v1beta1 and then converted back to v1alpha1 + // this check is making sure that we do not end up with different object than what we started with + if d := cmp.Diff(test.in, got); d != "" { + t.Errorf("roundtrip %s", diff.PrintWantGot(d)) + } + }) + } + } +} + +func TestPipelineConversion_Failure(t *testing.T) { + versions := []apis.Convertible{&v1beta1.Pipeline{}} + + tests := []struct { + name string + in *Pipeline + }{{ name: "simple conversion with task spec error", in: &Pipeline{ ObjectMeta: metav1.ObjectMeta{ @@ -152,29 +182,76 @@ func TestPipelineConversion(t *testing.T) { }}, }, }, - wantErr: true, }} - for _, test := range tests { for _, version := range versions { t.Run(test.name, func(t *testing.T) { ver := version - if err := test.in.ConvertTo(context.Background(), ver); err != nil { - if !test.wantErr { - t.Errorf("ConvertTo() = %v", err) - } - return - } - t.Logf("ConvertTo() = %#v", ver) - got := &Pipeline{} - if err := got.ConvertFrom(context.Background(), ver); err != nil { - t.Errorf("ConvertFrom() = %v", err) - } - t.Logf("ConvertFrom() = %#v", got) - if d := cmp.Diff(test.in, got); d != "" { - t.Errorf("roundtrip %s", diff.PrintWantGot(d)) + if err := test.in.ConvertTo(context.Background(), ver); err == nil { + t.Errorf("Expected ConvertTo to fail but did not produce any error") } + return }) } } } + +func TestPipelineConversionFromWithFinally(t *testing.T) { + versions := []apis.Convertible{&v1beta1.Pipeline{}} + p := &Pipeline{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + Generation: 1, + }, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, + }, + } + for _, version := range versions { + t.Run("finally not available in v1alpha1", func(t *testing.T) { + ver := version + // convert v1alpha1 to v1beta1 + if err := p.ConvertTo(context.Background(), ver); err != nil { + t.Errorf("ConvertTo() = %v", err) + } + // modify ver to introduce new field which causes failure to convert v1beta1 to v1alpha1 + source := ver + source.(*v1beta1.Pipeline).Spec.Finally = []v1beta1.PipelineTask{{Name: "finaltask", TaskRef: &TaskRef{Name: "task"}}} + got := &Pipeline{} + if err := got.ConvertFrom(context.Background(), source); err != nil { + cce, ok := err.(*CannotConvertError) + // conversion error contains the field name which resulted in the failure and should be equal to "Finally" here + if ok && cce.Field == FinallyFieldName { + return + } + t.Errorf("ConvertFrom() should have failed") + } + }) + } +} + +func TestPipelineConversionFromBetaToAlphaWithFinally_Failure(t *testing.T) { + p := &v1beta1.Pipeline{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + Generation: 1, + }, + Spec: v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, + Finally: []v1beta1.PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, + }, + } + t.Run("finally not available in v1alpha1", func(t *testing.T) { + got := &Pipeline{} + if err := got.ConvertFrom(context.Background(), p); err != nil { + cce, ok := err.(*CannotConvertError) + // conversion error (cce) contains the field name which resulted in the failure and should be equal to "finally" here + if ok && cce.Field == FinallyFieldName { + return + } + t.Errorf("ConvertFrom() should have failed") + } + }) +} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go index 67ea7de2917..b4cbb8fec11 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go @@ -42,4 +42,14 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) { for i := range ps.Params { ps.Params[i].SetDefaults(ctx) } + for _, ft := range ps.Finally { + if ft.TaskRef != nil { + if ft.TaskRef.Kind == "" { + ft.TaskRef.Kind = NamespacedTaskKind + } + } + if ft.TaskSpec != nil { + ft.TaskSpec.SetDefaults(ctx) + } + } } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_defaults_test.go b/pkg/apis/pipeline/v1beta1/pipeline_defaults_test.go new file mode 100644 index 00000000000..85057d115ba --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/pipeline_defaults_test.go @@ -0,0 +1,136 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1_test + +import ( + "context" + "testing" + + "github.com/tektoncd/pipeline/test/diff" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" +) + +func TestPipeline_SetDefaults(t *testing.T) { + p := &v1beta1.Pipeline{} + want := &v1beta1.Pipeline{} + ctx := context.Background() + p.SetDefaults(ctx) + if d := cmp.Diff(want, p); d != "" { + t.Errorf("Mismatch of Pipeline: empty pipeline must not change after setting defaults: %s", diff.PrintWantGot(d)) + } +} + +func TestPipelineSpec_SetDefaults(t *testing.T) { + cases := []struct { + desc string + ps *v1beta1.PipelineSpec + want *v1beta1.PipelineSpec + }{{ + desc: "empty pipelineSpec must not change after setting defaults", + ps: &v1beta1.PipelineSpec{}, + want: &v1beta1.PipelineSpec{}, + }, { + desc: "pipeline task - default task kind must be " + string(v1beta1.NamespacedTaskKind), + ps: &v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + }, + want: &v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", TaskRef: &v1beta1.TaskRef{Name: "foo-task", Kind: v1beta1.NamespacedTaskKind}, + }}, + }, + }, { + desc: "final pipeline task - default task kind must be " + string(v1beta1.NamespacedTaskKind), + ps: &v1beta1.PipelineSpec{ + Finally: []v1beta1.PipelineTask{{ + Name: "final-task", TaskRef: &v1beta1.TaskRef{Name: "foo-task"}, + }}, + }, + want: &v1beta1.PipelineSpec{ + Finally: []v1beta1.PipelineTask{{ + Name: "final-task", TaskRef: &v1beta1.TaskRef{Name: "foo-task", Kind: v1beta1.NamespacedTaskKind}, + }}, + }, + }, { + desc: "param type - default param type must be " + string(v1beta1.ParamTypeString), + ps: &v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "string-param", + }}, + }, + want: &v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "string-param", Type: v1beta1.ParamTypeString, + }}, + }, + }, { + desc: "pipeline task with taskSpec - default param type must be " + string(v1beta1.ParamTypeString), + ps: &v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", TaskSpec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "string-param", + }}, + }, + }}, + }, + want: &v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + Name: "foo", TaskSpec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "string-param", + Type: v1beta1.ParamTypeString, + }}, + }, + }}, + }, + }, { + desc: "final pipeline task with taskSpec - default param type must be " + string(v1beta1.ParamTypeString), + ps: &v1beta1.PipelineSpec{ + Finally: []v1beta1.PipelineTask{{ + Name: "foo", TaskSpec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "string-param", + }}, + }, + }}, + }, + want: &v1beta1.PipelineSpec{ + Finally: []v1beta1.PipelineTask{{ + Name: "foo", TaskSpec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "string-param", + Type: v1beta1.ParamTypeString, + }}, + }, + }}, + }, + }} + for _, tc := range cases { + t.Run(tc.desc, func(t *testing.T) { + ctx := context.Background() + tc.ps.SetDefaults(ctx) + if d := cmp.Diff(tc.want, tc.ps); d != "" { + t.Errorf("Mismatch of pipelineSpec after setting defaults: %s: %s", tc.desc, diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go index e1d6b10a91d..74cfe3dc5fe 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types.go @@ -71,6 +71,10 @@ type PipelineSpec struct { // Results are values that this pipeline can output once run // +optional Results []PipelineResult `json:"results,omitempty"` + // Finally declares the list of Tasks that execute just before leaving the Pipeline + // i.e. either after all Tasks are finished executing successfully + // or after a failure which would result in ending the Pipeline + Finally []PipelineTask `json:"finally,omitempty"` } // PipelineResult used to describe the results of a pipeline diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 1798ad87be3..94d9faf41e8 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -43,7 +43,7 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { // validateDeclaredResources ensures that the specified resources have unique names and // validates that all the resources referenced by pipeline tasks are declared in the pipeline -func validateDeclaredResources(resources []PipelineDeclaredResource, tasks []PipelineTask) error { +func validateDeclaredResources(resources []PipelineDeclaredResource, tasks []PipelineTask, finalTasks []PipelineTask) error { encountered := map[string]struct{}{} for _, r := range resources { if _, ok := encountered[r.Name]; ok { @@ -68,6 +68,16 @@ func validateDeclaredResources(resources []PipelineDeclaredResource, tasks []Pip } } } + for _, t := range finalTasks { + if t.Resources != nil { + for _, input := range t.Resources.Inputs { + required = append(required, input.Resource) + } + for _, output := range t.Resources.Outputs { + required = append(required, output.Resource) + } + } + } provided := make([]string, 0, len(resources)) for _, resource := range resources { @@ -146,13 +156,13 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { } // PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified - if err := validatePipelineTasks(ctx, ps.Tasks); err != nil { + if err := validatePipelineTasks(ctx, ps.Tasks, ps.Finally); err != nil { return err } // All declared resources should be used, and the Pipeline shouldn't try to use any resources // that aren't declared - if err := validateDeclaredResources(ps.Resources, ps.Tasks); err != nil { + if err := validateDeclaredResources(ps.Resources, ps.Tasks, ps.Finally); err != nil { return apis.ErrInvalidValue(err.Error(), "spec.resources") } @@ -175,8 +185,12 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { return err } + if err := validatePipelineParameterVariables(ps.Finally, ps.Params); err != nil { + return err + } + // Validate the pipeline's workspaces. - if err := validatePipelineWorkspaces(ps.Workspaces, ps.Tasks); err != nil { + if err := validatePipelineWorkspaces(ps.Workspaces, ps.Tasks, ps.Finally); err != nil { return err } @@ -185,12 +199,20 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { return apis.ErrInvalidValue(err.Error(), "spec.tasks.params.value") } + if err := validateTasksAndFinallySection(ps); err != nil { + return err + } + + if err := validateFinalTasks(ps.Finally); err != nil { + return err + } + return nil } // validatePipelineTasks ensures that pipeline tasks has unique label, pipeline tasks has specified one of // taskRef or taskSpec, and in case of a pipeline task with taskRef, it has a reference to a valid task (task name) -func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.FieldError { +func validatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { // Names cannot be duplicated taskNames := map[string]struct{}{} var err *apis.FieldError @@ -199,6 +221,11 @@ func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.Fiel return err } } + for i, t := range finalTasks { + if err = validatePipelineTaskName(ctx, "spec.finally", i, t, taskNames); err != nil { + return err + } + } return nil } @@ -245,7 +272,7 @@ func validatePipelineTaskName(ctx context.Context, prefix string, i int, t Pipel // validatePipelineWorkspaces validates the specified workspaces, ensuring having unique name without any empty string, // and validates that all the referenced workspaces (by pipeline tasks) are specified in the pipeline -func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []PipelineTask) *apis.FieldError { +func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { // Workspace names must be non-empty and unique. wsTable := make(map[string]struct{}) for i, ws := range wss { @@ -260,12 +287,22 @@ func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []Pipeli // Any workspaces used in PipelineTasks should have their name declared in the Pipeline's // Workspaces list. - for ptIdx, pt := range pts { - for wsIdx, ws := range pt.Workspaces { + for i, pt := range pts { + for j, ws := range pt.Workspaces { if _, ok := wsTable[ws.Workspace]; !ok { return apis.ErrInvalidValue( fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", pt.Name, ws.Workspace), - fmt.Sprintf("spec.tasks[%d].workspaces[%d]", ptIdx, wsIdx), + fmt.Sprintf("spec.tasks[%d].workspaces[%d]", i, j), + ) + } + } + } + for i, t := range finalTasks { + for j, ws := range t.Workspaces { + if _, ok := wsTable[ws.Workspace]; !ok { + return apis.ErrInvalidValue( + fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", t.Name, ws.Workspace), + fmt.Sprintf("spec.finally[%d].workspaces[%d]", i, j), ) } } @@ -396,3 +433,62 @@ func validatePipelineResults(results []PipelineResult) error { } return nil } + +func validateTasksAndFinallySection(ps *PipelineSpec) *apis.FieldError { + if len(ps.Finally) != 0 && len(ps.Tasks) == 0 { + return apis.ErrInvalidValue(fmt.Sprintf("spec.tasks is empty but spec.finally has %d tasks", len(ps.Finally)), "spec.finally") + } + return nil +} + +func validateFinalTasks(finalTasks []PipelineTask) *apis.FieldError { + for _, f := range finalTasks { + if len(f.RunAfter) != 0 { + return apis.ErrInvalidValue(fmt.Sprintf("no runAfter allowed under spec.finally, final task %s has runAfter specified", f.Name), "spec.finally") + } + if len(f.Conditions) != 0 { + return apis.ErrInvalidValue(fmt.Sprintf("no conditions allowed under spec.finally, final task %s has conditions specified", f.Name), "spec.finally") + } + } + + if err := validateTaskResultReferenceNotUsed(finalTasks); err != nil { + return err + } + + if err := validateTasksInputFrom(finalTasks); err != nil { + return err + } + + return nil +} + +func validateTaskResultReferenceNotUsed(tasks []PipelineTask) *apis.FieldError { + for _, t := range tasks { + for _, p := range t.Params { + expressions, ok := GetVarSubstitutionExpressionsForParam(p) + if ok { + if LooksLikeContainsResultRefs(expressions) { + return apis.ErrInvalidValue(fmt.Sprintf("no task result allowed under params,"+ + "final task param %s has set task result as its value", p.Name), "spec.finally.task.params") + } + } + } + } + return nil +} + +func validateTasksInputFrom(tasks []PipelineTask) *apis.FieldError { + for _, t := range tasks { + inputResources := []PipelineTaskInputResource{} + if t.Resources != nil { + inputResources = append(inputResources, t.Resources.Inputs...) + } + for _, rd := range inputResources { + if len(rd.From) != 0 { + return apis.ErrDisallowedFields(fmt.Sprintf("no from allowed under inputs,"+ + " final task %s has from specified", rd.Name), "spec.finally.task.resources.inputs") + } + } + } + return nil +} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index f7ba1c519e2..e73a40aaee2 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -101,7 +101,7 @@ func TestPipeline_Validate_Success(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := tt.p.Validate(context.Background()) if err != nil { - t.Errorf("Pipeline.Validate() returned error: %v", err) + t.Errorf("Pipeline.Validate() returned error for valid Pipeline: %s: %v", tt.name, err) } }) } @@ -134,7 +134,7 @@ func TestPipeline_Validate_Failure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := tt.p.Validate(context.Background()) if err == nil { - t.Error("Pipeline.Validate() did not return error, wanted error") + t.Errorf("Pipeline.Validate() did not return error for invalid pipeline: %s", tt.name) } }) } @@ -231,7 +231,7 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := tt.ps.Validate(context.Background()) if err == nil { - t.Error("PipelineSpec.Validate() did not return error, wanted error") + t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec: %s", tt.name) } }) } @@ -260,9 +260,9 @@ func TestValidatePipelineTasks_Success(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validatePipelineTasks(context.Background(), tt.tasks) + err := validatePipelineTasks(context.Background(), tt.tasks, []PipelineTask{}) if err != nil { - t.Errorf("Pipeline.validatePipelineTasks() returned error: %v", err) + t.Errorf("Pipeline.validatePipelineTasks() returned error for valid pipeline tasks: %s: %v", tt.name, err) } }) } @@ -315,49 +315,42 @@ func TestValidatePipelineTasks_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validatePipelineTasks(context.Background(), tt.tasks) + err := validatePipelineTasks(context.Background(), tt.tasks, []PipelineTask{}) if err == nil { - t.Error("Pipeline.validatePipelineTasks() did not return error, wanted error") + t.Error("Pipeline.validatePipelineTasks() did not return error for invalid pipeline tasks:", tt.name) } }) } } func TestValidateFrom_Success(t *testing.T) { - tests := []struct { - name string - tasks []PipelineTask - }{{ - name: "valid pipeline task - from resource referring to valid output resource of the pipeline task", - tasks: []PipelineTask{{ - Name: "bar", - TaskRef: &TaskRef{Name: "bar-task"}, - Resources: &PipelineTaskResources{ - Inputs: []PipelineTaskInputResource{{ - Name: "some-resource", Resource: "some-resource", - }}, - Outputs: []PipelineTaskOutputResource{{ - Name: "output-resource", Resource: "output-resource", - }}, - }, - }, { - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - Resources: &PipelineTaskResources{ - Inputs: []PipelineTaskInputResource{{ - Name: "wow-image", Resource: "output-resource", From: []string{"bar"}, - }}, - }, - }}, + desc := "valid pipeline task - from resource referring to valid output resource of the pipeline task" + tasks := []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "some-resource", Resource: "some-resource", + }}, + Outputs: []PipelineTaskOutputResource{{ + Name: "output-resource", Resource: "output-resource", + }}, + }, + }, { + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "wow-image", Resource: "output-resource", From: []string{"bar"}, + }}, + }, }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateFrom(tt.tasks) - if err != nil { - t.Errorf("Pipeline.validateFrom() returned error: %v", err) - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validateFrom(tasks) + if err != nil { + t.Errorf("Pipeline.validateFrom() returned error for: %s: %v", desc, err) + } + }) } func TestValidateFrom_Failure(t *testing.T) { @@ -449,7 +442,7 @@ func TestValidateFrom_Failure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := validateFrom(tt.tasks) if err == nil { - t.Error("Pipeline.validateFrom() did not return error, wanted error") + t.Error("Pipeline.validateFrom() did not return error for invalid pipeline task resources: ", tt.name) } }) } @@ -540,9 +533,9 @@ func TestValidateDeclaredResources_Success(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateDeclaredResources(tt.resources, tt.tasks) + err := validateDeclaredResources(tt.resources, tt.tasks, []PipelineTask{}) if err != nil { - t.Errorf("Pipeline.validateDeclaredResources() returned error: %v", err) + t.Errorf("Pipeline.validateDeclaredResources() returned error for valid resource declarations: %s: %v", tt.name, err) } }) } @@ -619,164 +612,123 @@ func TestValidateDeclaredResources_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateDeclaredResources(tt.resources, tt.tasks) + err := validateDeclaredResources(tt.resources, tt.tasks, []PipelineTask{}) if err == nil { - t.Error("Pipeline.validateDeclaredResources() did not return error, wanted error") + t.Errorf("Pipeline.validateDeclaredResources() did not return error for invalid resource declarations: %s", tt.name) } }) } } func TestValidateGraph_Success(t *testing.T) { - tests := []struct { - name string - tasks []PipelineTask - }{{ - name: "valid dependency graph with multiple tasks", - tasks: []PipelineTask{{ - Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, - }, { - Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, - }, { - Name: "foo1", TaskRef: &TaskRef{Name: "foo-task"}, RunAfter: []string{"foo"}, - }, { - Name: "bar1", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"bar"}, - }, { - Name: "foo-bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo1", "bar1"}, - }}, + desc := "valid dependency graph with multiple tasks" + tasks := []PipelineTask{{ + Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, + }, { + Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, + }, { + Name: "foo1", TaskRef: &TaskRef{Name: "foo-task"}, RunAfter: []string{"foo"}, + }, { + Name: "bar1", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"bar"}, + }, { + Name: "foo-bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo1", "bar1"}, }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateGraph(tt.tasks) - if err != nil { - t.Errorf("Pipeline.validateGraph() returned error: %v", err) - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validateGraph(tasks) + if err != nil { + t.Errorf("Pipeline.validateGraph() returned error for valid DAG of pipeline tasks: %s: %v", desc, err) + } + }) } func TestValidateGraph_Failure(t *testing.T) { - tests := []struct { - name string - tasks []PipelineTask - }{{ - name: "invalid dependency graph between the tasks with cyclic dependency", - tasks: []PipelineTask{{ - Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, RunAfter: []string{"bar"}, - }, { - Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"}, - }}, + desc := "invalid dependency graph between the tasks with cyclic dependency" + tasks := []PipelineTask{{ + Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, RunAfter: []string{"bar"}, + }, { + Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"}, }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateGraph(tt.tasks) - if err == nil { - t.Error("Pipeline.validateGraph() did not return error, wanted error") - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validateGraph(tasks) + if err == nil { + t.Error("Pipeline.validateGraph() did not return error for invalid DAG of pipeline tasks:", desc) + + } + }) } func TestValidateParamResults_Success(t *testing.T) { - tests := []struct { - name string - tasks []PipelineTask - }{{ - name: "invalid pipeline task referencing task result along with parameter variable", - tasks: []PipelineTask{{ - TaskSpec: &TaskSpec{ - Results: []TaskResult{{ - Name: "output", - }}, - Steps: []Step{{ - Container: corev1.Container{Name: "foo", Image: "bar"}, - }}, - }, - Name: "a-task", - }, { - Name: "foo", - TaskRef: &TaskRef{Name: "foo-task"}, - Params: []Param{{ - Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.foo) and $(tasks.a-task.results.output)"}, + desc := "valid pipeline task referencing task result along with parameter variable" + tasks := []PipelineTask{{ + TaskSpec: &TaskSpec{ + Results: []TaskResult{{ + Name: "output", }}, + Steps: []Step{{ + Container: corev1.Container{Name: "foo", Image: "bar"}, + }}, + }, + Name: "a-task", + }, { + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + Params: []Param{{ + Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.foo) and $(tasks.a-task.results.output)"}, }}, }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateParamResults(tt.tasks) - if err != nil { - t.Errorf("Pipeline.validateParamResults() returned error: %v", err) - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validateParamResults(tasks) + if err != nil { + t.Errorf("Pipeline.validateParamResults() returned error for valid pipeline: %s: %v", desc, err) + } + }) } func TestValidateParamResults_Failure(t *testing.T) { - tests := []struct { - name string - tasks []PipelineTask - }{{ - name: "invalid pipeline task referencing task results with malformed variable substitution expression", - tasks: []PipelineTask{{ - Name: "a-task", TaskRef: &TaskRef{Name: "a-task"}, - }, { - Name: "b-task", TaskRef: &TaskRef{Name: "b-task"}, - Params: []Param{{ - Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.a-task.resultTypo.bResult)"}}}, - }}, + desc := "invalid pipeline task referencing task results with malformed variable substitution expression" + tasks := []PipelineTask{{ + Name: "a-task", TaskRef: &TaskRef{Name: "a-task"}, + }, { + Name: "b-task", TaskRef: &TaskRef{Name: "b-task"}, + Params: []Param{{ + Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.a-task.resultTypo.bResult)"}}}, }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateParamResults(tt.tasks) - if err == nil { - t.Error("Pipeline.validateParamResults() did not return error, wanted error") - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validateParamResults(tasks) + if err == nil { + t.Errorf("Pipeline.validateParamResults() did not return error for invalid pipeline: %s", desc) + } + }) } func TestValidatePipelineResults_Success(t *testing.T) { - tests := []struct { - name string - results []PipelineResult - }{{ - name: "valid pipeline result", - results: []PipelineResult{{ - Name: "my-pipeline-result", - Description: "this is my pipeline result", - Value: "$(tasks.a-task.results.output)", - }}, + desc := "valid pipeline with valid pipeline results syntax" + results := []PipelineResult{{ + Name: "my-pipeline-result", + Description: "this is my pipeline result", + Value: "$(tasks.a-task.results.output)", }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validatePipelineResults(tt.results) - if err != nil { - t.Errorf("Pipeline.validatePipelineResults() returned error: %v", err) - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validatePipelineResults(results) + if err != nil { + t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err) + } + }) } func TestValidatePipelineResults_Failure(t *testing.T) { - tests := []struct { - name string - results []PipelineResult - }{{ - name: "invalid pipeline result reference", - results: []PipelineResult{{ - Name: "my-pipeline-result", - Description: "this is my pipeline result", - Value: "$(tasks.a-task.results.output.output)", - }}, + desc := "invalid pipeline result reference" + results := []PipelineResult{{ + Name: "my-pipeline-result", + Description: "this is my pipeline result", + Value: "$(tasks.a-task.results.output.output)", }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validatePipelineResults(tt.results) - if err == nil { - t.Error("Pipeline.validatePipelineResults() did not return error, wanted error") - } - }) - } + t.Run(desc, func(t *testing.T) { + err := validatePipelineResults(results) + if err == nil { + t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", desc) + } + }) } func TestValidatePipelineParameterVariables_Success(t *testing.T) { @@ -843,7 +795,7 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := validatePipelineParameterVariables(tt.tasks, tt.params) if err != nil { - t.Errorf("Pipeline.validatePipelineParameterVariables() returned error: %v", err) + t.Errorf("Pipeline.validatePipelineParameterVariables() returned error for valid pipeline parameters: %s: %v", tt.name, err) } }) } @@ -969,36 +921,28 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := validatePipelineParameterVariables(tt.tasks, tt.params) if err == nil { - t.Error("Pipeline.validatePipelineParameterVariables() did not return error, wanted error") + t.Errorf("Pipeline.validatePipelineParameterVariables() did not return error for invalid pipeline parameters: %s", tt.name) } }) } } func TestValidatePipelineWorkspaces_Success(t *testing.T) { - tests := []struct { - name string - workspaces []PipelineWorkspaceDeclaration - tasks []PipelineTask - }{{ - name: "unused pipeline spec workspaces do not cause an error", - workspaces: []PipelineWorkspaceDeclaration{{ - Name: "foo", - }, { - Name: "bar", - }}, - tasks: []PipelineTask{{ - Name: "foo", TaskRef: &TaskRef{Name: "foo"}, - }}, + desc := "unused pipeline spec workspaces do not cause an error" + workspaces := []PipelineWorkspaceDeclaration{{ + Name: "foo", + }, { + Name: "bar", }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validatePipelineWorkspaces(tt.workspaces, tt.tasks) - if err != nil { - t.Errorf("Pipeline.validatePipelineWorkspaces() returned error: %v", err) - } - }) - } + tasks := []PipelineTask{{ + Name: "foo", TaskRef: &TaskRef{Name: "foo"}, + }} + t.Run(desc, func(t *testing.T) { + err := validatePipelineWorkspaces(workspaces, tasks, []PipelineTask{}) + if err != nil { + t.Errorf("Pipeline.validatePipelineWorkspaces() returned error for valid pipeline workspaces: %s: %v", desc, err) + } + }) } func TestValidatePipelineWorkspaces_Failure(t *testing.T) { @@ -1039,9 +983,377 @@ func TestValidatePipelineWorkspaces_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validatePipelineWorkspaces(tt.workspaces, tt.tasks) + err := validatePipelineWorkspaces(tt.workspaces, tt.tasks, []PipelineTask{}) + if err == nil { + t.Errorf("Pipeline.validatePipelineWorkspaces() did not return error for invalid pipeline workspaces: %s", tt.name) + } + }) + } +} + +func TestValidatePipelineWithFinalTasks_Success(t *testing.T) { + tests := []struct { + name string + p *Pipeline + }{{ + name: "valid pipeline with final tasks", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task-1", + TaskRef: &TaskRef{Name: "final-task"}, + }, { + Name: "final-task-2", + TaskSpec: &TaskSpec{ + Steps: []Step{{ + Container: corev1.Container{Name: "foo", Image: "bar"}, + }}, + }, + }}, + }, + }, + }, { + name: "valid pipeline with resource declarations and their valid usage", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", Type: PipelineResourceTypeGit, + }, { + Name: "wonderful-resource", Type: PipelineResourceTypeImage, + }}, + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "bar-task"}, + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "some-workspace", Resource: "great-resource", + }}, + Outputs: []PipelineTaskOutputResource{{ + Name: "some-image", Resource: "wonderful-resource", + }}, + }, + Conditions: []PipelineTaskCondition{{ + ConditionRef: "some-condition", + Resources: []PipelineTaskInputResource{{ + Name: "some-workspace", Resource: "great-resource", + }}, + }}, + }}, + Finally: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "some-workspace", Resource: "great-resource", + }}, + Outputs: []PipelineTaskOutputResource{{ + Name: "some-image", Resource: "wonderful-resource", + }}, + }, + }}, + }, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.p.Validate(context.Background()) + if err != nil { + t.Errorf("Pipeline.Validate() returned error for valid pipeline with finally: %s: %v", tt.name, err) + } + }) + } +} + +func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { + tests := []struct { + name string + p *Pipeline + }{{ + name: "invalid pipeline without any non-final task (tasks set to nil) but at least one final task", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: nil, + Finally: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + }}, + }, + }, + }, { + name: "invalid pipeline without any non-final task (tasks set to empty list of pipeline task) but at least one final task", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{}}, + Finally: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + }}, + }, + }, + }, { + name: "invalid pipeline with valid non-final tasks but empty finally section", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{}}, + }, + }, + }, { + name: "invalid pipeline with duplicate final tasks", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + }, { + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + }}, + }, + }, + }, { + name: "invalid pipeline with same task name for final and non final task", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "common-task-name", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "common-task-name", + TaskRef: &TaskRef{Name: "final-task"}, + }}, + }, + }, + }, { + name: "final task missing tasfref and taskspec", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", + }}, + }, + }, + }, { + name: "final task with both tasfref and taskspec", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + TaskSpec: &TaskSpec{ + Steps: []Step{{ + Container: corev1.Container{Name: "foo", Image: "bar"}, + }}, + }, + }}, + }, + }, + }, { + name: "extra parameter called final-param provided to final task which is not specified in the Pipeline", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Params: []ParamSpec{{ + Name: "foo", Type: ParamTypeString, + }}, + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + Params: []Param{{ + Name: "final-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.foo) and $(params.does-not-exist)"}, + }}, + }}, + }, + }, + }, { + name: "invalid pipeline with invalid final tasks with runAfter and conditions", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task-1", + TaskRef: &TaskRef{Name: "final-task"}, + RunAfter: []string{"non-final-task"}, + }, { + Name: "final-task-2", + TaskRef: &TaskRef{Name: "final-task"}, + Conditions: []PipelineTaskCondition{{ + ConditionRef: "some-condition", + }}, + }}, + }, + }, + }, { + name: "invalid pipeline - workspace bindings in final task relying on a non-existent pipeline workspace", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", TaskRef: &TaskRef{Name: "foo"}, + Workspaces: []WorkspacePipelineTaskBinding{{ + Name: "shared-workspace", + Workspace: "pipeline-shared-workspace", + }}, + }}, + Workspaces: []WorkspacePipelineDeclaration{{ + Name: "foo", + }}, + }, + }, + }, { + name: "invalid pipeline with no tasks under tasks section and empty finally section", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Finally: []PipelineTask{{}}, + }, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.p.Validate(context.Background()) + if err == nil { + t.Errorf("Pipeline.Validate() did not return error for invalid pipeline with finally: %s", tt.name) + } + }) + } +} + +func TestValidateTasksAndFinallySection_Success(t *testing.T) { + tests := []struct { + name string + ps *PipelineSpec + }{{ + name: "pipeline with tasks and final tasks", + ps: &PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "non-final-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "valid pipeline with tasks and finally section without any tasks", + ps: &PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "my-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: nil, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateTasksAndFinallySection(tt.ps) + if err != nil { + t.Errorf("Pipeline.ValidateTasksAndFinallySection() returned error for valid pipeline with finally: %s: %v", tt.name, err) + } + }) + } +} + +func TestValidateTasksAndFinallySection_Failure(t *testing.T) { + desc := "invalid pipeline with empty tasks and a few final tasks" + ps := &PipelineSpec{ + Tasks: nil, + Finally: []PipelineTask{{ + Name: "final-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + } + t.Run(desc, func(t *testing.T) { + err := validateTasksAndFinallySection(ps) + if err == nil { + t.Errorf("Pipeline.ValidateTasksAndFinallySection() did not return error for invalid pipeline with finally: %s", desc) + } + }) +} + +func TestValidateFinalTasks_Failure(t *testing.T) { + tests := []struct { + name string + finalTasks []PipelineTask + }{{ + name: "invalid pipeline with final task specifying runAfter", + finalTasks: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + RunAfter: []string{"non-final-task"}, + }}, + }, { + name: "invalid pipeline with final task specifying conditions", + finalTasks: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + Conditions: []PipelineTaskCondition{{ + ConditionRef: "some-condition", + }}, + }}, + }, { + name: "invalid pipeline with final task output resources referring to other task input", + finalTasks: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "final-input-2", Resource: "great-resource", From: []string{"task"}, + }}, + }, + }}, + }, { + name: "invalid pipeline with final tasks having reference to task results", + finalTasks: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + Params: []Param{{ + Name: "param1", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.a-task.results.output)"}, + }}, + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateFinalTasks(tt.finalTasks) if err == nil { - t.Error("Pipeline.validatePipelineWorkspaces() did not return error, wanted error") + t.Errorf("Pipeline.ValidateFinalTasks() did not return error for invalid pipeline: %s", tt.name) } }) } diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 2bd08163d3a..0017ae23937 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -785,6 +785,13 @@ func (in *PipelineSpec) DeepCopyInto(out *PipelineSpec) { *out = make([]PipelineResult, len(*in)) copy(*out, *in) } + if in.Finally != nil { + in, out := &in.Finally, &out.Finally + *out = make([]PipelineTask, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return }