Taskrun CRD validation #130

shashwathi · 2018-10-11T19:02:26Z

Fixes #33

Added taskrun CRD validation
Added tests for metadata validation

shashwathi · 2018-10-11T19:30:01Z

/assign @pivotal-nader-ziada
/assign @bobcatfish

nader-ziada · 2018-10-11T19:46:45Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

+					}},
+				},
+			},
+			// TODO(shashwathi): wrong error msg


can we fix the error msg?

Damn I was suppose to fix it before sending PR :D

dlorenc · 2018-10-11T19:55:09Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+	}
+
+	// check for input resources
+	if err := checkForipelineResourceDuplicates(ts.Inputs.Resources, "spec.Inputs.Resources.Name"); err != nil {


Looks like there's a typo here, the P in Pipeline is missing

shashwathi · 2018-10-11T21:32:42Z

@pivotal-nader-ziada @dlorenc : addressed your comments

bobcatfish

Thanks for adding this @shashwathi ! Very thorough validation! :D (and testing!!) I just have a bunch of minor comments

/meow space

bobcatfish · 2018-10-11T21:11:57Z

pkg/apis/pipeline/v1alpha1/metadata_validation_test.go

+			t.Errorf("Failed to validate object meta data: %s", err)
+		}
+	}
+}


wooot more coverage :D

bobcatfish · 2018-10-11T21:12:32Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

+var _ apis.Validatable = (*TaskRun)(nil)
+var _ apis.Defaultable = (*TaskRun)(nil)
+
+// Assert that Task implements the GenericCRD interface.


i think the comments here should say TaskRun (or just "it" so we dont have to update it each time we copy it XD)

bobcatfish · 2018-10-11T21:16:22Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+	var emptyTarget = ResultTarget{}
+	if r == emptyTarget {
+		return nil
+	}


My comment is completely outside the scope of your work so feel free to ignore me but i think maybe the results section should move into status.

In 9031ed4#diff-b3fd75c449b28741c42f5ff5acc116ac I got a bit confused and removed it from the examples, since we are now requiring a reference to PipelineParams - but since the logs will be written to some subdir at those endpoints (probably) it's still useful to include them, however maybe the controller can decide the final location and report it in the status

I should probably just create an issue for this 🤔

Yeah lets make a separate issue for that 👍

kk created #146 sorry for ranting here about it XD

bobcatfish · 2018-10-11T21:21:57Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+	}
+	// If set then verify all variables pass the validation
+	if r.Name == "" {
+		return apis.ErrMissingField(fmt.Sprintf("%s.name", path))


im not sure what i think about checking that any field that is required and is a string isn't empty 🤔 one alternative would be to allow the usages to fail instead, seems a bit tedious to do this for every string field

on the other hand maybe it's always better to fail earlier?

That is the purpose for webhook validation, fail early to provide fast feedback about the object field validation.

bobcatfish · 2018-10-11T21:23:28Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+		// Check the unique combination of resource+version. Covers the use case of inputs with same resource name
+		// and different versions
+		key := fmt.Sprintf("%s%s", r.ResourceRef.Name, r.Version)
+		if _, ok := encountered[strings.ToLower(key)]; ok {


https://github.com/knative/build-pipeline/blob/master/pkg/apis/pipeline/v1alpha1/task_validation.go#L72 should probably be case insensitive also 🤔

bobcatfish · 2018-10-11T21:27:14Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

+	tests := []struct {
+		name    string
+		spec    *TaskRunSpec
+		wantErr *apis.FieldError


same thing, maybe a success case too?

bobcatfish · 2018-10-11T21:29:36Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

+						Type: TaskTriggerTypePipelineRun,
+					},
+				},
+			},


