Refactor Pipeline Workspaces Validation

In this change, we split up the long function for `workspaces` validation and remove duplications. The first function validates the `workspaces` declarations in the `pipeline` - that they are unique and non-empty. The second function validates the usage of `workspaces` in `pipelinetasks` - which is reused for both `tasks` and `finally`.
nicolaerosia · Mar 15, 2022 · f2da9a8 · f2da9a8
1 parent 45b7c37
commit f2da9a8
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 49 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go
@@ -372,6 +372,18 @@ func validateExecutionStatusVariablesExpressions(expressions []string, ptNames s
 	return errs
 }
 
+func (pt *PipelineTask) validateWorkspaces(workspaceNames sets.String) (errs *apis.FieldError) {
+	for i, ws := range pt.Workspaces {
+		if !workspaceNames.Has(ws.Workspace) {
+			errs = errs.Also(apis.ErrInvalidValue(
+				fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", pt.Name, ws.Workspace),
+				"",
+			).ViaFieldIndex("workspaces", i))
+		}
+	}
+	return errs
+}
+
 // TaskSpecMetadata returns the metadata of the PipelineTask's EmbeddedTask spec.
 func (pt *PipelineTask) TaskSpecMetadata() PipelineTaskMetadata {
 	return pt.TaskSpec.Metadata

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go
@@ -65,7 +65,9 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	errs = errs.Also(validatePipelineContextVariables(ps.Finally).ViaField("finally"))
 	errs = errs.Also(validateExecutionStatusVariables(ps.Tasks, ps.Finally))
 	// Validate the pipeline's workspaces.
-	errs = errs.Also(validatePipelineWorkspaces(ps.Workspaces, ps.Tasks, ps.Finally))
+	errs = errs.Also(validatePipelineWorkspacesDeclarations(ps.Workspaces))
+	errs = errs.Also(validatePipelineWorkspacesUsage(ps.Workspaces, ps.Tasks).ViaField("tasks"))
+	errs = errs.Also(validatePipelineWorkspacesUsage(ps.Workspaces, ps.Finally).ViaField("finally"))
 	// Validate the pipeline's results
 	errs = errs.Also(validatePipelineResults(ps.Results))
 	errs = errs.Also(validateTasksAndFinallySection(ps))
@@ -86,9 +88,9 @@ func ValidatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks
 	return errs
 }
 
-// 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, finalTasks []PipelineTask) (errs *apis.FieldError) {
+// validatePipelineWorkspacesDeclarations validates the specified workspaces, ensuring having unique name without any
+// empty string,
+func validatePipelineWorkspacesDeclarations(wss []PipelineWorkspaceDeclaration) (errs *apis.FieldError) {
 	// Workspace names must be non-empty and unique.
 	wsTable := sets.NewString()
 	for i, ws := range wss {
@@ -102,28 +104,19 @@ func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []Pipeli
 		}
 		wsTable.Insert(ws.Name)
 	}
+	return errs
+}
 
-	// Any workspaces used in PipelineTasks should have their name declared in the Pipeline's
-	// Workspaces list.
-	for i, pt := range pts {
-		for j, ws := range pt.Workspaces {
-			if !wsTable.Has(ws.Workspace) {
-				errs = errs.Also(apis.ErrInvalidValue(
-					fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", pt.Name, ws.Workspace),
-					"",
-				).ViaFieldIndex("workspaces", j).ViaFieldIndex("tasks", i))
-			}
-		}
+// validatePipelineWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in
+// the pipeline
+func validatePipelineWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) {
+	workspaceNames := sets.NewString()
+	for _, ws := range wss {
+		workspaceNames.Insert(ws.Name)
 	}
-	for i, t := range finalTasks {
-		for j, ws := range t.Workspaces {
-			if !wsTable.Has(ws.Workspace) {
-				errs = errs.Also(apis.ErrInvalidValue(
-					fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", t.Name, ws.Workspace),
-					"",
-				).ViaFieldIndex("workspaces", j).ViaFieldIndex("finally", i))
-			}
-		}
+	// Any workspaces used in PipelineTasks should have their name declared in the Pipeline's Workspaces list.
+	for i, pt := range pts {
+		errs = errs.Also(pt.validateWorkspaces(workspaceNames).ViaIndex(i))
 	}
 	return errs
 }

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
@@ -1610,7 +1610,22 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
 	}
 }
 
