Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[TEP-0048] Task Results without Results #954

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[TEP-0048] Task Results without Results #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 176 additions & 40 deletions teps/0048-task-results-without-results.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ authors:
- "@pritidesai"
- "@jerop"
- "@vinamra28"
- "@bobcatfish"
creation-date: 2020-10-20
last-updated: 2022-08-09
last-updated: 2023-02-20
status: implementable
---

Expand All @@ -27,6 +28,8 @@ status: implementable
- [<code>Task</code> claiming to produce <code>Result</code> fails if it doesn't produces](#-claiming-to-produce--fails-if-it-doesnt-produces)
- [Test Plan](#test-plan)
- [Alternatives](#alternatives)
- [Specifying Default Value during Runtime](#specifying-default-value-during-runtime)
- [Specifying Default Value at the time of Authoring the Task](#specifying-default-value-at-the-time-of-authoring-the-task)
- [Declaring Results as Optional](#declaring-results-as-optional)
- [Future Work](#future-work)
- [References](#references)
Expand Down Expand Up @@ -81,9 +84,9 @@ a missing task result. There are many reasons for a missing task result:
* a `task` producing task result failed, no result available
* a `task` producing result was skipped/disabled and no result generated
* a `task` producing result did not generate that result even without any failure. We have a
[bug report](https://github.com/tektoncd/pipeline/issues/3497) open to declare
[bug report][tektoncd/pipeline#3497] open to declare
such a task as failure. This reason might not hold true after issue
[#3497]((https://github.com/tektoncd/pipeline/issues/3497)) is fixed.
[#3497][tektoncd/pipeline#3497] is fixed.

Here are the major motivations for `pipeline` authors to design their pipelines with the missing task results:

Expand Down Expand Up @@ -426,47 +429,76 @@ will fix the `Pipeline`.

## Proposal

We propose adding an optional field - `default` - to the `Result` specification in `Task`.
We propose adding default value at the place where variable replacement
happens and let consumer decide which value it wants to pick up. For example:
Comment on lines +432 to +433
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can image use cases for both task authoring time and pipeline authoring time, I'm not sure one should exclude the other - but it'd be ok to prioritise one implementation against the other.

In terms of pipeline authoring time, the result default value could also be defined as part of the pipeline task.
Having it in the variable replacement means that, if the result is consumed more than once, the default will have to be repeated everywhere. It seems unlikely to me that a pipeline author would have different result default values for different consumers. But again, I can imagine us implementing both options eventually.


```yaml
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
name: task
spec:
results:
- name: merge_status
description: whether to rebase or squash
type: string
default: rebase
- name: branches
description: branches in the repository
type: array
default:
- main
- v1alpha1
- v1beta1
- v1
- name: images
type: object
properties:
node:
type: string
default: "node:latest"
gcloud:
type: string
default: "gcloud:latest"
steps:
...
results:
- name: merge_status
description: whether to rebase or squash
type: string
- name: branches
description: branches in the repository
type: array
- name: images
type: object
properties:
node:
type: string
gcloud:
type: string
---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
name: pipeline-with-defaults
spec:
tasks:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to make this example more realistic based on this TEP's use cases; it would help show why the Pipeline is the appropriate level for defaults rather than the task

- name: taskA
onError: continue
taskRef:
name: task
results:
- name: merge_status
- name: taskB
runAfter:
- "taskA"
taskRef:
name: demo-task
params:
- name: param1
value: $(tasks.taskA.results.merge_status || "foobar")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting; it seems like there are two ways to specify the default. Here, taskA's result defaults to "squash", so is there ever a case where it could be "foobar"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh sorry, I missed removing the default: squash (this is part of alternative solution 1)

- name: param2
value: $(tasks.taskA.results.branches || [foo, bar, baz])
- name: param3
value: $(tasks.taskA.results.images || {node="foo", gcloud="bar"})
- name: taskC
runAfter:
- "taskA"
taskRef:
name: demo-task
params:
- name: param1
value: $(tasks.taskA.results.merge_status)
- name: param2
value: $(tasks.taskA.results.branches || [foo, bar, baz])
- name: param3
value: $(tasks.taskA.results.images || {node="foo", gcloud="bar"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also do this for Pipeline results?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, @vinamra28 let's add an examples in pipeline results

...
```


Adding a default value to `Results` is optional; validation of a `Task` doesn't
fail if the default value isn't provided.

Adding a default value to a `Result` will guarantee that it will hold a value even
if the `Task` fails to produce it. If a `Task` does not produce a `Result` that does not
have a default value, then the `Task` should fail - see [tektoncd/pipeline#3497](https://github.com/tektoncd/pipeline/issues/3497) for further details.
If the `Task` doesn't produce any result then it should fail. In order to
continue with the execution of `Pipeline`, `onError: continue` should be added
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought onError was only available in a task spec? can you give an example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure whether TEP-0050 is 100% implemented or not but saw it there cc @jerop

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still "implementable" instead of "implemented" so maybe it wasn't completed? cc @QuanZhang-William

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is correct. I didn't get a chance to implement this feature. I also did a PR search in the pipeline and didn't find much related.

in the `PipelineTask` which we anticipate to fail and subsequent `Tasks`
which are consuming the results produced by the previous `Task` should have a
default value specified.

The proposed solution can be used to solve the above use cases as follows:

Expand All @@ -491,7 +523,7 @@ spec:
- name: build-image
runAfter: [ "check-pr-content" ]
when:
- input: "$(tasks.check-pr-content.results.image-change)"
- input: "$(tasks.check-pr-content.results.image-change || no)"
Comment on lines -494 to +526
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this, but it doesn't look very useful.
I think default results will be useful to execute tasks which depend on tasks which are controlled by a when expression, rather than the when expression directly. There is no reason why the "Check PR content" result should be missing. Instead, if the build-image is not executed, following tasks may won't a default value for the image to be used.

operator: in
values: ["yes"]
taskRef:
Expand Down Expand Up @@ -554,9 +586,9 @@ spec:
runAfter: [ "build-image" ]
params:
- name: trusted-image-name
value: "$(tasks.build-trusted.results.image)"
value: "$(tasks.build-trusted.results.image || "trusted")"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I think the strings "trusted" and "untrusted" here are a bit confusing, and don't really help convey how the default value would be useful. Maybe something like "my-image:latest-trusted" and "my-image:latest-untrusted" would be more indicative of the purpose?

- name: untrusted-image-name
value: "$(tasks.build-untrusted.results.image)"
value: "$(tasks.build-untrusted.results.image || "untrusted")"
taskRef:
name: propagate-image-name
# pipeline result
Expand Down Expand Up @@ -586,7 +618,6 @@ spec:
results:
- name: check
description: The result of the check, "passed" or "failed"
default: "passed"
steps:
- name: check-name
image: alpine
Expand Down Expand Up @@ -631,6 +662,7 @@ spec:
description: The pullRequestNumber
tasks:
- name: check-name-match
onError: continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this valid? I don't see it in the api

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replied in comment #954 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not available yet at task level, only at step level.

taskRef:
name: check-name-matches
params:
Expand All @@ -640,7 +672,7 @@ spec:
value: $(params.checkName)
- name: clone-repo
when:
- input: $(tasks.check-name-match.results.check)
- input: $(tasks.check-name-match.results.check || "failed")
operator: in
values: ["passed"]
taskRef:
Expand All @@ -655,8 +687,12 @@ spec:
...
```

In the above example, if the value of parameter `checkName` is not passed,
the `Task` is failed but in order to continue the execution of the `Pipeline`,
we use the `onError: continue` flag and

In the above example, even if the value of parameter `checkName` is not passed,
default value of `Result` will be produced `passed` and Pipeline's execution will
default value of `Result` will be produced `failed` and Pipeline's execution will
be continued. The only case when the execution of `Pipeline` is stopped when
the wrong value of parameter `checkName` is passed.

Expand All @@ -670,6 +706,103 @@ Having default results defined in check-name-matches Task will fix the Pipeline.

## Alternatives

### Specifying Default Value during Runtime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this alternative rejected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if was explicitly rejected - I don't see it as orthogonal to the chosen one - but perhaps it would be best to clarify whether these alternatives are meant as not going to be implemented or possibly going to be implemented later?


Adding an optional field - `default` for a `Result` which can be specified
during runtime, i.e., in `TaskRun` or `Pipeline` for a `Task` or in `PipelineRun` for a `Pipeline`.

```yaml
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
name: task
spec:
results:
- name: merge_status
description: whether to rebase or squash
type: string
- name: branches
description: branches in the repository
type: array
- name: images
type: object
properties:
node:
type: string
gcloud:
type: string
steps:
...
---
apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
name: task
spec:
taskRef:
name: task
results:
- name: merge_status
default: rebase
...
```

Passing the default value for the `Result` at runtime is optional; validation
of `TaskRun` or `Pipeline` or `PipelineRun` doesn't fail if the value isn't provided.

Adding a default value to `Results` is optional; validation of a `Task` doesn't
fail if the default value isn't provided.

Adding a default value to a `Result` will guarantee that it will hold a value even
if the `Task` fails to produce it. If a `Task` does not produce a `Result` that does not
have a default value, then the `Task` should fail - see [tektoncd/pipeline#3497][tektoncd/pipeline#3497] for further details.

Values specified in `TaskRun` or `Pipeline` should be overwritten by declaring
`Task` itself, if `Task` declares a `Result` then it should be considered as
the final value for that result.

### Specifying Default Value at the time of Authoring the Task
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is another alternative, which would be to define the value at the time of authoring the pipeline, but as part of the pipeline task as opposed to when the result is consumed via a variable.


Allow `Results` to declare an optional field as `default`. When a `Task` fails to
produce a `Result` but has a default value specified, then the `Task` does not fail. When a `Task` fails
to produce a `Result` that doesn't have any default value specified, then then the `Task` will fail.

```yaml
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
name: task
spec:
results:
- name: merge_status
description: whether to rebase or squash
type: string
default: "rebase"
- name: branches
description: branches in the repository
type: array
default:
- "foo"
- "bar"
- name: images
type: object
properties:
node:
type: string
gcloud:
type: string
default:
node: "16"
gcloud: "true"
steps:
...
```

This solution only solves the issue of `Tasks` not failing when they
don't produce `Results` as discussed in [tektoncd/pipeline#3497][tektoncd/pipeline#3497].
However, it does not addressed the use case where an user can
have different default values at runtime based on their `Pipeline`.

### Declaring Results as Optional

Allow `Results` to declare an optional field as `optional`. When a `Task` fails to
Expand Down Expand Up @@ -704,7 +837,7 @@ spec:
```

However, this solution only solves the issue of `Tasks` not failing when they don't
produce `Results` as discussed in [tektoncd/pipeline#3497](https://github.com/tektoncd/pipeline/issues/3497).
produce `Results` as discussed in [tektoncd/pipeline#3497][tektoncd/pipeline#3497].
It does not address the use cases for providing default `Results` that can be consumed in subsequent `Tasks`.


Expand All @@ -721,3 +854,6 @@ Determine if we need default `Results` declared at runtime in the future, and ho
* [Issue reported - "when" expressions do not match user expectations](https://github.com/tektoncd/pipeline/issues/3345)

* [Accessing Execution status of any DAG task from finally](https://github.com/tektoncd/community/blob/master/teps/0028-task-execution-status-at-runtime.md)


[tektoncd/pipeline#3497]: https://github.com/tektoncd/pipeline/issues/3497
2 changes: 1 addition & 1 deletion teps/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ This is the complete list of Tekton TEPs:
|[TEP-0045](0045-whenexpressions-in-finally-tasks.md) | WhenExpressions in Finally Tasks | implemented | 2021-06-03 |
|[TEP-0046](0046-finallytask-execution-post-timeout.md) | Finally tasks execution post pipelinerun timeout | implemented | 2021-12-14 |
|[TEP-0047](0047-pipeline-task-display-name.md) | Pipeline Task Display Name | implementable | 2022-01-04 |
|[TEP-0048](0048-task-results-without-results.md) | Task Results without Results | implementable | 2022-08-09 |
|[TEP-0048](0048-task-results-without-results.md) | Task Results without Results | implementable | 2022-02-20 |
|[TEP-0049](0049-aggregate-status-of-dag-tasks.md) | Aggregate Status of DAG Tasks | implemented | 2021-06-03 |
|[TEP-0050](0050-ignore-task-failures.md) | Ignore Task Failures | implementable | 2022-09-16 |
|[TEP-0051](0051-ppc64le-architecture-support.md) | ppc64le Support | proposed | 2021-01-28 |
Expand Down