retrieving deps outside of dag.build #3583
Conversation
Adding an attribute function to PipelineTaskList to retrieve deps (thanks @bobcatfish for great suggestion), passing this deps to Build function instead of retrieving it from within that function. This change was motivated while introducing task result consumption in finally tasks. Finally tasks can consume results from dag tasks but dag tasks are not part of the same graph and must not be added as part of deps of any finally tasks. This change allows reconciler (with the knowldege of dag tasks vs finally tasks) to manage deps while building a graph.
|
The following is the coverage report on the affected files.
|
| @@ -67,17 +67,16 @@ func (g *Graph) addPipelineTask(t Task) (*Node, error) { | |||
| } | |||
|
|
|||
| // Build returns a valid pipeline Graph. Returns error if the pipeline is invalid | |||
| func Build(tasks Tasks) (*Graph, error) { | |||
| func Build(tasks Tasks, deps map[string][]string) (*Graph, error) { | |||
There was a problem hiding this comment.
Any reason to pass deps there as it comes from tasks ? we could just do
func Build(tasks Tasks) (*Graph, error) {
deps = tasks.Deps()
}There was a problem hiding this comment.
@vdemeester that's what it currently does - maybe @pritidesai can add more info in the commit message to explain but the reason for this change is to be able to call Build with an empty dep list
in the case of finally tasks, we create 2 DAGs. when finally tasks depend on results from non-finally tasks, Deps() will return the non-finally Tasks (or will it error? im not sure). but in that case we don't want to build connections between the finally tasks and the non-finally tasks
making this change enables us to build a graph in both cases
another option might be to stop treating finally tasks as a graph but i have a strong feeling we will be adding more graph like features in future so im not sure we should do that now
There was a problem hiding this comment.
oh actually @pritidesai 's commit message does already explain this pretty well i think: 9716d32
There was a problem hiding this comment.
oh I did not read the commit 😅
|
nice!! one request, @pritidesai could you include some unit tests for the new function? /approve |
|
[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 |
|
/lgtm |
yup, creating a separate PR. |
Please see PR #3597 |
|
hey @vdemeester just a comment that it would have been better to include the unit tests in the same PR (I think including tests and docs in PRs has been a great strategy for us; i want to make sure we don't end up having to play catch up with changes) - maybe it would be been clearer if i'd included a |
|
a |
I think its my mistake, should have added unit tests in this PR instead of leaving it up to the reviewer. I was thinking the graphs we are testing in |
|
No worries @pritidesai !! :) ❤️ |
Changes
Adding an attribute function to
PipelineTaskListto retrievedeps(thanks @bobcatfish for great suggestion), passing thisdepstodag.Buildfunction instead of retrieving deps from within that function.This change was motivated while introducing task result consumption in finally tasks. Finally tasks can consume results from dag tasks but dag tasks are not part of the same graph/section and must not be added as part of deps of any finally tasks. This change allows reconciler (with the knowledge of dag tasks vs finally tasks) to manage deps while building a graph.
/kind misc
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes