Add docs and validation for providedBy
#342
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
the
approved
knative-prow-robot
added
the
size/L
| } | ||
|
|
||
| sameBindingExists := false | ||
| for _, output := range depTask.ResolvedTaskResources.Outputs { |
There was a problem hiding this comment.
not 1...
not 2...
not THREE
BUT FOUR
layers of for loop nesting
😎
it is a beautiful thing
There was a problem hiding this comment.
😅
In terms on indentantion, we could extract some loop body into functions 👼
There was a problem hiding this comment.
hm we could! but im not sure it would be any clearer - im even thinking that making findReferencedTask a separate function was maybe overkill 🤔 i'm happy to do it tho if you have any ideas - the best I could think of is moving the body of the for _, pb := range dep.ProvidedBy loop into a function but I'm not really sure how much that improves things (and we'd have to add another layer of error handling)
While working on creating an end to end test for tektoncd#168, @dlorenc and I had a very hard time determining why our pipeline wasn't working as expected - it turned out that we were using the wrong resource name in our `providedBy` clauses. While adding this validation I _really_ struggled to understand how the `providedBy` clause actually works. I've updated the docs with my best understanding of how it is currently working. Fortunately in tektoncd#320 we will be simplifying how this works! This is follow-up work for #124
bobcatfish
force-pushed
the
provided_by_validation
branch
from
fb51bc3
to
6a22931
Compare
|
The following is the coverage report on pkg/.
|
| name: skaffold-image-leeroy-app | ||
| - name: deploy-app | ||
| ... | ||
| - name: image |
There was a problem hiding this comment.
Shouldn't it be
- name: deploy-app
...
inputs:
- name: imageThere was a problem hiding this comment.
whoops, yeah that's better!
| } | ||
|
|
||
| sameBindingExists := false | ||
| for _, output := range depTask.ResolvedTaskResources.Outputs { |
There was a problem hiding this comment.
😅
In terms on indentantion, we could extract some loop body into functions 👼
| @@ -561,3 +561,283 @@ func TestGetPipelineConditionStatus(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestValidateProvidedBy(t *testing.T) { | |||
There was a problem hiding this comment.
You could use the test/builder package here 👼 😉
There was a problem hiding this comment.
Looks like I can definitely use the resource builders but I'm not sure I can use the builders to construct the state object - I'll explain more in my follow up PR!
|
/lgtm |
In tektoncd#342 I added docs and validation for the `providedBy` field, but I missed some feedback from @vdemeester before the PR was merged, so I'm addressing it now! The change to use the builders, even just for `PipelineResources`, removes a surprising amount of boilerplate! It doesn't seem like I can really use the builders for constructing the `state` object (slice of `ResolvedPipelineRunTask`) unless we add builder for `ResolvedPipelineRunTask`. That might be a good idea, however I'm planning to iterate on some of this for tektoncd#320 and tektoncd#168 (maybe) so I'm inclined to wait - open to other ideas tho!
In tektoncd#342 I added docs and validation for the `providedBy` field, but I missed some feedback from @vdemeester before the PR was merged, so I'm addressing it now! The change to use the builders, even just for `PipelineResources`, removes a surprising amount of boilerplate! It doesn't seem like I can really use the builders for constructing the `state` object (slice of `ResolvedPipelineRunTask`) unless we add builder for `ResolvedPipelineRunTask`. That might be a good idea, however I'm planning to iterate on some of this for tektoncd#320 and tektoncd#168 (maybe) so I'm inclined to wait - open to other ideas tho!
|
Follow up PR in #365 to address @vdemeester 's feedback! |
In #342 I added docs and validation for the `providedBy` field, but I missed some feedback from @vdemeester before the PR was merged, so I'm addressing it now! The change to use the builders, even just for `PipelineResources`, removes a surprising amount of boilerplate! It doesn't seem like I can really use the builders for constructing the `state` object (slice of `ResolvedPipelineRunTask`) unless we add builder for `ResolvedPipelineRunTask`. That might be a good idea, however I'm planning to iterate on some of this for #320 and #168 (maybe) so I'm inclined to wait - open to other ideas tho!
While working on creating an end to end test for #168, @dlorenc and I
had a very hard time determining why our pipeline wasn't working as
expected - it turned out that we were using the wrong resource name in
our
providedByclauses.While adding this validation I really struggled to understand how the
providedByclause actually works. I've updated the docs with my bestunderstanding of how it is currently working.
Fortunately in #320 we will be simplifying how this works!
This is follow-up work for #124