Skip to content

fix(task): task run finished not consistent #304

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.

Sign up for GitHub

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

Open
xibz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(task): task run finished not consistent #304

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion schemas/taskrunfinished.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@
]
},
"outcome": {
"type": "string"
"type": "string",
"enum": ["success", "failure", "cancel", "error"]
Copy link
Copy Markdown
Member

@afrittoli afrittoli Mar 26, 2026

Choose a reason for hiding this comment

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

Tests typically have assertions, which may succeed or fail, and in case the tests fails to execute before the assertion is evaluated, that's considered an error. How does that model apply to task runs?

Copy link
Copy Markdown
Contributor Author

@xibz xibz Mar 26, 2026

Choose a reason for hiding this comment

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

The same as how it applies to pipeline run. If pipeline run has it as enum, this should probably be an enum was my thinking

Copy link
Copy Markdown
Member

@afrittoli afrittoli Mar 26, 2026

Choose a reason for hiding this comment

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

Oh, I see, it was added in #238.... well, I'm ok to make things consistent, but only if there's a meaning - I guess I already expressed by concerns at the time in #237.

I'm fine to keep the two type of failures for TaskRuns and PipelineRuns if there is a meaning to it.
For Tekton I wouldn't know which one to use. I guess we could use one for validation errors (so it didn't run at all) and another one for runtime errors, although the distinction is arbitrary as validation error, when using dynamic values in matrices and results, can also happen at runtime.

As longs as there's clear guidance for the meaning I'm ok. If not I would rather remove it from PipelineRuns.

Copy link
Copy Markdown
Contributor Author

@xibz xibz Mar 26, 2026

Choose a reason for hiding this comment

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

Yes, and that is a very important concern. The idea (my plan) is to add these to the _defs so it is very clear on what these mean. But holding off, until we define the subjects that encompass this. Ill create an issue for that, if that helps

Copy link
Copy Markdown
Contributor

@davidB davidB Mar 31, 2026

Choose a reason for hiding this comment

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

Can the _defs could also explain how to map skip, ignore (currently I convert them into cancel, but maybe it's not the good solution).

Copy link
Copy Markdown
Contributor Author

@xibz xibz Mar 31, 2026

Choose a reason for hiding this comment

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

Can the _defs could also explain how to map skip, ignore (currently I convert them into cancel, but maybe it's not the good solution).

They need to. I understand the want to convert some of the fields. When writing the java SDK, I made a bunch of changes as well for a better DX. I also felt like it wasn't the best solution. Maybe we should sync up and see how we can address these issues in the schemas cause I identified so many areas for improvements

},
"errors": {
"type": "string"
Expand Down