TaskRun Validation - Controller Side #208

aaron-prindle · 2018-10-31T00:26:05Z

What is the problem being solved?
Fixes #188, TaskRun Validation - Controller Side. This PR adds additional TaskRun validation to the TaskRun controller which checks that referenced Params and Resources exist and that they are bound properly from the Task.

Why is this the best approach?
This approach follows the style and convention for a similar issues, #162, PipelineRun validation.

What other approaches did you consider?
N/A

What side effects will this approach have?
It is possible that some demo/sample yaml which use to execute will no longer work as we have this additional validation.

What future work remains to be done?
For the TaskRun controller, I believe that this validation is sufficient. In the future we should extend validation to all of our primitive types, especially PipelineResource.

tejal29

lgtm except for few small nits.

tejal29 · 2018-10-31T04:42:56Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -294,7 +309,7 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun, pvcName string) (*buildv1
 		},
 	})

-	// Override the entrypoint so that we can use our custom
+	// Oerride the entrypoint so that we can use our custom


nit: override

tejal29 · 2018-10-31T04:43:06Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -319,7 +334,7 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun, pvcName string) (*buildv1
 		Spec: *bSpec,
 	}
 	// Pass service account name from taskrun to build
-	// if task specifies service account name override with taskrun SA
+	// if task specifies service account name oerride with taskrun SA


nit: override

tejal29 · 2018-10-31T04:44:14Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

 		},
 	}

 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
 			c, _, clients := test.GetTaskRunController(d)
+			// c, _, _ := test.GetTaskRunController(d)


nit: remove.

tejal29 · 2018-10-31T04:46:38Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

 			err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.taskRun))
 			// When a TaskRun is invalid and can't run, we don't want to return an error because
 			// an error will tell the Reconciler to keep trying to reconcile; instead we want to stop
 			// and forget about the Run.
 			if err != nil {
 				t.Errorf("Did not expect to see error when reconciling invalid TaskRun but saw %q", err)
 			}
