update log trailer msg for onError:continue

[LyndseyBu]

Changes

When onError:continue is used in a step:
Update the log trailer message to indicate if the step completed successfully. Or if the step completed with a non 0 exit code

Storybook has also been updated to include the new state for log messages

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide
for more details.

[solsson]

@LyndseyBu What do you think about a visual change to the step list (in TaskRuns) as well? For example when exitCode != 0 then reason=Completed would become yellow instead of green checkmark, or a different icon.

[AlanGreene]

@solsson We actually had that implemented originally (yellow warning icon on the step) but received strong feedback that it was unexpected / confusing since the underlying status as recorded/reported by Tekton is still success.

With the onError: continue config on a step, the author is explicitly stating that this step is allowed to fail and should not affect the status of the Task / Pipeline, so that is how it's presented.

For TEP-0050 (which is still 'proposed') it may make sense to decorate these cases with a warning icon of some kind on the TaskRun and possibly even the PipelineRun, but that would depend on the behaviour agreed if/when the TEP is marked as implementable.

[solsson]

Of course the use cases for onError:continue will vary. The distinction between the author and the user is interesting. I have no opinion on TEP-0050. Based on our experience (we opted in to onError:continue well before tektoncd/pipeline#4251), I would argue that as it works now only the author can figure out that they should look at the logs of the previous step. A developer will see that the build failed, fiind the TaskRun that failed and post a message to the team containing the logs from the next step 🙂

Personally I think yellow warning icon was a great idea, but feedback is feedback. Could there be a different symbol, like green but with an arrow down? Or could the next step have an arrow up?

Also the text in tkn--status-message probably needs to adapt after #4251. In our case the step after onError:continue will communicate the error to developers, then exit 1. The status message rightly says so, but should probably enumerate the other nonzero steps as "exited with code 1".

[AlanGreene]

the use cases for onError:continue will vary

💯 %, and unfortunately it's very difficult to handle them all in a consistent manner. There's been some discussion about potentially expanding the onContinue: error functionality in future to allow steps to determine at runtime whether a particular error is safe to ignore or should be considered fatal and stop the run.

A developer will see that the build failed, fiind the TaskRun that failed and post a message to the team containing the logs from the next step

The purpose of the onError: continue (as implemented) is to prevent the step failing the TaskRun if it exits non-zero so I'm not sure I understand what you're describing here. If you have some custom logic in a subsequent step that detects the failure and exits non-zero to fail the run this is not something we can detect / respond to as not everyone will have the same opinion on which (if any) of the potentially multiple previous steps that exited non-zero are of interest.

In the meantime I would suggest either including the relevant info in the logs of the step you're using to fail the run, or that maybe onError: continue isn't the correct approach here or a different structure (e.g. extracting some of the steps to separate Tasks) might make more sense depending on your specific use case.

I'll merge this PR as it adds some additional useful info to alert users to the fact that something different happened, and it doesn't prevent us surfacing this or similar states in a more obvious fashion (via different / additional icons etc.) separately as discussion on the current and future features evolve. Thanks for the feedback! 😸

[AlanGreene]

Looks great, thanks @LyndseyBu. One minor change needed and this is good to go.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlanGreene

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:

• OWNERS [AlanGreene]

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

[AlanGreene]

/lgtm