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.

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

TEP-0048: task results without results - problem statement #240

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

pritidesai
Copy link
Member

@pritidesai pritidesai commented Oct 22, 2020

Listing use cases and problem statement for task results without results based on the latest API changes.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2020
@pritidesai pritidesai force-pushed the task-results branch 2 times, most recently from 0472685 to 18dd2e0 Compare October 22, 2020 07:42
Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up @pritidesai !

I think this is pretty reasonable (with a small tweak that id rather call the field something like "default" than "value") (tho I really like @chhsia0's alternative as well!!)

I'd also like to explore another alternative (I added a comment with more details): what if we provide the entire status of the pipelinerun on disk in a file for these use cases? These Tasks seem to want to:

  1. Run regardless of the skipped + failure status of the non-finally Tasks
  2. Want to access results from one or more Tasks (if they succeeded)
  3. Probably also want to look at the execution status of one or more Tasks

If these are true, I'm not sure that simple variable substitution will be enough to support them, i.e. these Tasks will probably be highly bespoke, and it might work better to give them a file on disk to introspect

it might help to look examples of one or more of these Tasks (the boskos Tasks might be too simple)

value: $(params.second)
results:
- name: sum
value: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. could we call this something like default instead of value? I would think that value would override the result created by the Task, but it sounds like what we want is to use this only in the case where the Task fails or is skipped (or does not produce a result?)
  2. "or does not produce a result?" --> if the Task succeeds but does not produce a result, is this default used? (I suggest in the first iteration we don't support that)

Copy link
Member Author

Choose a reason for hiding this comment

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

yup sure, lets call it default instead of value. Yes use this default value when a task does not produce a result.

We have an issue to declare such task (does not produce a result) failure: tektoncd/pipeline#3497. This fix will prevent executing any other dag tasks and no need for any defaults. But finally tasks are still executed and you suggest not using default in finally as well?

This example shows that `B` is guarded with task results from `A` where `A` was skipped but is it really a possible
scenario? When this `pipeline` says continue executing `B` even though `A` was skipped, why would `B` be guarded with a
task result from `A`?

Copy link
Contributor

Choose a reason for hiding this comment

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

another alternative would be to move the "default" specification into the "params" section in the pipelineTask instead, e.g.:

spec:
  params:
    - name: owner-name
      default: "the-best-owner"
  tasks:
   - name: boskos-acquire
     taskRef:
       name: boskos-acquire
     params:
       - name: owner-name
         value: $(params.owner-name)
   - name: do-stuff-with-resource
     taskRef:
       name: do-stuff-with-resource
     params:
       - name: resource-name
         value:  $(tasks.boskos-acquire.results.leased-resource)
 finally:
   - name: boskos-release
     taskRef:
       name: boskos-release
     params:
       - name: leased-resource
         value:  $(tasks.boskos-acquire.results.leased-resource)
         default: "fake-awesome-project"

if we wanted to make this work for when expressions, we'd need some way of expressing defaults in when expressions as well which is the big downside of this approach

one upside of this approach tho is that it lets the consumer of the value decide what to do with it vs. having the producer understand that the consumer still wants to run

Copy link
Contributor

Choose a reason for hiding this comment

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

another alternative would be to provide/require a completely different way of accessing results for the use cases above

i.e. if you want to look at a result value, and you want to run regardless of the state of the producing task (i.e. even if failed, skipped, etc.), we provide another way to provide this info, e.g.:

  1. write a file to disk with the status of the entire pipelinerun
  2. provide variable replacement for the entire pipelinerun status

i think that providing something like (1) might be a reasonable alternative here; i have a feeling that the tasks that we want to create for the use case above might be more complex than just the boskos example we are including

Copy link
Contributor

Choose a reason for hiding this comment

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

@chhsia0 also mentioned another interesting option in the working group today which i think it worth exploring: introducing a syntax for the variable replacement that allows a default to be specified, e.g.:

$(tasks.boskos-acquire.results.leased-resource | fake-awesome-project)

I really like that this option would let us both:

  1. Let the consumer of the value decide how to handle the case where it is not provided
  2. Work for when expressions AND params

However if we go that route i think we'd also want to take into account #1393 i.e. what would this syntax look like in a world where we have more types than just strings and can do stuff like array indexing

Copy link
Contributor

Choose a reason for hiding this comment

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

I also liked the idea of expressing a default value in the variable substitution syntax itself, especially if it looks somewhat similar to Bash syntax, e.g. $(tasks.boskos-acquire.results.leased-resource:-fake-awesome-project}. This is a well-known feature of shell scripting so people would just "get it" without a lot of explanation required. I guess the downside is that you need to specify the default on every reference as opposed to having a single place where you could set it as in Priti's proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

another alternative would be to move the "default" specification into the "params" section in the pipelineTask instead

Yup, this can also allow specifying separate defaults for each reference, but downside with when expressions is significant as well. How would we allow having defaults with when expressions then?

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you for the suggestion, have included as one of alternatives

Copy link
Member

Choose a reason for hiding this comment

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

I liked the proposed options. But also defaulting on the producer and consumer side do not have to be orthogonal.
Defaulting a result would be an helper for task author to ensure a result is always provided, that the author has enough context in the task to be able to do that meaningfully.
Defaulting a variable value would be a helper the task author (or runner) to provide a default value for situations where the pipeline / pipelinerun context is required to provide a meaningful default.
Both could be implemented as separate features.

scenario? When this `pipeline` says continue executing `B` even though `A` was skipped, why would `B` be guarded with a
task result from `A`?

## References
Copy link
Contributor

Choose a reason for hiding this comment

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

another alternative would be to provide/require a completely different way of accessing results for the use cases above

i.e. if you want to look at a result value, and you want to run regardless of the state of the producing task (i.e. even if failed, skipped, etc.), we provide another way to provide this info, e.g.:

  1. write a file to disk with the status of the entire pipelinerun
  2. provide variable replacement for the entire pipelinerun status

i think that providing something like (1) might be a reasonable alternative here; i have a feeling that the tasks that we want to create for the use case above might be more complex than just the boskos example we are including

Copy link
Member Author

Choose a reason for hiding this comment

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

(1) is being tracked here I think we still have a use case for that feature and will have a separate TEP for it.

But in this context, (1) again adds extra responsibility of parsing metadata to consumer task and does not work with when expressions.


A minor alternative to the proposal in which specifying `results` default values is limited to `optional` task `params`,
instead, allow specifying default values for both `required` and `optional` task `params`. And skip the task when
default values are not specified while results are missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the reason to not allow the default result value to be used with a required parameter? The pipeline author might be trying to reuse a task that does not have a default for a param value. It seems to me like the pipeline author should be able to pass the default result value anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was adhering to the same concept of required v/s optional params. Dropped this limitation.

@afrittoli afrittoli changed the title adding tep for task results without results TEP-0028 adding tep for task results without results Nov 16, 2020
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thank you for this!
This will add great value to Tekton usability!

The main comment that I have is that I feel that we should not restrict this to optional task parameters; that seems to be the least interesting use case to me. Mandatory task params is where I see most of the value.

teps/0028-task-results-without-results.md Outdated Show resolved Hide resolved
teps/0028-task-results-without-results.md Outdated Show resolved Hide resolved

### Non-Goals

This proposal is not offering a solution without any use case.
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 I understand this 🙏


### Goals

Identify a set of use cases for `finally` and `continueAfterSkip` without task results and design a common solution
Copy link
Member

@afrittoli afrittoli Nov 19, 2020

Choose a reason for hiding this comment

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

I feel the goal could be more generic about default value for results, and finally and continueAfterSkip could go into use cases? Even more so since continueAfterSkip is not a feature we support yet.

teps/0028-task-results-without-results.md Outdated Show resolved Hide resolved

## Proposal

Enable `pipeline` author to specify a default value for a `task result` in case of a missing result. Pipeline author
Copy link
Member

Choose a reason for hiding this comment

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

+1
I agree the default value here should be controlled by the pipeline author and not by the task author

teps/0028-task-results-without-results.md Outdated Show resolved Hide resolved
tasks in a `pipeline`. `params` in a `task` can be declared as required and optional. Required `params` does not allow
any defaults and have to be initialized at the `pipeline` or `pipelineRun` level.

The proposal is designed on the similar lines, when a `param` is declared `optional` at the `task` level and referring to the
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 afraid I don't understand the reasoning behind this.
If a parameter is declared as optional in the Task, by definition it is acceptable not to pass any value to it.

Optional parameters are those least in need of a default, because if the expression injected by the pipeline depends on something not available (like a result), the pipeline could simply not pass any value, and it would work, because it's optional.

Mandatory parameters instead cause a Task failure if not passed. If the pipeline author binds a mandatory task parameter to a result from another task, we need a way for the pipeline author to supply a default value (if they want) in case the result in question is not available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Optional parameters are those least in need of a default, because if the expression injected by the pipeline depends on something not available (like a result), the pipeline could simply not pass any value, and it would work, because it's optional.

Optional param at the task level referring to task result in a Pipeline is considered as required i.e. if a task result is not initiated, pipeline stops executing even if a referenced param was optional at the task level.

value: $(task.add.results.sum)
```

No default for a `required` task param (referring to a task result) allowed. If a task result is missing, `pipeline`
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 afraid I don't understand the reason behind this restriction.

It should be up to the pipeline author whether to specify the default or not.
Assuming the pipeline about, you could have the following combinations:

x param type result default multiply execution
mandatory supplied executed
mandatory not supplied skipped
optional supplied executed
optional not supplied executed

teps/0028-task-results-without-results.md Outdated Show resolved Hide resolved
@afrittoli
Copy link
Member

/test pull-community-teps-lint

@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Nov 23, 2020

#### Story 2

Need user story for `finally` task with optional `param` referring to a task result.
Copy link
Contributor

Choose a reason for hiding this comment

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

Pipeline that scans code and packages within a container image.
The pipeline fails if the scan does not pass a certain threshold
The task still collects and publishes results even if the scan task fails by outputting the scan summary to a result string
A finally task would consume the task result on success and failure and publish that information to an external consumer for review.

@pritidesai pritidesai force-pushed the task-results branch 2 times, most recently from 9bd8a3c to 1cac70b Compare January 23, 2021 01:05
@bobcatfish
Copy link
Contributor

/assign @jerop

( @afrittoli would you like to be assigned as a reviewer for this one?)

@afrittoli
Copy link
Member

/test pull-community-teps-lint

@bobcatfish
Copy link
Contributor

/assign

@pritidesai pritidesai force-pushed the task-results branch 2 times, most recently from 5d24357 to 3340701 Compare February 2, 2021 22:39
Base automatically changed from master to main February 3, 2021 16:34
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

The one thing I have trouble wrapping my head around is the use of results as field. It's similar in name and "shape" with the results field for Tasks and Pipelines, but it doesn't have the same behaviour, it is a "failsafe" for result to have a value if the task failed.
Maybe we should use another name ?

Also, I am a bit fuzzy on what is the limitation for Option F with the when expression 🤔

+ This works well without having task results default. The `param` in the task has full control over where its value
coming from and what is the default if it's not initialized.
+ Works with `continueAfterSkip` feature.
- Cannot be extended to `when` expressions since task result in `when` expression is not mapped to any `param` in the
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand this one 🤔


+ This alternative allows task result consumer to decide the default value based on its use case v/s task
result producer defining common default for all consumers.
- Not compatible with `when` expression.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add more detail on why it's not compatible ? I'm not sure I understand 😝

Comment on lines 277 to 279
results:
- name: sum
value: 0
Copy link
Member

Choose a reason for hiding this comment

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

What happens if add-task define a result name sum and what happens if add-task doesn't define a result name sum ?

Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

Finally got a chance to take a look!! My main feedback is that it's hard to weigh the options without having some really compelling use cases - is there any way we can gather some more (and/or add more detail to the existing ones) (maybe you have some more in mind that are motivating this TEP @pritidesai ?)

#### Story 1

For a cleanup use case, `finally` task receives the project name, or the configuration name as a `task result` from
the `acquire` task. If `acquire` task fails to acquire any resources, `cleanup` is skipped.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think there's some detail missing here - it seems like if the resources weren't acquired, you don't want to do the cleanup?

params:
- name: imageRef
value: "$(tasks.build-image.results.builtImage)"
```
Copy link
Contributor

Choose a reason for hiding this comment

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

the example ALMOST makes sense to me but a couple things i think might be missing:

  • The name of the image returned by build-image seems like it would be targetImage? If so it doesnt seem like deploy-image actually cares about the result of build-image, this seems more like a case for wanting to apply the when expression to only build-image

I also don't totally understand the context of this use case - why do we want to build an image only if it doesn't exist? (is the image getting periodically deleted? or is it like built once a day and named after the day?) any more details you can provide might help

value: $(params.owner-name)
results:
- name: leased-resource
value: "fake-awesome-project"
Copy link
Contributor

Choose a reason for hiding this comment

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

i think using value here doesn't quite imply the defaulting behavior we're adding - maybe default ?

Pros/Cons:

- Consumer task has no control over default values.
- Task consuming such default value cannot identify whether its an invalid value or a default value.
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont understand why that is, can you explain more?


Instead of specifying defaults for task results, leverage task `param` default value. When the task fails and the `param`
referring to task results has a default, use that `param` default. If that `param` is required and no default specified,
skip the task.
Copy link
Contributor

Choose a reason for hiding this comment

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

could you include an example?

### Option C

No defaults allowed (neither in the `task` producing results nor in the `pipelineTask` for the `task` producing results),
just skip `finally` task with a reason when a task result is missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the current functionality?

The `cleanup` is necessary when the resources are acquired but not needed if the acquisition fails. This option is
proposing introducing `when` expression to `finally`. `Cleanup` task in `finally` checks the execution status of
`acquire` task, and guards `cleanup` if `acquire` failed. This proposal helps avoid dealing with missing task results
in `finally` i.e. no default or no zero values for task results needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont totally follow this one, maybe an example would help?

This example shows that `B` is guarded with task results from `A` where `A` was skipped but is it really a possible
scenario? When this `pipeline` says continue executing `B` even though `A` was skipped, why would `B` be guarded with a
task result from `A`?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another proposal that came up (I'm not sure where?? i feel like @chhsia0 was involved but not sure) is to introduce a defaulting syntax when using the variable interpolation for results, e.g. something like:

$(tasks.foo.results.bar?somedefault) (or maybe instead of somedefault we require reference to another var?)

I might be kinda crazy but i actually like this option b/c it puts the control completely on the consuming PipelineTask (like option b) but also works for when expressions

if we go this route hopefully we could borrow the syntax from somewhere else

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 -- just wondering how it'd look like if the default was a variable

would it be $(tasks.foo.results.bar?$(params.someDefault)) 🙃 -- maybe $(tasks.foo.results.bar?params.someDefault) is better?

but then how would this look like if we add support for array results proposed in #477?

(back to focusing on the problem statement only lol)

```


## Design Details
Copy link
Contributor

Choose a reason for hiding this comment

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

can we include pros and cons for the proposed option? (seems like we have them for the alternatives)

@afrittoli
Copy link
Member

/assign @afrittoli

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2021
value: $(task.add.results.sum)
```

### User Stories
Copy link
Member

Choose a reason for hiding this comment

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

I have a user story that seems to apply to this TEP as I'm hoping it would enable the flow I desire.

I have a pipeline that conditionally forks between two different tasks. Both of the tasks provide a result of APP_IMAGE_DIGEST. Now within my pipeline I'd like to propagate that result as the result of the pipeline.

Currently, I can't find a way to achieve what I'm desiring. The proposed solution should allow me to execute a consolidation task on the pipeline that can check to see which task results are present and set those as the Pipeline results. Is my thinking correct?

  Pipeline
                       │
 ┌─────────────────────┼──────────────────────────┐
 │                     │                          │
 │               ┌─────▼──────┐                   │
 │               │            │                   │
 │               │  git-clone │                   │
 │               │            │                   │
 │               └─────┬──────┘                   │
 │                     │                          │
 │       ┌─────────────┴───────────────┐          │
 │       │        (condition)          │          │
 │ ┌─────▼──────┐           ┌──────────┴────────┐ │
 │ │            │           │                   │ │
 │ │ buildpacks │           │ buildpacks-phases │ │
 │ │            │           │                   │ │
 │ └─────┬──────┘           └──────────┬────────┘ │
 │       │                             │          │
 │       └──────────────┬──────────────┘          │
 │     results          │           results       │
 │                      │                         │
 └──────────────────────┼─────────────────────────┘
                        │
                  ┌─────▼─────┐
                  │           │
                  │  results  │
                  │           │
                  └───────────┘

Copy link
Contributor

Choose a reason for hiding this comment

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

ah interesting - so you want to use the result of one of those two build tasks in the last task? it does sound like this proposal should help with the consolidation task (im wondering if being able to make sub pipelines might help you too - but i think you'd still need the consolidation task + @jerop )- i think we've also had feature requests for better support for the specific scenario you're describing as well (e.g. choose which of several tasks to run vs. just gating them) but we've never pursued it

@tekton-robot
Copy link
Contributor

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.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 7, 2021
Adding TEP with problem statement to evaluate the need for task results
 without results.
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2021
@pritidesai pritidesai changed the title TEP-0028 adding tep for task results without results TEP-0048: task results without results - problem statement Jun 11, 2021
@pritidesai
Copy link
Member Author

pritidesai commented Jun 11, 2021

The recent comments were raising concerns over the use cases. I have limited the content of this PR to first evaluate the use cases and problem statement. Once we agree on the problem statement, I will add the proposal section back.

/remove-lifecycle stale

Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

Thanks for adding the use cases! I have a tweak I'd like to propose for one of the use cases but overall lgtm :)

/approve


Further, extending the same use case, when someone approves the PR, the `approver` would be set to an appropriate
name. At the same time, set the task result `approver` to **None** in case the `manual-approval` task is skipped and
the `approver` is not initialized. It is still possible to send a notification that no one approved the PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm interesting - in TEP-0059 we were assuming that the user would WANT to skip the manual approval entirely - including the notification. for example, if this was running against a pull request, would you want to send a slack notification every time the PR is updated? maybe but probably not (which is what i understand to be the premise of the use case in TEP-59)

maybe we can tweak the example a bit to make it work here - in this case, where we're running the pipeline on a PR, maybe we need to start a temporary cluster to deploy to? (since we don't want to deploy to prod)

        lint                     unit-tests
         |                           |
         v                           v
 report-linter-output        integration-tests
                                     |
                                     v
                               manual-approval
                               |            |
                               v        (approver)
(if pull-request)          build-image       |
 set-up-cluster                |             v
        |                      |        slack-msg
        v                      |
 (cluster ip, etc.).           |
        |                      v
        -------------->   deploy-image

In this example, when we run the pipeline for a pull request, we want to setup a cluster and pass the info about that cluster to deploy-image, but when we want to deploy to prod, we want to use params that are passed in with the prod cluster

(this is a bit flawed b/c you probably need more than just the ip and a string result might not be the best way to pass that - but maybe we could imagine in a world with dict params + results this makes a bit more sense?)

so we'd need to be able to express that when set-up-cluster is skipped, instead of trying to use it's results and skipping deploy-image (which depends on set-up-cluster), we use the params

Copy link
Contributor

Choose a reason for hiding this comment

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

(or we could remove it, the next 2 use cases have it covered!)

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 a useful use case, where the default results are variables that users would need to pass in as parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, you have precisely captured a tweak here with the use case from TEP-0059.

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2021
@jerop
Copy link
Member

jerop commented Jun 28, 2021

/remove-lifecycle stale

@tekton-robot tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2021
- name: APP_IMAGE
value: $(tasks.propagate-image-name.results.image)
```

Copy link
Contributor

@ScrapCodes ScrapCodes Jul 9, 2021

Choose a reason for hiding this comment

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

Is it correct to think, another possible use case can be, for resuming failed pipelines.

The tasks that ran fine can be somehow disabled (either with some future TEP or when expression), and their results pre-populated, while copying and creating a new Pipeline/PipelineRun in order to retry/resume.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's an interesting use case for this @ScrapCodes 🤔

my feeling is that in the resuming a pipeline case, usually youd want to use the result value produced by the previous run, and not the default value - tho maybe sometimes you would?

for example, say you were trying to do a build + deploy:

spec:
  tasks:
    - name: build-image
      taskRef:
        name: build-image
      results:
        - name: image-name
    - name: deploy-image
      params:
        - name: image-name
          value: "$(tasks.build-image.results.image-name)" # using the image that was built by build-image
      taskRef:
        name: deploy-image

Maybe deploy-image failed, and you want to run the pipeline again, but you don't want to have to build again, you probably want to use the value of tasks.build-image.results.image-name from the previous run.

If you used the functionality proposed here, you'd be using some default value for the image-name instead of the value that was just built. For example, imagining a syntax for specifying a default to use:

spec:
  tasks:
    - name: build-image
      taskRef:
        name: build-image
      results:
        - name: image-name
    - name: deploy-image
      params:
        - name: image-name
          value: "$(tasks.build-image.results.image-name?my-website:v0.4.0)"
      taskRef:
        name: deploy-image

Let's say this built an image my-website:v0.7.0, but deploy-image failed. If you wanted to re-run, i.e. partially run the pipeline with build-image disabled, I'm guessing you'd want to pass my-website:v0.7.0 (the one you just built) and not use the default my-website:v0.4.0

Probably there are cases for both tho!

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

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

compelling problem statement, thanks @pritidesai 😁

This example shows that `B` is guarded with task results from `A` where `A` was skipped but is it really a possible
scenario? When this `pipeline` says continue executing `B` even though `A` was skipped, why would `B` be guarded with a
task result from `A`?

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 -- just wondering how it'd look like if the default was a variable

would it be $(tasks.foo.results.bar?$(params.someDefault)) 🙃 -- maybe $(tasks.foo.results.bar?params.someDefault) is better?

but then how would this look like if we add support for array results proposed in #477?

(back to focusing on the problem statement only lol)


Further, extending the same use case, when someone approves the PR, the `approver` would be set to an appropriate
name. At the same time, set the task result `approver` to **None** in case the `manual-approval` task is skipped and
the `approver` is not initialized. It is still possible to send a notification that no one approved the PR.
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 a useful use case, where the default results are variables that users would need to pass in as parameters

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Nice, looking forward to this! Thank you @pritidesai @jerop
/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish, jerop

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pritidesai
Copy link
Member Author

As per the new policy, we have approvals from all the assignees, adding lgtm during the API WG
/lgtm

@tekton-robot
Copy link
Contributor

@pritidesai: you cannot LGTM your own PR.

In response to this:

As per the new policy, we have approvals from all the assignees, adding lgtm during the API WG
/lgtm

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.

@jerop
Copy link
Member

jerop commented Aug 2, 2021

adding lgtm because @pritidesai (facilitator in API WG) can't lgtm her own pr

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2021
@tekton-robot tekton-robot merged commit e9c4bf7 into tektoncd:main Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

10 participants