Adding TEP for task results in final tasks #153

pritidesai · 2020-07-18T10:06:23Z

Proposal for enabling final tasks to get value from non-final tasks.

bobcatfish

Looks great! I have a bit of feedback about a user story and an example but overall I'm very excited and I really like the proposal :D

teps/draft-20200716-task-results-in-final-tasks.md

bobcatfish · 2020-07-20T13:04:47Z

teps/draft-20200716-task-results-in-final-tasks.md

+
+* No API changes.
+
+* Finally contract does not break and all final tasks are attempted.


👍 this was a great call, thanks for thinking of this approach @pritidesai !

teps/draft-20200716-task-results-in-final-tasks.md

pritidesai · 2020-07-21T00:17:07Z

@bobcatfish I have addressed your comments and also added a very high level workflow to help understand it better.

What are the next steps in terms of changing the filename or status? 🤔

pritidesai · 2020-07-21T00:56:58Z

Also, two things I came across while working on this PR regarding the TEP template in general:

Links to GitHub issue and/or design doc in Google drive - should we create a separate section called References or add a placeholder to insert the GitHub issue? or something similar?
Inserting images, might be a good idea to designate a directory called /teps/images/ and ask TEP authors to add files in that directory by following a specific naming convention.

@vdemeester @bobcatfish Thoughts?

vdemeester · 2020-07-21T10:14:17Z

1. Links to GitHub issue and/or design doc in Google drive - should we create a separate section called `References` or add a placeholder to insert the GitHub issue? or something similar?

Good point, I think we can definitely have a References header to help referering to anything we may want to refer (other TEPs, design docs, issues, …)

2. Inserting images, might be a good idea to designate a directory called `/teps/images/` and  ask `TEP` authors to add files in that directory by following a specific naming convention.

Either that or /teps/TEP-0004/ for TEP-0004 resources (images, …) ? Not sure which one I prefer 😝

@vdemeester @bobcatfish Thoughts?

vdemeester

Few small comments
/lgtm

vdemeester · 2020-07-21T10:18:49Z

teps/draft-20200716-task-results-in-final-tasks.md

+However if the `Task` being attempted has a default value for that parameter, it can still run,
+using the default when result is not available.


I would highly a bit more the default part. In a few sentences what we are proposing is : You can refer to results from tasks, and provide a default value if those results did not get populated. Any non-populated results that do not have a default will make the finally task fail.

(oh it's on the "default" params, I see)

vdemeester · 2020-07-21T10:20:44Z

teps/draft-20200716-task-results-in-final-tasks.md

+```
+
+**Note:** Today final tasks are executed all in parallel and can not depend on any other `Task` in the `Pipeline`.
+With adding support for `Task` `Results`, we are introducing dependencies to finally `Tasks` but that


I am not sure I understand this (nor if it should be there). What we are saying is "now finally tasks depends on tasks" ? that was implicitly the case already (from the user perpective) as the finally task will wait for all the "normal" task to complete (success or failure).

What we are saying is "now finally tasks depends on tasks" ? that was implicitly the case already (from the user perpective) as the finally task will wait for all the "normal" task to complete (success or failure).

Finally tasks did not have any implicit/explicit dependency on any other task. The scheduling algorithm sends final tasks to execute after all normal/DAG tasks are completed. Now, with task results, implicit dependency is being introduced. There is no impact on scheduling yet, final tasks still scheduled after all DAG. But worth calling out now and will easier to understand what happens when we add support for task results from other final tasks, runAfter in final tasks, from in final tasks, etc

bobcatfish · 2020-07-21T13:50:46Z

Good point, I think we can definitely have a References header to help referering to anything we may want to refer (other TEPs, design docs, issues, …)

great idea!!

Inserting images, might be a good idea to designate a directory called /teps/images/ and ask TEP authors to add files in that directory by following a specific naming convention.
Either that or /teps/TEP-0004/ for TEP-0004 resources (images, …) ? Not sure which one I prefer 😝

i have no strong opinion! slight inclination toward /teps/images/ to keep it simple

bobcatfish · 2020-07-21T14:02:13Z

What are the next steps in terms of changing the filename or status? 🤔

I'm thinking we can at least merge as "proposed" asap (correct me if I'm wrong @vdemeester ) with the final filename, and then open another PR to change that to "implementable" and continue discussing

bobcatfish

Just some minor feedback from me, but nothing that would stop us from merging with proposed status!

bobcatfish · 2020-07-21T14:03:29Z

teps/draft-20200716-task-results-in-final-tasks.md

+
+**A.**
+
+```


can you explain the context of this workflow a bit more? e.g. " Param Validation " i'm not sure if you mean, params to the finally tasks? params for all tasks?

(also im confused how we can validate the params before we apply results from dependent tasks?)

In this context, "Param validation" resolves params and run syntax check on params to all final tasks:

(1) check if parameter name is unique
(2) type of the default value matches with its type
(3) param with task results has valid result reference i.e. $(tasks.results.taskName.param)
(4) resolve parameter mapping and apply values from PipelineRun or Pipeline

"Validate and Apply results from Dependent Tasks" actually resolves the task result from dependent tasks and applies those values to params in the final tasks.

In general, the same workflow is executed for non-final tasks too.

k i think this is a bit more clear to me

bobcatfish · 2020-07-21T14:08:35Z

teps/draft-20200716-task-results-in-final-tasks.md

+
+* This proposal is extensible with a param validation proposal using Schema -
+[#1393](https://github.com/tektoncd/pipeline/issues/1393#issuecomment-558833526) by reading default from
+`Task` specification.


would it make sense to remove this reference to schema as well? maybe this is a discussion for somewhere else, but this is how i would see this feature interacting with schema:

Param defaults should always pass their own schema; if a task declares a default that does not pass it's schema, the Task creation should fail

If we one day add Result defaults, when a Task fails, the default of the result could be used as the Result of that Task. If a Task wants to use that result as the value for one if its own params, then the Task validation will evaluate that value; if the default is invalid, the validation will fail, if it is valid, the Task can continue

sure @bobcatfish, shall I drop the whole bullet point along with the example? or just the schema reference?

never mind, I just dropped the reference and have the example as is.

bobcatfish · 2020-07-21T14:10:28Z

teps/draft-20200716-task-results-in-final-tasks.md

+
+> `PipelineRun` reads parameter defaults from `Pipeline` specification but in case of
+parameters associated with `Task` `Results`, the default values will be used from the `Task` since
+`Pipeline` has no direct mapping between `Pipeline` parameter and `Task` parameter.


is the example below meant to demonstrate that even if boskos-acquire fails, boskos-release will still run because it has a default for the param? if so i think we might want to say that explicitly

yup, added it explicitly 😄

vdemeester · 2020-07-21T15:33:16Z

So we should "assign" a number to the TEP before merging as proposed (so that we rename it 0005-…), that would be the only requirement :)
Feels like it should be 0004 as I see this one getting merged before #147 for example 🙃

Proposal for enabling final tasks to get value from non-final tasks.

pritidesai · 2020-07-21T23:10:43Z

@bobcatfish @vdemeester I have renamed the file to 0004-task-results-in-final-tasks.md, made two changes:

Dropped the reference to GitHub issue with param validation schema
Added explicitly "task is executed with default value if dependency fails and no task result produced" under "Advantages" section.

Ready to review again 🙏

vdemeester

/lgtm

bobcatfish · 2020-07-22T18:05:46Z

Let's go for it! Then we can open a PR to get it to implementable :D

/approve

tekton-robot · 2020-07-22T18:05:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

…stamp (tektoncd#153) * Disallow requiring the branch to be up to date for rekor-monitor/timestamp This was a bit of a nuisance since it required a rebase constantly. Also turned off merge commits for rekor-monitor. Also turned off merge commits for fulcio, and enforced admins too. Signed-off-by: Hayden B <[email protected]> * add dismissStaleReviews Signed-off-by: cpanato <[email protected]> Signed-off-by: Hayden B <[email protected]> Signed-off-by: cpanato <[email protected]> Co-authored-by: cpanato <[email protected]>

tekton-robot requested review from imjasonh and kimsterv July 18, 2020 10:06

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 18, 2020

bobcatfish reviewed Jul 20, 2020

View reviewed changes

pritidesai force-pushed the tep-final-task-results branch 3 times, most recently from e19f6dd to 90b4a0b Compare July 20, 2020 23:54

vdemeester reviewed Jul 21, 2020

View reviewed changes

tekton-robot assigned vdemeester Jul 21, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020

bobcatfish reviewed Jul 21, 2020

View reviewed changes

pritidesai force-pushed the tep-final-task-results branch from 90b4a0b to 881e4c5 Compare July 21, 2020 22:25

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020

Adding TEP for task results in final tasks

2138b9d

Proposal for enabling final tasks to get value from non-final tasks.

pritidesai force-pushed the tep-final-task-results branch from 881e4c5 to 2138b9d Compare July 21, 2020 23:06

vdemeester reviewed Jul 22, 2020

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2020

tekton-robot merged commit 0919d59 into tektoncd:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding TEP for task results in final tasks #153

Adding TEP for task results in final tasks #153

pritidesai commented Jul 18, 2020

bobcatfish left a comment

bobcatfish Jul 20, 2020

pritidesai commented Jul 21, 2020

pritidesai commented Jul 21, 2020 •

edited

Loading

vdemeester commented Jul 21, 2020

vdemeester left a comment

vdemeester Jul 21, 2020

vdemeester Jul 21, 2020

vdemeester Jul 21, 2020

pritidesai Jul 21, 2020

bobcatfish commented Jul 21, 2020

bobcatfish commented Jul 21, 2020

bobcatfish left a comment

bobcatfish Jul 21, 2020

pritidesai Jul 21, 2020 •

edited

Loading

bobcatfish Jul 22, 2020

bobcatfish Jul 21, 2020

pritidesai Jul 21, 2020 •

edited

Loading

pritidesai Jul 21, 2020 •

edited

Loading

bobcatfish Jul 21, 2020

pritidesai Jul 21, 2020

vdemeester commented Jul 21, 2020

pritidesai commented Jul 21, 2020 •

edited

Loading

vdemeester left a comment

bobcatfish commented Jul 22, 2020

tekton-robot commented Jul 22, 2020


		* No API changes.

		* Finally contract does not break and all final tasks are attempted.

		However if the `Task` being attempted has a default value for that parameter, it can still run,
		using the default when result is not available.


		A.

		```

Adding TEP for task results in final tasks #153

Adding TEP for task results in final tasks #153

Conversation

pritidesai commented Jul 18, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Jul 21, 2020

pritidesai commented Jul 21, 2020 • edited Loading

vdemeester commented Jul 21, 2020

vdemeester left a 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

bobcatfish commented Jul 21, 2020

bobcatfish commented Jul 21, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

pritidesai Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Jul 21, 2020

pritidesai commented Jul 21, 2020 • edited Loading

vdemeester left a comment

Choose a reason for hiding this comment

bobcatfish commented Jul 22, 2020

tekton-robot commented Jul 22, 2020

pritidesai commented Jul 21, 2020 •

edited

Loading

pritidesai Jul 21, 2020 •

edited

Loading

pritidesai Jul 21, 2020 •

edited

Loading

pritidesai Jul 21, 2020 •

edited

Loading

pritidesai commented Jul 21, 2020 •

edited

Loading