Nr. 2 - [TEP-0056]: Update state methods to check for child PipelineRuns.
#8878
Conversation
tekton-robot
added
do-not-merge/work-in-progress
tekton-robot
added
the
size/XXL
|
The following is the coverage report on the affected files.
|
twoGiants
force-pushed
the
issue-8760-tep-0056-pinp-state
branch
from
d4eaab4 to
68d6528
Compare
tekton-robot
added
size/XL
|
/kind feature |
tekton-robot
removed
the
do-not-merge/work-in-progress
[TEP-0056]: Second PR of Pipelines-in-Pipelines feature implementation. Extend existing methods in `pipelinerunstate.go` to check for child `PipelirRuns` and add new test cases. Issues tektoncd#8760, tektoncd#7166. Signed-off-by: Stanislav Jakuschevskij <[email protected]>
twoGiants
force-pushed
the
issue-8760-tep-0056-pinp-state
branch
from
68d6528 to
a3ba4de
Compare
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
tekton-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
/meow |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| // if any of the dag pipeline tasks failed, change the aggregate status to failed and return | ||
| if !t.IsCustomTask() && t.haveAnyTaskRunsFailed() || | ||
| t.IsCustomTask() && t.haveAnyCustomRunsFailed() || | ||
| t.IsChildPipeline() && t.haveAnyChildPipelineRunsFailed() { |
There was a problem hiding this comment.
Since this is not binary anymore, I'm not sure that !t.IsCustomTask() is the best check anymore.
Should we use something like:
if t.IsTask() && t.haveAnyTaskRunsFailed() ||
t.IsCustomTask() && t.haveAnyCustomRunsFailed() ||
t.IsChildPipeline() && t.haveAnyChildPipelineRunsFailed() {
I guess IsTask may not exist...
There was a problem hiding this comment.
Yeah, it's painfull to look at it 🤕 🤣 . This is a point where that polymorphism refactoring would do its magic 😸.
I will add a IsTask a method and update it in a few places. That would be cleaner anyway.
There was a problem hiding this comment.
@afrittoli I have updated the condition. Please take another look. IsTask was not necessary.
The previous condition had a small harmless bug. In case the PipelineTask was a child Pipeline the condition would first evaluate !t.IsCustomTask() to true and move on to the next check t.haveAnyTaskRunsFailed() and evaluate that. But because our PipelineTask is a child Pipeline we don't want t.haveAnyTaskRunsFailed() to be evaluated. This was an unnecessary check.
Luckily it had no further implications because it returned false and the condition would eventually check t.IsChildPipeline() and do the check we want. Nevertheless, changing the order and decomposing the
condition makes the flow clearer.
I wanted to add a test first to catch this case but it makes no sense because it "should" ( 😁 🙈 ) be impossible to for a PipelineTask to be a child Pipeline and have any TaskRuns. This "should" ( 😁 🙈 ) never be the case.
Wdys?
P.S.: I'll squash if you approve.
The types of a `PipelineTask` are not binary anymore. We now have three: `Pipeline`, `Task`, `CustomTask`. The previous condition had a bug. In case the `PipelineTask` was a child `Pipeline` the condition would first evaluate `!t.IsCustomTask()` to true and move on to the next check `t.haveAnyTaskRunsFailed()` and evaluate that. But because our `PipelineTask` is a child `Pipeline` we don't want `t.haveAnyTaskRunsFailed()` to be evaluated. This was an unnecessary check. Luckily it had no further implications because it returned `false` and the condition would eventually check `t.IsChildPipeline()` and do the check we want. Nevertheless, changing the order and decomposing the condition makes the flow clearer. Additionally "(PinP)" was removed from comments and a few typos fixed. Signed-off-by: Stanislav Jakuschevskij <[email protected]>
5 participants
Important
Pull request number 2.
The numbering
Nr. 2 - [TEP-0056]means the PRs must be merged in order. They build on each other.Changes
[TEP-0056]: Second PR of Pipelines-in-Pipelines feature implementation.
Extend existing methods in
pipelinerunstate.goto check for childPipelirRunsand add new test cases.Issue #8760, #7166.
Release notes will be added with the last PR which will make this feature functional for users.
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes