-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: add allowFailure option for workflow steps #6114
Conversation
3a14b9e
to
8b31bf5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
8b31bf5
to
da2bff8
Compare
@stefreak: Got the errors resolved and the builds successful. Thank you! |
da2bff8
to
4dd9c17
Compare
60ef3f8
to
8d2468b
Compare
@stefreak : I've updated the PR based on the discussion from the issue. Could you please do another review when a chance is afforded? |
8d2468b
to
f9c9e69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on this! I think adding an allowFailure
flag on a step is a great idea, however after a longer discussion @stefreak and I figured that in the current format the distinction between onFailure
and onError
is a bit difficult to understand. This is not due to your implementation, which is great. Some more suggestions on how to proceed in another comment soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-kattathra-johnson Wow, great job, really amazing how you drilled down into the workflow code, and your code looks like it behaves exactly as proposed by @salotz in #6077
During deeper review @twelvemo and me took the time to think again about the user perspective, and while initially I thought adding onFailure
is a good idea, it turns out to make workflows harder to explain and workflow code harder to read.
Our proposal is the following:
- Let's go back to the older version of the PR where you only added
allowFailure
(And possibly rename that tocontinueOnError
. Let's keep the added tests from the current version 👍 - Let's open an issue and postpone the
onFailure
handler to another PR; What we think might be ideal is something like the following, but implementation of that is too complex to shove it into this PR.
kind: Workflow
name: test
steps:
- name: allowed_to_fail
script: this_will_fail
continueOnError: true
- if: ${steps.allowed_to_fail.outcome == "error"}
script: echo "Step 1 failed, let's clean it up"
Sliced the PR with the Occam's razor! 😄 |
912e650
to
56e6b9a
Compare
56e6b9a
to
c4c521f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for taking the time to rework it again!
c4c521f
to
ee1caf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twelvemo and me made some additional changes to the logging and error handling behaviour. Looks good to go now, if the tests pass
* do not fail the entire workflow if a step labeled with allowFailure throws an error Signed-off-by: Alex Johnson <[email protected]>
3bd0f47
to
24059cd
Compare
Co-authored-by: Anna Mager <[email protected]>
Fixes #6077