-func TestValidatePipelineWorkspaces_Success(t *testing.T) {
+func TestValidatePipelineWorkspacesDeclarations_Success(t *testing.T) {
+	desc := "pipeline spec workspaces do not cause an error"
+	workspaces := []PipelineWorkspaceDeclaration{{
+		Name: "foo",
+	}, {
+		Name: "bar",
+	}}
+	t.Run(desc, func(t *testing.T) {
+		err := validatePipelineWorkspacesDeclarations(workspaces)
+		if err != nil {
+			t.Errorf("Pipeline.validatePipelineWorkspacesDeclarations() returned error for valid pipeline workspaces: %v", err)
+		}
+	})
+}
+
+func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) {
 	desc := "unused pipeline spec workspaces do not cause an error"
 	workspaces := []PipelineWorkspaceDeclaration{{
 		Name: "foo",
@@ -1621,36 +1636,20 @@ func TestValidatePipelineWorkspaces_Success(t *testing.T) {
 		Name: "foo", TaskRef: &TaskRef{Name: "foo"},
 	}}
 	t.Run(desc, func(t *testing.T) {
-		err := validatePipelineWorkspaces(workspaces, tasks, []PipelineTask{})
+		err := validatePipelineWorkspacesUsage(workspaces, tasks)
 		if err != nil {
-			t.Errorf("Pipeline.validatePipelineWorkspaces() returned error for valid pipeline workspaces: %v", err)
+			t.Errorf("Pipeline.validatePipelineWorkspacesUsage() returned error for valid pipeline workspaces: %v", err)
 		}
 	})
 }
 
-func TestValidatePipelineWorkspaces_Failure(t *testing.T) {
+func TestValidatePipelineWorkspacesDeclarations_Failure(t *testing.T) {
 	tests := []struct {
 		name          string
 		workspaces    []PipelineWorkspaceDeclaration
 		tasks         []PipelineTask
 		expectedError apis.FieldError
 	}{{
-		name: "workspace bindings relying on a non-existent pipeline workspace cause an error",
-		workspaces: []PipelineWorkspaceDeclaration{{
-			Name: "foo",
-		}},
-		tasks: []PipelineTask{{
-			Name: "foo", TaskRef: &TaskRef{Name: "foo"},
-			Workspaces: []WorkspacePipelineTaskBinding{{
-				Name:      "taskWorkspaceName",
-				Workspace: "pipelineWorkspaceName",
-			}},
-		}},
-		expectedError: apis.FieldError{
-			Message: `invalid value: pipeline task "foo" expects workspace with name "pipelineWorkspaceName" but none exists in pipeline spec`,
-			Paths:   []string{"tasks[0].workspaces[0]"},
-		},
-	}, {
 		name: "multiple workspaces sharing the same name are not allowed",
 		workspaces: []PipelineWorkspaceDeclaration{{
 			Name: "foo",
@@ -1679,12 +1678,48 @@ func TestValidatePipelineWorkspaces_Failure(t *testing.T) {
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := validatePipelineWorkspaces(tt.workspaces, tt.tasks, []PipelineTask{})
-			if err == nil {
-				t.Errorf("Pipeline.validatePipelineWorkspaces() did not return error for invalid pipeline workspaces")
+			errs := validatePipelineWorkspacesDeclarations(tt.workspaces)
+			if errs == nil {
+				t.Errorf("Pipeline.validatePipelineWorkspacesDeclarations() did not return error for invalid pipeline workspaces")
 			}
-			if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
-				t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d))
+			if d := cmp.Diff(tt.expectedError.Error(), errs.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
+				t.Errorf("PipelineSpec.validatePipelineWorkspacesDeclarations() errors diff %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
+func TestValidatePipelineWorkspacesUsage_Failure(t *testing.T) {
+	tests := []struct {
+		name          string
+		workspaces    []PipelineWorkspaceDeclaration
+		tasks         []PipelineTask
+		expectedError apis.FieldError
+	}{{
+		name: "workspace bindings relying on a non-existent pipeline workspace cause an error",
+		workspaces: []PipelineWorkspaceDeclaration{{
+			Name: "foo",
+		}},
+		tasks: []PipelineTask{{
+			Name: "foo", TaskRef: &TaskRef{Name: "foo"},
+			Workspaces: []WorkspacePipelineTaskBinding{{
+				Name:      "taskWorkspaceName",
+				Workspace: "pipelineWorkspaceName",
+			}},
+		}},
+		expectedError: apis.FieldError{
+			Message: `invalid value: pipeline task "foo" expects workspace with name "pipelineWorkspaceName" but none exists in pipeline spec`,
+			Paths:   []string{"tasks[0].workspaces[0]"},
+		},
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			errs := validatePipelineWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks")
+			if errs == nil {
+				t.Errorf("Pipeline.validatePipelineWorkspacesUsage() did not return error for invalid pipeline workspaces")
+			}
+			if d := cmp.Diff(tt.expectedError.Error(), errs.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
+				t.Errorf("PipelineSpec.validatePipelineWorkspacesUsage() errors diff %s", diff.PrintWantGot(d))
 			}
 		})
 	}