-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
access execution status of any task in finally #3390
Conversation
The following is the coverage report on the affected files.
|
examples/v1beta1/pipelineruns/pipelinerun-task-execution-status.yaml
Outdated
Show resolved
Hide resolved
ebdb86d
to
1b3a310
Compare
The following is the coverage report on the affected files.
|
1b3a310
to
a7bae2d
Compare
The following is the coverage report on the affected files.
|
hey @pritidesai !! I'm very excited to see this moving forward :D Do you think this might be worth a TEP to explore the problem and the alternatives a bit? For example the mapping between the "execution states" and "status" you're proposing (#1020 (comment)) is interesting - it's also different from what you currently see in the status field of an executing TaskRun/PipelineRun so I think it might be worth contrasting this with some different approaches, e.g.:
Looking at the docs on the execution status it feels like this proposal is making the trying to make the "reason" field available to users, and treating that as the overall status (and mixing 'skipped' in as well)
This one in particular is confusing: "unknown" as a condition status currently means "running" but we are using "unknown" to mean "not set" (i think?) |
a7bae2d
to
0fb33f4
Compare
The following is the coverage report on the affected files.
|
One more thing that might be worth exploring: some of these statuses won't have meaning or be available. For example:
For all of these, the values will only be available to tasks that execute during or after the task that is trying to use the values |
🎉 🎉 🎉
Yup, definitely 👍
The use cases we have here are:
The proposed list of states here are derived based on
👍 This can be next in line and can enhance the existing proposal. The existing proposal brings quick access to the state without investing into json path.
Definitely we can rename the states to
Yup, so that its not hidden in
It means there is absolutely no way to determine what is the status at that point in time. We could introduce Within Within So worth a TEP 😉 |
When any |
|
Thanks for the detailed explanation! I'm going to take my comments mostly into the TEP; in general I'm wondering if there is a simpler subset of these values we could start with initially, and maybe to keep things simpler, start with just the expected values of the Succeeded ConditionType, then we can be deliberate about adding more if needed.
At the moment I don't think there's a scenario where a task is cancelled and other tasks will start to execute after that. If there is later on, we could add additional information to the variable replacement to cover it.
Oh yeah great point! imo this is even more reason not to include this as one of the possible values of the new field (more in my TEP comments!)
Oh, do you mean when a taskrun that is part of a pipelinerun is cancelled? I was thinking of when the entire PipelineRun is cancelled. I guess I could see someone doing that but it seems like a strange scenario to support - but in the TEP it looks like "cancelled" isnt being proposed initially so maybe we can cross that bridge when we come to it :D |
0fb33f4
to
4e8d93c
Compare
The following is the coverage report on the affected files.
|
4e8d93c
to
2a4e707
Compare
The following is the coverage report on the affected files.
|
b1d7889
to
9665da2
Compare
The following is the coverage report on the affected files.
|
9665da2
to
a6191a0
Compare
The following is the coverage report on the affected files.
|
examples/v1beta1/pipelineruns/pipelinerun-task-execution-status.yaml
Outdated
Show resolved
Hide resolved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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 |
a6191a0
to
e70d619
Compare
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pritidesai !
This looks good, the only problem left that I see is the parameter variable validation, which makes the assumption that the parameter is in the format "$(var)" only.
I would prefer not checking explicitly for cancel / timeout, since that code will never be reached anyways and we have no tests for it. It should be a quick fix if you want to update the PR it can still be in v0.20.0
s = v1beta1.TaskRunReasonSuccessful.String() | ||
case t.IsFailure(): | ||
s = v1beta1.TaskRunReasonFailed.String() | ||
case t.IsCancelled(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cancelled is a special case of failed. IsCancelled
returns true only if the condition is false (==failure) and the reason is a special one. There may be a case of tasks for which cancelled has been requested but they have not been cancelled yet, but those are not covered by the IsCancelled
check, as it should be - for them we would return None
.
s = v1beta1.TaskRunReasonFailed.String() | ||
case t.IsCancelled(): | ||
s = v1beta1.TaskRunReasonFailed.String() | ||
case t.TaskRun != nil && t.TaskRun.HasTimedOut(ctx): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout should be a special case of failed. A Task that has timed out is failed.
This check does not verify that the condition is set to false, only that the task is timed out, so it includes task yet not completed in the sense that the controller has not enforced the timeout yet.
I think we should only read the condition, and return false is false, true if true or none otherwise.
@@ -60,6 +60,22 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError | |||
return nil | |||
} | |||
|
|||
func ValidateVariablePS(value, prefix string, suffix string, vars sets.String) *apis.FieldError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: it would be nice to have a comment that describes what the function does (here and for several other functions in this module).
} | ||
} | ||
for _, paramValue := range paramValues { | ||
if strings.HasPrefix(stripVarSubExpression(paramValue), "tasks.") && strings.HasSuffix(stripVarSubExpression(paramValue), ".status") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: we don't need to invoke stripVarSubExpression
twice here.
@@ -60,6 +60,22 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError | |||
return nil | |||
} | |||
|
|||
func ValidateVariablePS(value, prefix string, suffix string, vars sets.String) *apis.FieldError { | |||
if vs, present := extractVariablesFromString(value, prefix); present { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pritidesai - I do not see the check in this function - perhaps you refer to https://github.com/tektoncd/pipeline/pull/3390/files#diff-78ff878240189b20e0ef82f2912c93c0b3c7b445865e04fa9b98aab851be6130R321 (L321)? However that check will only work for parameters in the "$(var)" format, but not if extra text or more variables are included.
e70d619
to
8f567fa
Compare
Introducing a variable which can be used to access the execution status of any pipelineTask in a pipeline. Use $(tasks.<pipelineTask>.status) as param value which contains the status, one of, Succeeded, Failed, or None.
8f567fa
to
64d9689
Compare
The following is the coverage report on the affected files.
|
thanks @afrittoli, I have created issue on validation as per our slack conversation and updated the checks to check for successful condition type with status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the updates and the patience!
It's great to have this new feature in Tekton :)
/lgtm
@@ -87,6 +87,17 @@ func ApplyTaskResults(targets PipelineRunState, resolvedResultRefs ResolvedResul | |||
} | |||
} | |||
|
|||
//ApplyPipelineTaskContext replaces context variables referring to execution status with the specified status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: missing space // Apply...
@@ -132,6 +132,18 @@ func (t ResolvedPipelineRunTask) IsStarted() bool { | |||
return t.TaskRun != nil && t.TaskRun.Status.GetCondition(apis.ConditionSucceeded) != nil | |||
} | |||
|
|||
// IsConditionStatusFalse returns true when a task has succeeded condition with status set to false | |||
// it includes task failed after retries are exhausted, cancelled tasks, and time outs | |||
func (t ResolvedPipelineRunTask) IsConditionStatusFalse() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you!
@@ -60,6 +60,22 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError | |||
return nil | |||
} | |||
|
|||
func ValidateVariablePS(value, prefix string, suffix string, vars sets.String) *apis.FieldError { | |||
if vs, present := extractVariablesFromString(value, prefix); present { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I reasonable fix here could be to have a variation of extractVariablesFromString
that returns the stripped suffix
so that it could be verified.
We should follow up on this PR to had such fix to make sure the code does not break when we add support for results like @GregDritschler mentioned.
$(tasks..status) How would a PipelineRun that has its Pipeline/Task specs fully inlined access these parameters? I believe for a fully inlined PipelineRun, tasks get random suffixes attached to their names. |
@riceluxs1t please provide an example YAML if possible.
Here is an example of pipelineRun with inlined specification: https://github.com/tektoncd/pipeline/blob/master/examples/v1beta1/pipelineruns/pipelinerun-task-execution-status.yaml
Are you seeing suffixes getting attached to pipelineTask names? |
Changes
Introducing a variable which can be used to access the execution status of any
pipelineTask
in apipeline
. This can be used in anyfinally
task (and limited tofinally
tasks for the first iteration).Use
$(tasks.<pipelineTask>.status)
as param value which resolves to the status, one of,Succeeded
,Failed
, andNone
.E.g.:
Partially Closes #1020
Implements TEP #0028
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:
cmd
dir, 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
/kind feature