Use the same resource validation logic for both pipelineruns + taskruns #294

bobcatfish · 2018-12-03T04:13:06Z

Now when resolving a PipelineRun, we will resolve all of the resource references as well. To do this, we make it so that PipelineRuns and TaskRuns use the same types to bind resources.

Then we use exactly the same logic that was used to validate taskrun resources to validate pipelinerun resources.

The structure that contains the "resolved" pipelinerun can probably be iterated on (some of the tests only need a subset of it, which is a hint that we probably shouldnt pass the whole thing around everywhere) but this should help with #168 b/c to implement a DAG we should just need to put the resolved objects into a DAG instead of a list.

Bonuses:

pipelineruns will now respect tasks that have defaults for their resources
users can provide resource paths in pipelineruns if they want to (which before only worked for taskruns)

Fixes #213

There is some future work we can do to use these resolved objects in the rest of the reconcilers as well (e.g. when applying templating) but I think we can iterate on it!

knative-prow-robot · 2018-12-03T04:13:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

bobcatfish · 2018-12-03T18:23:21Z

I1203 04:39:10.739] --- FAIL: TestPipelineRun (0.00s)
I1203 04:39:10.739]     --- FAIL: TestPipelineRun/service_account_propagation (600.23s)
I1203 04:39:10.740]     	pipelinerun_test.go:157: Error waiting for PipelineRun helloworld-pipelinerun1 to finish: timed out waiting for the condition
I1203 04:39:10.740]     --- FAIL: TestPipelineRun/fan-in_and_fan-out (600.23s)
I1203 04:39:10.741]     	pipelinerun_test.go:157: Error waiting for PipelineRun helloworld-pipelinerun1 to finish: timed out waiting for the condition

ruh roh

nader-ziada

neat PR 😎
I have some minor comments/questions, otherwise very cool, Thanks

nader-ziada · 2018-12-03T19:26:47Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

+ }
+ err = resources.ResolveTaskRuns(c.taskRunLister.TaskRuns(pr.Namespace).Get, pipelineState)
+ if err != nil {
+ return fmt.Errorf("error getting TaskRunss for Pipeline %s: %s", p.Name, err)


typo "TaskRunss"

whoops thanks 😅

(it's PipelineParamses all over again XD)

nader-ziada · 2018-12-03T19:28:53Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

+ return nil
+ }
+ }
+ err = resources.ResolveTaskRuns(c.taskRunLister.TaskRuns(pr.Namespace).Get, pipelineState)


I feel this func is not really doing much, but just here for the test!

I felt kind of weird about it too! This logic used to be inside of ResolvePipelineRun - I think it makes sense to break it out but I'm not sure happy with how it looks right now :S Do you have any other ideas?

I'm thinking we might want to create a type that hold the PipelineState and have functions like this operate on it maybe :J that's the only idea I have right now

Having some internal types for ResolvedTasks would be useful

we can iterate on that one, but lets merge it for now

nader-ziada · 2018-12-03T19:31:45Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -194,8 +205,8 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error
 return nil
 }

