refactor GetPipelineConditionStatus - drop unexported function isdagstopped #2869
Conversation
tekton-robot
added
the
size/L
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
1 similar comment
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
pritidesai
changed the title
|
/kind cleanup |
tekton-robot
added
kind/misc
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
pritidesai
force-pushed
the
drop-isdagstopped
branch
from
e509170
to
522c81f
Compare
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
/remove-kind misc |
tekton-robot
removed
the
kind/misc
pritidesai
force-pushed
the
drop-isdagstopped
branch
from
522c81f
to
3d3f5f4
Compare
|
The following is the coverage report on the affected files.
|
pritidesai
force-pushed
the
drop-isdagstopped
branch
from
3d3f5f4
to
4e797da
Compare
|
The following is the coverage report on the affected files.
|
pritidesai
force-pushed
the
drop-isdagstopped
branch
from
4e797da
to
5c01c94
Compare
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Thanks for this.
I think this change might be rendered obsolete by #2821, but I don't mind merging it since the other PR is much larger and we still have to discuss on it.
The PR shows two commits, one of which is merged already.
I wonder if GitHub now is able to understand stacked PRs like this or if it will try to merge the first commit again? That's the only thing to clarify from my POV, else I'd approve.
/hold
tekton-robot
added
the
do-not-merge/hold
|
Oh, it looks like there is a conflict now, possibly with your other PR that just merged. |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
Adding an attribute to PipelineRunState to tell whether a PipelineTask will not be run because (1) its condition checks failed or (2) one of the parents conditions failed or (3) pipeline is in stopping state Instead of explicitly checking (3) in GetPipelineConditionStatus, moved this check to the IsSkipped function which is used by both the scheduler and while determining pipeline status.
pritidesai
force-pushed
the
drop-isdagstopped
branch
from
5c01c94
to
2f8539c
Compare
|
thanks @afrittoli I have renamed the variable and squashed two commits into one, PTAL 🙏 |
tekton-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, 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 |
Changes
This is a minor refactoring of
GetPipelineConditionStatus.Adding an attribute (
IsSkipped) toPipelineRunStateto tell whether a PipelineTask will not be run because:(1) its condition checks failed or
(2) one of the parents conditions failed or
(3) pipeline is in stopping state
Instead of explicitly checking (3) in
GetPipelineConditionStatus, moved this check to theIsSkippedfunction which is used by both the scheduler and while determining pipeline status.Based on the discussion here.
One additional refactoring by adding a local function to check if a task exists in Graph and use that helper function instead of:
/cc @bobcatfish @afrittoli
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.