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.

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

Improve description of workflow.failOnIgnore #5119

Closed
wants to merge 1 commit into from
Closed

Improve description of workflow.failOnIgnore #5119

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 docs/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1780,7 +1780,8 @@ The `workflow` scope provides workflow execution options.
`workflow.failOnIgnore`
: :::{versionadded} 24.05.0-edge
:::
: When `true`, the pipeline will exit with a non-zero exit code if any failed tasks are ignored using the `ignore` error strategy.
: When `true`, the pipeline will exit with a non-zero exit code if any failed tasks are ignored using the `ignore` error strategy.
: This '`fail`' differs from the default `fail` `errorStrategy` in that the former allows all other possible tasks to be executed and complete before the pipeline fails, whereas the latter will prevent any new tasks from being started from the point of the failed task.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is correct. The main (and only) difference is that that pipeline completes with a -1 exit status instead of 0, there's no change how the tasks are executed.

Copy link
Contributor Author

@jfy133 jfy133 Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. I think that confirms my point. To me with the current docs it's not clear exactly what the behaviour is 😬. If the exit code is justb different, what is the difference with the standard terminate erorrStrategy?

Would it be better to say something like:

'Allows the ignore errorSrategy to be respected, but still indicates a pipeline failure at the end of the run'?

My understanding of the discussions on slack was the combination of ignore and this new option (with ignore) was to allow the pipeline to otherwise 'get as far as it can' rather than killing the entire run...

If just the exit code is different, the purpose of this should be described, otherwise I don't think it's very clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively this guidance could be moved to the errorStrategy docs under ignore with an info box saying

'if you would still like the pipeline to continue even in the presence of failed tasks, but still ultimately have the pipeline fail at the end, see workflow.failOnIgnore'

Or something similar.

I guess that make more sense as the docs on this current page appears to be more terse.

Would that make more sense?

Copy link
Member

Choose a reason for hiding this comment

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

what is the difference with the standard terminate erorrStrategy?

terminate: the execution is stopped as soon as an error is encountered. ignore: any process error is just ignore, the execution continues until there's something to be run, the workflow execution completes with 0 exit status. When setting failOnIgnore=true, the workflow execution completes with -1.

Think it's fine keep it here better specifying the -1 exit status for the workflow execution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamrtalbot @pinin4fjords And I came to a similar conclusion independently now, to leave this as it's but better describe the use case (with the different options you described above) but in the erorrStrategy page, linking back here. Will close this over Adam's PR he will make in a bit 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

#5121


(config-miscellaneous)=

Expand Down
Loading