- if err := ValidateTaskRunAndTask(tr.Spec.Inputs.Params, rtr); err != nil {
- c.Logger.Error("Failed to validate taskrun %s with error %v", tr.Name, err)
+ if err := ValidateResolvedTaskResources(tr.Spec.Inputs.Params, rtr); err != nil {


can validateResolvedTaskResources be part of ResolveTaskResources? we don't want to get back invalid resources

I really liked keeping these separate tbh! The logic required to follow all the references and find the objects I think is pretty different from the logic required to make sure all the required resources + params are present.

And we can have two different errors, one where you referred to something that couldn't be retrieved, and one where you didn't provide something that was needed

fair point about different errors, we can always optimize later if we find out we need to

bobcatfish · 2018-12-04T00:50:54Z

There was a bug that the end to end tests caught! :D

Ready for another look @pivotal-nader-ziada

This is so that the PipelineRun reconciler can use the same resource resolution. When the PipelineRun resolves resources, there are not yet any TaskRuns to pass along, those get created later. Seems a bit weird that the resolution function still needs to be passed the taskspec + name at all really, but we'll see how this goes! This is part of tektoncd#213

Now when resolving a PipelineRun, we will resolve all of the resource references as well. To do this, we make it so that PipelineRuns and TaskRuns use the same types to bind resources. This has the bonus of allowing users to provide resource paths in pipelineruns if they wanted to. This will allow us to use the same logic to validate resource bindings for both pipelineruns and taskruns (for tektoncd#213).

The logic was called task run resolution b/c it was originally only used by the taskrun reconciler, but now that we've expanded it to be used by the pipeline run reconciler, it turns out that it has nothing to do with task runs (and in the pipeline run case, at the time when we call it, the task runs may not even exist)

This way we can test the validation without needing an entire reconciler, and we can also eventually call the same resource validation logic used by TaskRuns (tektoncd#213)

Now the pipelinerun reconciler can call the existing task resource validation and not need to duplciate any of it. Bonus: pipelineruns will now respect tasks that have defaults for their resources! Fixes tektoncd#213

This was actually panicing b/c the ResolvedTaskResources was `nil` :O Caught by an end to end test!

knative-metrics-robot · 2018-12-04T01:25:57Z

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/reconciler/v1alpha1/pipelinerun/pipelinerun.go	77.2%	77.9%	0.7
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	Do not exist	91.0%
pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/taskrun/resources/taskspec.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/taskrun/taskrun.go	72.2%	71.1%	-1.1

nader-ziada · 2018-12-04T01:28:17Z

/lgtm

knative-prow-robot requested review from shashwathi and steuhs December 3, 2018 04:13

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 3, 2018

nader-ziada reviewed Dec 3, 2018

View reviewed changes

bobcatfish added 7 commits December 3, 2018 17:20

Validate PipelineRuns without reconciler

e3b6820

This way we can test the validation without needing an entire reconciler, and we can also eventually call the same resource validation logic used by TaskRuns (tektoncd#213)

Remove pipeline & service account retreival from pipelinerun validation

eddcc5a

Use existing task resource validation

d3aa742

Now the pipelinerun reconciler can call the existing task resource validation and not need to duplciate any of it. Bonus: pipelineruns will now respect tasks that have defaults for their resources! Fixes tektoncd#213

Handle case where no resources are bound

bb1bf67

This was actually panicing b/c the ResolvedTaskResources was `nil` :O Caught by an end to end test!

bobcatfish force-pushed the resource_validation_synergy branch from 6c5c3b7 to bb1bf67 Compare December 4, 2018 01:24

knative-prow-robot assigned nader-ziada Dec 4, 2018

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

knative-prow-robot merged commit f70944e into tektoncd:master Dec 4, 2018

mchmarny unassigned nader-ziada Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same resource validation logic for both pipelineruns + taskruns #294

Use the same resource validation logic for both pipelineruns + taskruns #294

bobcatfish commented Dec 3, 2018

knative-prow-robot commented Dec 3, 2018

bobcatfish commented Dec 3, 2018

nader-ziada left a comment

nader-ziada Dec 3, 2018

bobcatfish Dec 3, 2018

nader-ziada Dec 3, 2018

bobcatfish Dec 3, 2018

nader-ziada Dec 4, 2018

nader-ziada Dec 4, 2018

nader-ziada Dec 3, 2018

bobcatfish Dec 3, 2018

nader-ziada Dec 4, 2018

bobcatfish commented Dec 4, 2018

knative-metrics-robot commented Dec 4, 2018

nader-ziada commented Dec 4, 2018

Use the same resource validation logic for both pipelineruns + taskruns #294

Use the same resource validation logic for both pipelineruns + taskruns #294

Conversation

bobcatfish commented Dec 3, 2018

knative-prow-robot commented Dec 3, 2018

bobcatfish commented Dec 3, 2018

nader-ziada left a 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

bobcatfish commented Dec 4, 2018

knative-metrics-robot commented Dec 4, 2018

nader-ziada commented Dec 4, 2018