[TEP-0048] Add support for default results #5620
Conversation
|
Skipping CI for Draft Pull Request. |
tekton-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
size/L
|
/test all |
|
The following is the coverage report on the affected files.
|
vinamra28
force-pushed
the
default-results
branch
from
0150c97
to
58e2459
Compare
|
/test all |
|
The following is the coverage report on the affected files.
|
|
/test tekton-pipeline-unit-tests |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tekton-robot
added
the
needs-rebase
vinamra28
force-pushed
the
default-results
branch
from
58e2459
to
c18ea51
Compare
tekton-robot
removed
the
needs-rebase
|
/test pull-tekton-pipeline-alpha-integration-tests |
tekton-robot
added
the
needs-rebase
vinamra28
force-pushed
the
default-results
branch
from
a5f6c3a
to
de04679
Compare
tekton-robot
removed
the
needs-rebase
|
/test all |
|
The following is the coverage report on the affected files.
|
|
/test all |
|
The following is the coverage report on the affected files.
|
tekton-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
vinamra28
force-pushed
the
default-results
branch
from
af5afd7
to
0139358
Compare
|
The following is the coverage report on the affected files.
|
vinamra28
force-pushed
the
default-results
branch
from
0139358
to
12a3fb4
Compare
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
vinamra28
force-pushed
the
default-results
branch
from
12a3fb4
to
db72816
Compare
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
vinamra28
force-pushed
the
default-results
branch
from
db72816
to
993e4ed
Compare
|
The following is the coverage report on the affected files.
|
Today, a Task can declare a Result but not produce it and it's execution will still be successful whereas if a subsequent Task in a Pipeline attempts to use that Result it will fail. This behaviour can be confusing to the users as the Task which didn't produced the Result passed and the follow-up Task fails. Moreover, the user of the Task will loose faith in it as it failed to produce what it's claiming. This patch introduces the default results, ie, Task author can now add a default value to the results so that if their Task doesn't produce any result then the default value can be emitted. Now, with this change, if the Task author doesn't specifies any default value for the result and one of the step within that Task also doesn't produces it then the Task execution in whole will be marked as failed as it failed to prove it's contract.
vinamra28
force-pushed
the
default-results
branch
from
993e4ed
to
78cb4b1
Compare
|
The following is the coverage report on the affected files.
|
| } | ||
| r.Default.ObjectVal = defaults | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems to be the same logic as in lines 18:34. May be wrap it into a function and call it here?
tekton-robot
added
the
needs-rebase
|
@vinamra28: PR needs rebase. 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. |
| image: bash:latest | ||
| script: | | ||
| #!/usr/bin/env bash | ||
| echo -n "this script won't emit array result" |
There was a problem hiding this comment.
I wanted to highlight the issue #3497 which probably seems to go against this example?
There was a problem hiding this comment.
The discussion in that issue hasn't come to a conclusion so this might be ok for now. I wonder if your examples could genuinely error out and the steps have an onError --> continue and then have the default result use?
| Type: tR.Type, | ||
| Value: *tR.Default, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I think this part needs to be tested. Seems to be missing in the coverage report.
There was a problem hiding this comment.
ie.the test case needs to triggerif tR.Default != nil {
|
@vinamra28 we discussed taking a different approach for default results - closing this pull request as we await the TEP update (feel free to reopen if we end up continuing with this approach, just trying to clean up the repo) 🙏🏾 /close |
|
@jerop: Closed this PR. In response to this:
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. |
Who is we and is anyone is working on this ? |
|
@vinamra28: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@vdemeester here we refers to me and @jerop 😅 |
|
@vinamra28 ah ok 🙃 😬 Anything we can do to help there ? (on updating the TEP, …) |
@vdemeester you can help by reviewing this TEP update: tektoncd/community#954 🙏🏾 |
Changes
Today, a Task can declare a Result but not produce it and it's execution
will still be successful whereas if a subsequent Task in a Pipeline
attempts to use that Result it will fail. This behaviour can be
confusing to the users as the Task which didn't produced the Result
passed and the follow-up Task fails. Moreover, the user of the Task will
loose faith in it as it failed to produce what it's claiming.
This patch introduces the default results, ie, Task author can now add a
default value to the results so that if their Task doesn't produce any
result then the default value can be emitted. Now, with this change, if
the Task author doesn't specifies any default value for the result and
one of the step within that Task also doesn't produces it then
the Task execution in whole will be marked as failed as it failed to
prove it's contract.
For more, please refer TEP-0048
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes