Use the same resource validation logic for both pipelineruns + taskruns #294
Conversation
|
[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:
Approvers can indicate their approval by writing |
knative-prow-robot
added
size/XXL
ruh roh |
| } | ||
| 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) |
There was a problem hiding this comment.
whoops thanks 😅
(it's PipelineParamses all over again XD)
| return nil | ||
| } | ||
| } | ||
| err = resources.ResolveTaskRuns(c.taskRunLister.TaskRuns(pr.Namespace).Get, pipelineState) |
There was a problem hiding this comment.
I feel this func is not really doing much, but just here for the test!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Having some internal types for ResolvedTasks would be useful
There was a problem hiding this comment.
we can iterate on that one, but lets merge it for now
| @@ -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 { | |||
There was a problem hiding this comment.
can validateResolvedTaskResources be part of ResolveTaskResources? we don't want to get back invalid resources
There was a problem hiding this comment.
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
There was a problem hiding this comment.
fair point about different errors, we can always optimize later if we find out we need to
|
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!
bobcatfish
force-pushed
the
resource_validation_synergy
branch
from
6c5c3b7 to
bb1bf67
Compare
|
The following is the coverage report on pkg/.
|
|
/lgtm |
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:
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!