[TEP-0100] Update to handle PipelineTask condition checks and when expressions #645

abayer · 2022-03-02T15:20:59Z

With the current, full embedded status for TaskRuns and Runs, we include ConditionChecks and WhenExpressions fields as well as the TaskRun/Run's status. The minimal embedded status approach as initially written does fine for anything that's available within the TaskRun/Run statuses, but ConditionChecks and WhenExpressions are not actually known by the TaskRun/Run. They're only known at the full PipelineRun level. In order to make that information available for reporting, etc in the new minimal embedded status approach, we will need to store ConditionChecks and WhenExpressions along with the kind/version/TaskRun or Run name/PipelineTask name.

cc @jerop @lbernick - not sure who else should review this?

Signed-off-by: Andrew Bayer [email protected]

lbernick · 2022-03-03T15:33:16Z

teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md

+}
+```
+
+Additionally, we will add a new struct `PipelineRunChildReferenceConditionCheckStatus`


I'm a bit confused by this, can't we reuse the existing PipelineRunConditionCheckStatus struct?

Because PipelineRunConditionCheckStatus doesn't have the ConditionCheckName as a field - it's currently just used in a map of ConditionCheckName to PipelineRunConditionCheckStatus, so we need to add a field to it, basically, unless we wanted to use a map instead of an array.

Aaaah - we could just use PipelineRunConditionCheckStatus and add a new ConditionCheckName field to it, leaving it as the empty string when it's being used in the old fully-embedded form.

It already has ConditionName, is that different from ConditionCheckName?

It is! ConditionCheckName is to a PipelineTask name as ConditionName is to a Task name, basically.

got it-- I think I'd prefer something like:

type PipelineRunChildConditionCheckStatus { ConditionCheckName string PipelineRunConditionCheckStatus }

with ChildStatus:

type ChildStatus struct { ... ConditionChecks []PipelineRunChildConditionCheckStatus }

and a note that the ConditionChecks field will be removed when support for Conditions is removed.

Ok, that was what I was doing initially. =)

lbernick · 2022-03-03T15:43:10Z

teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md

@@ -221,13 +221,28 @@ identify related objects.

 #### 1. Add Minimal Embedded Status

-We will introduce a new struct to hold references to child `TaskRuns` and `Runs`:
+We will introduce a new struct to hold references to child `TaskRuns` and `Runs`, and
+their corresponding `WhenExpressions` and `ConditionChecks`:


a few quick comments:

could you please make it clear that whenexpressions and conditionchecks are already present in PipelineRunTaskRunStatus?

Runs don't currently have conditionChecks; I'm guessing this is because PipelineRunRunStatus was created after conditions were deprecated. Because conditions are deprecated, we may be able to remove conditionchecks from childReference if we do it in the correct order. see Deprecate Conditions CRD pipeline#3377.

with the inclusion of "whenexpressions" the childReference is back to being a "childStatus".

1: Will do!

2: Yeah, that's what I assumed was the case. I guess we could say "if you're using minimal for embedded-status, you shouldn't be using conditions, since that's going away before full embedded-status goes away"?

3: Yeah, in my work-in-progress, I'm actually calling the struct ChildStatusReference - it's got some status in it alongside the reference.

sounds good! I think we might have to include conditionChecks in the minimal status for v1beta1 only, making a note that they will be removed when support for Conditions is removed. This can just be empty for runs.

Oh, and in my work-in-progress branch (which I'll be opening as a WIP implementation PR to pipeline after this PR gets merged), it's already not doing anything with Conditions for Runs. I'll mark the field as deprecated as well.

Aaaaand PR is updated!

lbernick · 2022-03-03T16:41:18Z

teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md

+
+```go
+type PipelineRunChildConditionCheckStatus struct {
+	*PipelineRunConditionCheckStatus        `json:",inline"` // the inlined condition check status


nit: I don't think this needs to be a pointer. also please link to the issue on deprecating conditions

aaaaah - ok, since PipelineRunConditionCheckStatus was always used as a pointer, I just assumed I should keep doing that here. =) Easy enough to fix!

lbernick · 2022-03-03T16:41:45Z

