[TEP-0048] Task Results without Results by vinamra28 · Pull Request #954 · tektoncd/community

vinamra28 · 2023-02-13T16:24:16Z

This TEP is updated with the new proposed solution where we now specify the default value for results at the time where they are consumed.

/kind feature

Signed-off-by: vinamra28 jvinamra776@gmail.com

vinamra28 · 2023-02-13T16:24:35Z

/kind tep

pritidesai · 2023-02-13T17:09:48Z

Before: default results at the authoring time
Now: default results at the time of resolution

pritidesai · 2023-02-13T17:17:20Z

API WG - is confusing in the first review, default in a pipelineTask instead of task authoring time
Alternative 1 - instead of default value during the resolution, specify default explicitly.

/assign @afrittoli
/assign @lbernick

jerop · 2023-02-13T17:26:35Z

noting that this was first suggested and discussed in #240 (comment) by @bobcatfish and @chhsia0

a user @kyubisation - also asked for this feature - tektoncd/pipeline#6139 (comment)

teps/0048-task-results-without-results.md

lbernick · 2023-02-14T15:21:01Z

teps/0048-task-results-without-results.md

-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 produces any result then it should fail. In order to
+continue with the execution of `Pipeline`, `onError: continue` should be added


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

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

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

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.

lbernick · 2023-02-14T15:22:16Z

teps/0048-task-results-without-results.md

+metadata:
+  name: pipeline-with-defaults
+spec:
+  tasks:


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

lbernick · 2023-02-14T15:25:05Z

teps/0048-task-results-without-results.md

+        name: demo-task
+      params:
+      - name: param1
+        value: $(tasks.taskA.results.merge_status || "foobar")


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"?

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

lbernick · 2023-02-14T15:25:48Z

teps/0048-task-results-without-results.md

+      - name: param2
+        value: $(tasks.taskA.results.branches || [foo, bar, baz])
+      - name: param3
+        value: $(tasks.taskA.results.images || {node="foo", gcloud="bar"})


Can you also do this for Pipeline results?

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

lbernick · 2023-02-14T15:26:45Z

teps/0048-task-results-without-results.md

      description: The pullRequestNumber
  tasks:
    - name: check-name-match
+      onError: continue


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

replied in comment #954 (comment)

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

teps/0048-task-results-without-results.md

lbernick · 2023-02-14T15:27:44Z

teps/0048-task-results-without-results.md


 ## Alternatives

+### Specifying Default Value during Runtime


why was this alternative rejected?

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?

tekton-robot · 2023-02-15T13:06:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from afrittoli after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

teps/OWNERS

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

Yongxuanzhang · 2023-02-15T20:13:12Z

/assign

This TEP is updated with the new proposed solution where we now specify the default value for results at the time where they are consumed. Signed-off-by: vinamra28 <jvinamra776@gmail.com>

tekton-robot · 2023-02-19T03:26:12Z

The following Tekton test failed:

Test name	Commit	Details	Required	Rerun command
pull-community-teps-lint	`f76fefe`	link	true	`/test pull-community-teps-lint`

afrittoli

Thanks! This looks mostly good, I have a couple of comments though.
I'm not entirely sure we should prioritise the variable option vs. the pipeline task one.

afrittoli · 2023-03-06T10:59:32Z

teps/0048-task-results-without-results.md

+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:


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.

afrittoli · 2023-03-06T11:07:10Z

teps/0048-task-results-without-results.md

-        - input: "$(tasks.check-pr-content.results.image-change)"
+        - input: "$(tasks.check-pr-content.results.image-change || no)"


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.

afrittoli · 2023-03-06T11:09:57Z

teps/0048-task-results-without-results.md

      params:
        - name: trusted-image-name
-          value: "$(tasks.build-trusted.results.image)"
+          value: "$(tasks.build-trusted.results.image || "trusted")"


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?

afrittoli · 2023-03-06T11:10:39Z

teps/0048-task-results-without-results.md

      description: The pullRequestNumber
  tasks:
    - name: check-name-match
+      onError: continue


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

afrittoli · 2023-03-06T11:13:32Z

teps/0048-task-results-without-results.md


 ## Alternatives

+### Specifying Default Value during Runtime


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?

afrittoli · 2023-03-06T11:14:44Z

teps/0048-task-results-without-results.md

+`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


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.

tekton-robot · 2023-03-30T16:52:49Z

@vinamra28: PR needs rebase.

Details

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.

pritidesai · 2023-05-15T16:07:03Z

API WG - @jerop to check with @vinamra28

tekton-robot · 2023-08-13T16:16:47Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

vinamra28 · 2023-08-19T17:58:28Z

/remove-lifecycle stale
apologies for leaving this in between 😅 , will take this up and try to get this done

chitrangpatel · 2024-03-04T17:11:49Z

API WG: @vinamra28 any updates on this?

chitrangpatel · 2024-05-13T16:07:39Z

API WG: Any new updates here @vinamra28?

jameshwc · 2024-10-02T15:32:59Z

Any updates on this? This proposal has existed for long and the default result feature is necessary for our pipelines

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 13, 2023

tekton-robot requested review from pradeepitm12 and pratap0007 February 13, 2023 16:24

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 13, 2023

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Feb 13, 2023

vinamra28 force-pushed the update-tep-0048 branch from ac66492 to c9e04e2 Compare February 13, 2023 16:53

tekton-robot assigned afrittoli and lbernick Feb 13, 2023

This was referenced Feb 13, 2023

Pipeline failing with "invalid pipelineresults" when referencing results of skipped tasks tektoncd/pipeline#6139

Closed

don't validate skipped task results for pipeline results tektoncd/pipeline#6157

Merged

lbernick reviewed Feb 14, 2023

View reviewed changes

vinamra28 force-pushed the update-tep-0048 branch from c9e04e2 to a07b496 Compare February 15, 2023 13:06

tekton-robot assigned Yongxuanzhang Feb 15, 2023

[TEP-0048] Task Results without Results

f76fefe

This TEP is updated with the new proposed solution where we now specify the default value for results at the time where they are consumed. Signed-off-by: vinamra28 <jvinamra776@gmail.com>

vinamra28 force-pushed the update-tep-0048 branch from a07b496 to f76fefe Compare February 19, 2023 03:10

jerop mentioned this pull request Feb 21, 2023

[TEP-0048] Add support for default results tektoncd/pipeline#5620

Closed

7 tasks

afrittoli reviewed Mar 6, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 13, 2023

tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 19, 2023

		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:

		- input: "$(tasks.check-pr-content.results.image-change)"
		- input: "$(tasks.check-pr-content.results.image-change \|\| no)"

Conversation

vinamra28 commented Feb 13, 2023

Uh oh!

vinamra28 commented Feb 13, 2023

Uh oh!

pritidesai commented Feb 13, 2023

Uh oh!

pritidesai commented Feb 13, 2023

Uh oh!

jerop commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Feb 15, 2023

Uh oh!

Yongxuanzhang commented Feb 15, 2023

Uh oh!

tekton-robot commented Feb 19, 2023

Uh oh!

afrittoli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Mar 30, 2023

Uh oh!

pritidesai commented May 15, 2023

Uh oh!

tekton-robot commented Aug 13, 2023

Uh oh!

vinamra28 commented Aug 19, 2023

Uh oh!

chitrangpatel commented Mar 4, 2024

Uh oh!

chitrangpatel commented May 13, 2024

Uh oh!

jameshwc commented Oct 2, 2024

Uh oh!

Reviewers

Assignees

Labels