Separate logic to follow references from TaskRun resource validation #261
Conversation
knative-prow-robot
added
the
size/XL
|
[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
the
approved
| // ResolveTaskRun looks up CRDs referenced by the TaskRun and returns an instance | ||
| // of TaskRunResource with all of the relevant data populated. If referenced CRDs can't | ||
| // be found, an error is returned. | ||
| func ResolveTaskRun(tr *v1alpha1.TaskRunSpec, gt GetTask, gr GetResource) (*ResolvedTaskRun, error) { |
There was a problem hiding this comment.
nit: any reason to use such short variable names ? Using getTask, getResource would make the following code a bit easier to read 👼
There was a problem hiding this comment.
sure I'll change it!
I'm always torn between whether to use shorter or longer names - I feel like Go code in general seems to prefer shorter names, but it looks like I did it at the expense of readability here :D
There was a problem hiding this comment.
@bobcatfish I tend to use https://github.com/golang/go/wiki/CodeReviewComments as a guideline, adding to this a "try to not have too close variable names" 👼 😝
The basic rule: the further from its declaration that a name is used, the more descriptive the name must be. For a method receiver, one or two letters is sufficient. Common variables such as loop indices and readers can be a single letter (i, r). More unusual things and global variables need more descriptive names.
There was a problem hiding this comment.
oh nice, that's great advice!! thanks for the link :D
| // ReasonFailedValidation indicated that the reason for failure status is | ||
| // that pipelinerun failed runtime validation | ||
| // that taskrun failed runtime validation |
In the TaskRun reconciler, there are several different places where logic exists to retrieve references Tasks and Resources; this library will allow us to retrieve these references up front and use them instead of using Listers everywhere. This is a step toward fixing tektoncd#213 (the goal is to reuse and simplify this logic).
With this change, we can use the resolved TaskRun when validating the TaskRun, so we no longer need access to the reconciler or to do any listing when validating the TaskRun. This is a step along the way to tektoncd#213. Next we can do a similar refactoring to the PipelineRun validation. (The goal is that we can share the validation logic, since it's looking at the same thing just the structure of the input objects is different.) We'll probably also separate the param validation out from the resource validation. The current validation will miss the case where extra resources or parameters that aren't needed are supplied (this was the case already).
bobcatfish
force-pushed
the
refactor_resource_validation
branch
from
50dcd0b to
711668f
Compare
|
The following is the coverage report on pkg/.
|
|
/lgtm |
Add 0.9.0 and 0.9.1
In the TaskRun reconciler, there were several different places where logic exists to retrieve references Tasks and Resources; now we use a library that allows us to retrieve these references up front and use them instead of using Listers everywhere.
We now use the resolved TaskRun when validating the TaskRun, so we no longer need access to the reconciler or to do any listing when validating the TaskRun.
This is a step along the way to #213. Next we can do a similar refactoring to the PipelineRun validation. (The goal is that we can share the validation logic, since it's looking at the same thing just the structure of the input objects is different.) We'll probably also separate the param validation out from the resource validation.
The current validation will miss the case where extra resources or parameters that aren't needed are supplied (this was the case already).