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

feat: add allowFailure option for workflow steps #6114

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

alex-kattathra-johnson
Copy link
Contributor

@alex-kattathra-johnson alex-kattathra-johnson commented May 31, 2024

Fixes #6077

@stefreak

This comment was marked as resolved.

@alex-kattathra-johnson
Copy link
Contributor Author

@stefreak: Got the errors resolved and the builds successful. Thank you!

@stefreak stefreak self-requested a review June 3, 2024 13:21
@stefreak stefreak changed the title Add an allowFailure field to WorkflowStepSpec feat: add allowFailure option for workflow steps Jun 3, 2024
@alex-kattathra-johnson
Copy link
Contributor Author

@stefreak : I've updated the PR based on the discussion from the issue. Could you please do another review when a chance is afforded?

Copy link
Collaborator

@twelvemo twelvemo left a 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.

core/src/config/workflow.ts Outdated Show resolved Hide resolved
Copy link
Member

@stefreak stefreak left a 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:

  1. Let's go back to the older version of the PR where you only added allowFailure (And possibly rename that to continueOnError. Let's keep the added tests from the current version 👍
  2. 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"

core/src/commands/workflow.ts Outdated Show resolved Hide resolved
core/src/config/workflow.ts Outdated Show resolved Hide resolved
core/src/config/workflow.ts Outdated Show resolved Hide resolved
@alex-kattathra-johnson
Copy link
Contributor Author

alex-kattathra-johnson commented Jun 13, 2024

Sliced the PR with the Occam's razor! 😄

twelvemo
twelvemo previously approved these changes Jun 17, 2024
Copy link
Collaborator

@twelvemo twelvemo left a 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!

stefreak
stefreak previously approved these changes Jun 18, 2024
Copy link
Member

@stefreak stefreak left a 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

@stefreak stefreak requested a review from twelvemo June 18, 2024 13:21
* do not fail the entire workflow if a step labeled with
  allowFailure throws an error

Signed-off-by: Alex Johnson <hello@alex-johnson.net>
@stefreak stefreak added this pull request to the merge queue Jun 18, 2024
Merged via the queue into garden-io:main with commit 70b8ca8 Jun 18, 2024
17 checks passed
@alex-kattathra-johnson alex-kattathra-johnson deleted the issue-6077 branch June 18, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: allow failure for workflow tasks
3 participants