id rather have a separate test function completely for the success cases, i think it makes the tests much easier to understand (like I made @dlorenc do in https://github.com/knative/build-pipeline/blob/master/pkg/apis/pipeline/v1alpha1/task_validation_test.go)

first reading this i actually didnt think there were any success cases

Yeah that makes sense. I have updated tests to have clear distinction between valid and invalid test cases.

sounds good, thanks @shashwathi !

bobcatfish · 2018-10-11T21:30:05Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

+				},
+				Trigger: TaskTrigger{
+					TriggerRef: TaskTriggerRef{
+						Type: TaskTriggerTypePipelineRun,


if the type is pipelinerun there should be a name field as well with the name of the pipelinerun

bobcatfish · 2018-10-11T21:32:03Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

+			wantErr: apis.ErrMissingField("spec.results.runs.URL"),
+		},
+		{
+			name: "valid task type result",


one idea to reduced the boilerplate in the testcases would be to test validation of the individual types in separate tests, e.g. have a set of tests just for Results, one just for Inputs, one just for Params, etc., then instead of having to instantiate an entire TaskRun each time, you could just instantiate the section you need to validate

I did think about that but I am generally not a fan of testing private functions. Probably I could make Validate functions on each of the types and separate the cases. That might be lot easier to read.

bobcatfish · 2018-10-11T21:34:40Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+	// Check for TaskRef
+	if ts.TaskRef.Name == "" {
+		return apis.ErrMissingField("spec.taskref.name")
+	}


should we also validate that the referenced Task exists?

For that kind of validation, pipeline client is required. In depth validation of related object existence could be done in taskrun controller.

ah interesting, okay good to know!

knative-prow-robot · 2018-10-11T21:35:16Z

@bobcatfish:

In response to this:

Thanks for adding this @shashwathi ! Very thorough validation! :D (and testing!!) I just have a bunch of minor comments

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nader-ziada · 2018-10-12T14:35:03Z

/approve

shashwathi · 2018-10-12T16:53:42Z

@bobcatfish : Addressed your comments. Let me know if there is anything else

tejal29

/approve

knative-prow-robot · 2018-10-12T21:35:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, shashwathi, tejal29

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~~ [pivotal-nader-ziada,tejal29]

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

bobcatfish · 2018-10-12T21:47:47Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

+			}
+		})
+	}
+}


<3 the tests!!

bobcatfish · 2018-10-12T21:48:34Z

I am happy to /lgtm - i think there's a conflict that has to get resolved first tho 😩

- Add tests for metadata validation - Add tests for taskrun validation CRD

- Applying any taskrun object will invoke the validate function

- Fix typo - Remove redundant field in kritis.yaml that did not comply with task run validation - Fix TODO in test

- TaskRun.trigger.Type is accepted with any capitalization

- Split taskrunspec validation into smaller validation for easy testing and readability - Update task validation to be consistent - Update tests to test individual objects under taskrun

shashwathi · 2018-10-13T02:18:19Z

@bobcatfish : Resolved the conflicts.

nader-ziada · 2018-10-13T02:36:17Z

@bobcatfish I'm going to lgtm it since you said you are all good with it and now the conflicts are resolved

/lgtm

knative-prow-robot requested review from aaron-prindle and dlorenc October 11, 2018 19:02

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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 11, 2018

knative-prow-robot assigned bobcatfish and nader-ziada Oct 11, 2018

nader-ziada reviewed Oct 11, 2018

View reviewed changes

dlorenc reviewed Oct 11, 2018

View reviewed changes

bobcatfish reviewed Oct 11, 2018

View reviewed changes

tejal29 approved these changes Oct 12, 2018

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2018

bobcatfish reviewed Oct 12, 2018

View reviewed changes

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

}

})

}

}

Copy link

Collaborator

bobcatfish Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3 the tests!!

Shash Reddy added 2 commits October 12, 2018 21:37

Add validation for taskrun CRD

c04f6b4

- Add tests for metadata validation - Add tests for taskrun validation CRD

Add better test diff msg

7f88925

shashwathi force-pushed the task-validation branch from af5e1e8 to 90cec7b Compare October 13, 2018 01:38

Shash Reddy and others added 6 commits October 12, 2018 21:38

Update webhook to include TaskRun

6eab668

- Applying any taskrun object will invoke the validate function

Add generation to avoid webhook crashloop

97f69cf

Address comments

53f3b6b

- Fix typo - Remove redundant field in kritis.yaml that did not comply with task run validation - Fix TODO in test

Remove TODO

5fe48e5

Validations are more robust for different capitalization

ea3c30d

- TaskRun.trigger.Type is accepted with any capitalization

Granular validation for taskrun objects

90cec7b

- Split taskrunspec validation into smaller validation for easy testing and readability - Update task validation to be consistent - Update tests to test individual objects under taskrun

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

knative-prow-robot merged commit 4999a65 into tektoncd:master Oct 13, 2018

mchmarny unassigned nader-ziada Mar 7, 2019

Taskrun CRD validation #130

Taskrun CRD validation #130

Conversation

shashwathi commented Oct 11, 2018 • edited Loading

shashwathi commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi commented Oct 11, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Oct 11, 2018

nader-ziada commented Oct 12, 2018 • edited Loading

shashwathi commented Oct 12, 2018

tejal29 left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Oct 12, 2018

Choose a reason for hiding this comment

bobcatfish commented Oct 12, 2018

shashwathi commented Oct 13, 2018

nader-ziada commented Oct 13, 2018

shashwathi commented Oct 11, 2018 •

edited

Loading

bobcatfish Oct 11, 2018 •

edited

Loading

nader-ziada commented Oct 12, 2018 •

edited

Loading