LGTM! (I have no powers here :( )
Please squash into a single commit.

abayer · 2022-03-03T16:48:00Z

Squashed - I tend to wait to squash til I've addressed a round of reviews. =)

pritidesai

Thanks @abayer for the updates!

Please feel free to add yourself as an author!

I am seeing this status for the first time 😉 , we support proposed, implementable, implemented, withdraw, and replaced:

https://github.com/tektoncd/community/blob/main/teps/0001-tekton-enhancement-proposal-process.md#tep-workflow

Conditions will be gone from v1 API and so as this new addition to the ChildStatusReference but its necessary to add right now in beta API.

abayer · 2022-03-04T16:29:43Z

/wip

Putting this on hold for a bit - I think it's also missing some sort of trivial way to quickly see the status of a TaskRun skipped for conditions.

pritidesai · 2022-03-04T16:51:04Z

/wip

Putting this on hold for a bit - I think it's also missing some sort of trivial way to quickly see the status of a TaskRun skipped for conditions.

Its great we are discovering these findings at the implementation phase. Please feel free to propose any simplifications that you think might fit better 🙏

jerop

Thanks for proposing these changes @abayer! I wonder if we really need to add the condition and when expressions though given the motivation of the TEP to make the status minimal?

the conditions have been deprecated for a long time as discussed above, so I'd prefer we don't support them in the minimal status (we can document that they are not supported, and encourage users to migrate to when expressions to use the minimal status)
the when expressions are already in the pipeline spec in the pipelinerun status and all variables used (e.g. params, results) are available in the pipelinerun, taskrun or run statuses - we previously added them for convenience to users but in the spirit of keeping the status minimal, maybe we shouldn't duplicate this information in the minimal status?

abayer · 2022-03-08T16:45:46Z

@jerop Ah, for whatever reason, I just assumed the status of the when expressions was in there too. But if it's just duplicating the configuration of the when expressions, it's definitely not needed. And if there's a consensus around not including conditions in the minimal status, I'm completely fine with that too. Lemme think on that a little bit to be sure. =)

jerop

@abayer - that's right, we'd be duplicating the when expressions but with resolved values, however we discussed at the pipeline working group on 03/08 (notes) and decided that while it's duplicated information, it provides a huge inconvenience to see when expressions with the params, results, and other variables replaced

for conditions, we agreed in the same meeting to only have child references to the taskruns of the conditions instead of the full status (similar to other taskruns) - this looks like the only change that needs to be made to the TEP and should be good to go

thank you again for all your help here @abayer 😀

jerop · 2022-03-14T10:37:51Z

teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md

+To support `ConditionChecks`, we will add a new struct `PipelineRunChildConditionCheckStatus`
+which will hold the names and statuses of condition checks for the `PipelineTask`. It
+will inline the `PipelineRunConditionCheckStatus` currently used in the full embedded
+statuses. This is needed because `PipelineRunConditionCheckStatus` doesn't contain
+the `ConditionCheckName`, which is the equivalent of a `PipelineTask`'s name, just
+`ConditionName`, which is the equivalent of a `TaskRun` or `Run`'s name. Since we're 
+going to store an array of `PipelineRunChildConditionCheckStatus` rather than a map of 
+`ConditionCheckName` to `PipelineRunConditionCheckStatus`, we need the `ConditionCheckName`
+in the new struct.


given that conditionchecks are taskruns, can we have childreferences to them instead of the whole status, the same way we'll be doing it for taskruns?

I don't think we're storing the ConditionCheckStatus's contents anywhere else - ConditionCheckStatus is much simpler than TaskRunStatus. It's just a duckv1beta1.Status, PodName, StartTime, CompletionTime, and an optional corev1.ContainerState, vs TaskRunStatus's embedded RetriesStatus, Steps, CloudEvents, ResourceResults, TaskRunResults, Sidecars, and TaskSpec. I'll dig to see if we've got the ConditionCheckStatus anywhere we can reference to, but if not, I think it's ok to just leave it as is. Particularly since we'll be removing conditions in general at some point in the not-too-distant-future.

Yeah, verified - ConditionCheckStatus is only here. I guess we could reference the actual TaskRun behind the scenes, but this seems minimal enough to not be worth the bother to me.

good point that we're removing it soon after all and hopefully not many people are using it 👍🏾

Aaaah, wait, this can just be a reference to the underlying ConditionName and ConditionCheckName with the condition check step name from within the TaskRun, I think. I forgot that conditions aren't separate TaskRuns!

Since this will be stored inside the child reference for the relevant TaskRun (meaning it's already got the information needed to look up the TaskRun's status), we already have the duckv1beta.Status, PodName, StartTime, and CompletionTime which are used in the ConditionCheckStatus available. Then adding the condition check step name to the ConditionName and ConditionCheckName is enough to get the container state for the condition check step from the TaskRun's status.