-			if len(clients.Build.Actions()) != 1 {
-				t.Errorf("expected no actions to be created by the reconciler, got %v", clients.Build.Actions())
+			if len(clients.Build.Actions()) != 0 {


nice catch! shdn't have created a build for invalid task.

tejal29 · 2018-10-31T04:47:33Z

pkg/reconciler/v1alpha1/taskrun/validate.go

+		return fmt.Errorf("Error listing task ref %s: %v",
+			tr.Spec.TaskRef.Name, err)
+	}
+	if err := validateTaskRunAndTask(c, *tr, t, tr.Namespace); err != nil {


you can just return validateTaskRunAndTask here.

tejal29 · 2018-10-31T04:48:47Z

pkg/reconciler/v1alpha1/taskrun/validate_test.go

+		},
+		Spec: v1alpha1.TaskRunSpec{
+			TaskRef: v1alpha1.TaskRef{
+				//


nit: remove

tejal29 · 2018-10-31T04:52:45Z

pkg/reconciler/v1alpha1/taskrun/validate_test.go

+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+


for completeness, can we add a test for valid TaskRun and then make sure validateTaskRun does not return err. Ignore if this comment if this is already tested somewhere else.

tejal29 · 2018-10-31T04:54:45Z

pkg/reconciler/v1alpha1/taskrun/validate_test.go

+		}, {
+			name:    "resource-mismatch",
+			taskrun: trs[3],
+			reason:  "input-resource-mismatch",


nit: this reason is not used anywhere. can we drop it?

we can drop it, i think it supposed to be a helpful descriptor for the test. I am copying the test format used in PipelineRun. I will leave this as is for now.

vdemeester

Same nit as @tejal29 otherwise looks good ;)

vdemeester · 2018-10-31T10:01:22Z

pkg/reconciler/v1alpha1/taskrun/validate_test.go

+		},
+		Spec: v1alpha1.TaskRunSpec{
+			TaskRef: v1alpha1.TaskRef{
+				//


nit: remove (aussi)

bobcatfish · 2018-10-31T22:01:56Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

@@ -130,7 +132,6 @@ func (r TaskTriggerRef) Validate(path string) *apis.FieldError {

 		if taskType == allowedType {
 			if allowedType == strings.ToLower(string(TaskTriggerTypePipelineRun)) && r.Name == "" {
-				fmt.Println("HERE")


happens to the best of us :D

bobcatfish · 2018-10-31T22:02:47Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

 func (t *TaskRun) Validate() *apis.FieldError {
 	if err := validateObjectMetadata(t.GetObjectMeta()).ViaField("metadata"); err != nil {
 		return err
 	}
 	return t.Spec.Validate()
 }

+// Validate taskrun spec


i know the linters want this, but if we're going to include a comment here, can we add some more information? maybe one or both of:

Talk about how this is going to be used (called by the controller on creation?)

Talk about what it means for the TaskRun to be valid

going to leave as is for now, will refactor PipelineRun and TaskRun comments when the refactor goes in combining their logic

aaron-prindle · 2018-11-01T19:56:05Z

all requested changes addressed/changed

tejal29

/lgtm

knative-prow-robot · 2018-11-01T19:57:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle, tejal29, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aaron-prindle,tejal29]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

What is the problem being solved? Fixes #188, TaskRun Validation - Controller Side. This PR adds additional TaskRun validation to the TaskRun controller which checks that referenced Params and Resources exist and that they are bound properly from the Task. Why is this the best approach? This approach follows the style and convention for a similar issues, #162, PipelineRun validation. What other approaches did you consider? N/A What side effects will this approach have? It is possible that some demo/sample yaml which use to execute will no longer work as we have this additional validation. What future work remains to be done? For the TaskRun controller, I believe that this validation is sufficient. In the future we should extend validation to all of our primitive types, especially PipelineResource.

knative-metrics-robot · 2018-11-01T20:31:13Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	93.2%	93.1%	-0.1
pkg/reconciler/v1alpha1/taskrun/taskrun.go	76.4%	72.7%	-3.7
pkg/reconciler/v1alpha1/taskrun/validate.go	Do not exist	92.9%

tejal29 · 2018-11-01T20:52:41Z

/lgtm

knative-prow-robot requested review from imjasonh and tejal29 October 31, 2018 00:26

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 31, 2018

aaron-prindle changed the title ~~What is the problem being solved?~~ TaskRun Validation - Controller Side Oct 31, 2018

tejal29 reviewed Oct 31, 2018

View reviewed changes

vdemeester approved these changes Oct 31, 2018

View reviewed changes

bobcatfish reviewed Oct 31, 2018

View reviewed changes

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2018

aaron-prindle mentioned this pull request Nov 1, 2018

PipelineRun Controller Validation - Output Binding Issue #213

Closed

tejal29 approved these changes Nov 1, 2018

View reviewed changes

knative-prow-robot assigned tejal29 Nov 1, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018

knative-prow-robot merged commit 9ae0688 into tektoncd:master Nov 1, 2018

mchmarny unassigned tejal29 Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TaskRun Validation - Controller Side #208

TaskRun Validation - Controller Side #208

aaron-prindle commented Oct 31, 2018 •

edited

Loading

tejal29 left a comment

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited

Loading

aaron-prindle Nov 1, 2018

vdemeester left a comment

vdemeester Oct 31, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 31, 2018

bobcatfish Oct 31, 2018

aaron-prindle Nov 1, 2018

aaron-prindle commented Nov 1, 2018

tejal29 left a comment

knative-prow-robot commented Nov 1, 2018

knative-metrics-robot commented Nov 1, 2018

tejal29 commented Nov 1, 2018

TaskRun Validation - Controller Side #208

TaskRun Validation - Controller Side #208

Conversation

aaron-prindle commented Oct 31, 2018 • edited Loading

tejal29 left a comment

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 31, 2018

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

aaron-prindle Nov 1, 2018

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Oct 31, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

bobcatfish Oct 31, 2018

Choose a reason for hiding this comment

bobcatfish Oct 31, 2018

Choose a reason for hiding this comment

aaron-prindle Nov 1, 2018

Choose a reason for hiding this comment

aaron-prindle commented Nov 1, 2018

tejal29 left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 1, 2018

knative-metrics-robot commented Nov 1, 2018

tejal29 commented Nov 1, 2018

aaron-prindle commented Oct 31, 2018 •

edited

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 31, 2018 •

edited

Loading

vdemeester Oct 31, 2018 •

edited by aaron-prindle

Loading