diff --git a/pkg/apis/pipeline/v1alpha1/conversion_error.go b/pkg/apis/pipeline/v1alpha1/conversion_error.go index 990b34d9b95..e666cd5fcdb 100644 --- a/pkg/apis/pipeline/v1alpha1/conversion_error.go +++ b/pkg/apis/pipeline/v1alpha1/conversion_error.go @@ -26,6 +26,10 @@ const ( // resources when they cannot be converted to warn of a forthcoming // breakage. ConditionTypeConvertible apis.ConditionType = v1beta1.ConditionTypeConvertible + // Conversion Error Field for finally + ConversionErrorFinally = "Finally" + // Conversion Error descriptive message for finally + ConversionErrorFinallyMsg = ConversionErrorFinally + " 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..8206fe8da0b 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_conversion.go @@ -51,6 +51,7 @@ func (source *PipelineSpec) ConvertTo(ctx context.Context, sink *v1beta1.Pipelin } } } + sink.Finally = nil return nil } @@ -97,6 +98,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(ConversionErrorFinally, ConversionErrorFinallyMsg) + } return nil } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go index 1a38d6e3dab..2d1180739d6 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,77 @@ 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) + } + t.Logf("ConvertTo() = %#v", ver) + // 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 == ConversionErrorFinally { + 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 contains the field name which resulted in the failure and should be equal to "Finally" here + if ok && cce.Field == ConversionErrorFinally { + return + } + t.Errorf("ConvertFrom() should have failed") + } + }) +} diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_defaults_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_defaults_test.go new file mode 100644 index 00000000000..d565b9b769a --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/pipeline_defaults_test.go @@ -0,0 +1,118 @@ +/* +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 v1alpha1_test + +import ( + "context" + "testing" + + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + + "github.com/google/go-cmp/cmp" +) + +func TestPipeline_SetDefaults(t *testing.T) { + cases := []struct { + desc string + p *v1alpha1.Pipeline + want *v1alpha1.Pipeline + }{{ + desc: "empty pipeline", + p: &v1alpha1.Pipeline{}, + want: &v1alpha1.Pipeline{}, + }} + for _, tc := range cases { + t.Run(tc.desc, func(t *testing.T) { + ctx := context.Background() + tc.p.SetDefaults(ctx) + if diff := cmp.Diff(tc.want, tc.p); diff != "" { + t.Errorf("Mismatch of Pipeline: %s", diff) + } + }) + } +} + +func TestPipelineSpec_SetDefaults(t *testing.T) { + cases := []struct { + desc string + ps *v1alpha1.PipelineSpec + want *v1alpha1.PipelineSpec + }{{ + desc: "empty pipelineSpec", + ps: &v1alpha1.PipelineSpec{}, + want: &v1alpha1.PipelineSpec{}, + }, { + desc: "pipeline task - default task kind must be " + string(v1alpha1.NamespacedTaskKind), + ps: &v1alpha1.PipelineSpec{ + Tasks: []v1alpha1.PipelineTask{{ + Name: "foo", TaskRef: &v1alpha1.TaskRef{Name: "foo-task"}, + }}, + }, + want: &v1alpha1.PipelineSpec{ + Tasks: []v1alpha1.PipelineTask{{ + Name: "foo", TaskRef: &v1alpha1.TaskRef{Name: "foo-task", Kind: v1alpha1.NamespacedTaskKind}, + }}, + }, + }, { + desc: "param type - default param type must be " + string(v1alpha1.ParamTypeString), + ps: &v1alpha1.PipelineSpec{ + Params: []v1alpha1.ParamSpec{{ + Name: "string-param", + }}, + }, + want: &v1alpha1.PipelineSpec{ + Params: []v1alpha1.ParamSpec{{ + Name: "string-param", Type: v1alpha1.ParamTypeString, + }}, + }, + }, { + desc: "pipeline task with taskSpec - default param type must be " + string(v1alpha1.ParamTypeString), + ps: &v1alpha1.PipelineSpec{ + Tasks: []v1alpha1.PipelineTask{{ + Name: "foo", TaskSpec: &v1alpha1.TaskSpec{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1alpha1.ParamSpec{{ + Name: "string-param", + }}, + }, + }, + }}, + }, + want: &v1alpha1.PipelineSpec{ + Tasks: []v1alpha1.PipelineTask{{ + Name: "foo", TaskSpec: &v1alpha1.TaskSpec{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1alpha1.ParamSpec{{ + Name: "string-param", + Type: v1alpha1.ParamTypeString, + }}, + }, + }, + }}, + }, + }} + for _, tc := range cases { + t.Run(tc.desc, func(t *testing.T) { + ctx := context.Background() + tc.ps.SetDefaults(ctx) + if diff := cmp.Diff(tc.want, tc.ps); diff != "" { + t.Errorf("Mismatch of PipelineSpec: %s", diff) + } + }) + } +} 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..51f062f1aab --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/pipeline_defaults_test.go @@ -0,0 +1,145 @@ +/* +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/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" +) + +func TestPipeline_SetDefaults(t *testing.T) { + cases := []struct { + desc string + p *v1beta1.Pipeline + want *v1beta1.Pipeline + }{{ + desc: "empty pipeline", + p: &v1beta1.Pipeline{}, + want: &v1beta1.Pipeline{}, + }} + for _, tc := range cases { + t.Run(tc.desc, func(t *testing.T) { + ctx := context.Background() + tc.p.SetDefaults(ctx) + if diff := cmp.Diff(tc.want, tc.p); diff != "" { + t.Errorf("Mismatch of Pipeline: %s", diff) + } + }) + } +} + +func TestPipelineSpec_SetDefaults(t *testing.T) { + cases := []struct { + desc string + ps *v1beta1.PipelineSpec + want *v1beta1.PipelineSpec + }{{ + desc: "empty pipelineSpec", + 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 diff := cmp.Diff(tc.want, tc.ps); diff != "" { + t.Errorf("Mismatch of PipelineSpec: %s", diff) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go index 6a4234bf2e1..a36249c1a04 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 ea12e6450a3..004a0d03860 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 []WorkspacePipelineDeclaration, pts []PipelineTask) *apis.FieldError { +func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { // Workspace names must be non-empty and unique. wsTable := make(map[string]struct{}) for i, ws := range wss { @@ -270,6 +297,16 @@ func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []Pipeli } } } + for tIdx, t := range finalTasks { + for wsIdx, 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]", tIdx, wsIdx), + ) + } + } + } return nil } @@ -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 := validateTaskResultReference(finalTasks); err != nil { + return err + } + + if err := validateTasksInputFrom(finalTasks); err != nil { + return err + } + + return nil +} + +func validateTaskResultReference(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 06a3574a77d..27e5815bef2 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -260,7 +260,7 @@ 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) } @@ -315,7 +315,7 @@ 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") } @@ -540,7 +540,7 @@ 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) } @@ -619,7 +619,7 @@ 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") } @@ -993,7 +993,7 @@ func TestValidatePipelineWorkspaces_Success(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() returned error: %v", err) } @@ -1039,10 +1039,395 @@ 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.Error("Pipeline.validatePipelineWorkspaces() did not return error, wanted error") } }) } } + +func TestValidatePipelineWithFinalTasks(t *testing.T) { + tests := []struct { + name string + p *Pipeline + failureExpected bool + }{{ + 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"}, + }}, + }, + }}, + }, + }, + failureExpected: false, + }, { + 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", + }}, + }, + }}, + }, + }, + failureExpected: false, + }, { + name: "invalid pipeline without any non-final task 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"}, + }}, + }, + }, + failureExpected: true, + }, { + name: "invalid pipeline without any non-final 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"}, + }}, + }, + }, + failureExpected: true, + }, { + name: "invalid pipeline with 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{{}}, + }, + }, + failureExpected: true, + }, { + 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"}, + }}, + }, + }, + failureExpected: true, + }, { + name: "invalid pipeline with same task label for final and non final task", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "same-task-label", + TaskRef: &TaskRef{Name: "non-final-task"}, + }}, + Finally: []PipelineTask{{ + Name: "same-task-label", + TaskRef: &TaskRef{Name: "final-task"}, + }}, + }, + }, + failureExpected: true, + }, { + 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", + }}, + }, + }, + failureExpected: true, + }, { + name: "undefined parameter variable in final task", + 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)"}, + }}, + }}, + }, + }, + failureExpected: true, + }, { + name: "invalid pipeline - final task with duplicate resource declaration", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Resources: []PipelineDeclaredResource{{ + Name: "duplicate-resource", Type: PipelineResourceTypeGit, + }, { + Name: "duplicate-resource", Type: PipelineResourceTypeGit, + }}, + Tasks: []PipelineTask{{ + Name: "non-final-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "foo-task"}, + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + Name: "the-resource", Resource: "duplicate-resource", + }}, + }, + }}, + }, + }, + failureExpected: true, + }, { + name: "invalid pipeline with invalid 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"}, + RunAfter: []string{"non-final-task"}, + }, { + Name: "final-task-2", + TaskRef: &TaskRef{Name: "final-task"}, + Conditions: []PipelineTaskCondition{{ + ConditionRef: "some-condition", + }}, + }}, + }, + }, + failureExpected: true, + }, { + 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", + }}, + }, + }, + failureExpected: true, + }, { + name: "invalid pipeline with empty finally", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Finally: []PipelineTask{{}}, + }, + }, + failureExpected: true, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.p.Validate(context.Background()) + if (!tt.failureExpected) && (err != nil) { + t.Errorf("Pipeline.Validate() returned error: %v", err) + } + + if tt.failureExpected && (err == nil) { + t.Error("Pipeline.Validate() did not return error, wanted error") + } + }) + } +} + +func TestValidateTasksAndFinallySection(t *testing.T) { + tests := []struct { + name string + ps *PipelineSpec + failureExpected bool + }{{ + 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"}, + }}, + }, + failureExpected: false, + }, { + name: "invalid pipeline with empty tasks and a few final tasks", + ps: &PipelineSpec{ + Tasks: nil, + Finally: []PipelineTask{{ + Name: "final-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + }, + failureExpected: true, + }, { + name: "valid pipeline with tasks and finally section without any tasks", + ps: &PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "my-task", TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: nil, + }, + failureExpected: false, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateTasksAndFinallySection(tt.ps) + if (!tt.failureExpected) && (err != nil) { + t.Errorf("Pipeline.ValidateTasksAndFinallySection() returned error: %v", err) + } + + if tt.failureExpected && (err == nil) { + t.Error("Pipeline.ValidateTasksAndFinallySection() did not return error, wanted error") + } + }) + } +} + +func TestValidateFinalTasks(t *testing.T) { + tests := []struct { + name string + finalTasks []PipelineTask + failureExpected bool + }{{ + name: "invalid pipeline with final task specifying runAfter", + finalTasks: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + RunAfter: []string{"non-final-task"}, + }}, + failureExpected: true, + }, { + name: "invalid pipeline with final task specifying conditions", + finalTasks: []PipelineTask{{ + Name: "final-task", + TaskRef: &TaskRef{Name: "final-task"}, + Conditions: []PipelineTaskCondition{{ + ConditionRef: "some-condition", + }}, + }}, + failureExpected: true, + }, { + 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"}, + }}, + }, + }}, + failureExpected: true, + }, { + 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)"}, + }}, + }}, + failureExpected: true, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateFinalTasks(tt.finalTasks) + if (!tt.failureExpected) && (err != nil) { + t.Errorf("Pipeline.ValidateFinalTasks() returned error: %v", err) + } + + if tt.failureExpected && (err == nil) { + t.Error("Pipeline.ValidateFinalTasks() did not return error, wanted error") + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index a8c0313d3b0..d4d9becf209 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 } diff --git a/third_party/github.com/hashicorp/go-multierror/go.mod b/third_party/github.com/hashicorp/go-multierror/go.mod index 2534331d5f9..0afe8e6f9d6 100644 --- a/third_party/github.com/hashicorp/go-multierror/go.mod +++ b/third_party/github.com/hashicorp/go-multierror/go.mod @@ -1,3 +1,5 @@ module github.com/hashicorp/go-multierror +go 1.14 + require github.com/hashicorp/errwrap v1.0.0