Now I just need to figure out if we even need to specify the condition check step name - feels like that probably is one of ConditionName or ConditionCheckName - I can never remember which of those is which. =)

Ok, I've reconsidered - while that's almost certainly a viable option, it's more work than feels worthwhile for something that's not going to be relevant for that much longer anyway, so I am going with the full ConditionCheckStatus approach.

jerop

thanks again @abayer!

tekton-robot · 2022-03-15T15:04:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, pritidesai

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:

~~teps/OWNERS~~ [jerop,pritidesai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jerop · 2022-03-15T15:04:51Z

/assign @jerop
/assign @pritidesai

jerop · 2022-03-15T15:05:46Z

teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md

 title: Embedded TaskRuns and Runs Status in PipelineRuns
 creation-date: '2022-01-24'
-last-updated: '2022-02-14'
+last-updated: '2022-03-03'
 authors:
 - '@lbernick'
 - '@jerop'


@abayer please add yourself as an author 🙏🏾

…pressions With the current, full embedded status for `TaskRun`s and `Run`s, we include `ConditionChecks` and `WhenExpressions` fields as well as the `TaskRun`/`Run`'s status. The minimal embedded status approach as initially written does fine for anything that's available within the `TaskRun`/`Run` statuses, but `ConditionChecks` and `WhenExpressions` are not actually known by the `TaskRun`/`Run`. They're only known at the full `PipelineRun` level. In order to make that information available for reporting, etc in the new minimal embedded status approach, we will need to store `ConditionChecks` and `WhenExpressions` along with the kind/version/`TaskRun` or `Run` name/`PipelineTask` name. This also moves the TEP to `implementing` and updates its `last-updated`. Signed-off-by: Andrew Bayer <[email protected]>

abayer · 2022-03-25T15:53:23Z

So what more do we need to merge this?

lbernick · 2022-03-28T12:57:04Z

So what more do we need to merge this?

I'll bring this up at today's WG-- this can probably be merged then!

lbernick · 2022-03-28T16:08:57Z

/kind tep

afrittoli · 2022-03-28T16:17:00Z

Merged during the API WG

/lgtm

tekton-robot requested review from khrm and pritidesai March 2, 2022 15:21

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 2, 2022

abayer force-pushed the tep-100-add-conditions-and-whenexpressions branch from 50d8d22 to c38ad09 Compare March 2, 2022 15:25

lbernick reviewed Mar 3, 2022

View reviewed changes

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 3, 2022

lbernick reviewed Mar 3, 2022

View reviewed changes

abayer force-pushed the tep-100-add-conditions-and-whenexpressions branch from 9a5a106 to b3da905 Compare March 3, 2022 16:47

pritidesai approved these changes Mar 3, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2022

jerop reviewed Mar 8, 2022

View reviewed changes

jerop reviewed Mar 14, 2022

View reviewed changes

jerop approved these changes Mar 15, 2022

View reviewed changes

tekton-robot assigned jerop and pritidesai Mar 15, 2022

jerop reviewed Mar 15, 2022

View reviewed changes

abayer force-pushed the tep-100-add-conditions-and-whenexpressions branch from b3da905 to e05e05b Compare March 18, 2022 14:36

abayer force-pushed the tep-100-add-conditions-and-whenexpressions branch from e05e05b to db481a4 Compare March 23, 2022 17:31

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Mar 28, 2022

tekton-robot assigned afrittoli Mar 28, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2022

tekton-robot merged commit b97ee02 into tektoncd:main Mar 28, 2022

adietish mentioned this pull request Dec 23, 2022

migrate to kubernetes-client 6.4.0 redhat-developer/intellij-tekton#641

Closed

[TEP-0100] Update to handle PipelineTask condition checks and when expressions #645

[TEP-0100] Update to handle PipelineTask condition checks and when expressions #645

Conversation

abayer commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbernick commented Mar 3, 2022

abayer commented Mar 3, 2022

pritidesai left a comment • edited Loading

Choose a reason for hiding this comment

abayer commented Mar 4, 2022

pritidesai commented Mar 4, 2022

jerop left a comment • edited Loading

Choose a reason for hiding this comment

abayer commented Mar 8, 2022

jerop left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 15, 2022

jerop commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Mar 25, 2022

lbernick commented Mar 28, 2022

lbernick commented Mar 28, 2022

afrittoli commented Mar 28, 2022

pritidesai left a comment •

edited

Loading

jerop left a comment •

edited

Loading

jerop left a comment •

edited

Loading