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

Report action cleanup errors to the user & various cleanup fixes for recipe action #301

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

obbardc
Copy link
Member

@obbardc obbardc commented Jan 5, 2022

Draft as depends on #303

@obbardc obbardc marked this pull request as draft March 9, 2022 11:33
@obbardc

This comment was marked as outdated.

@obbardc obbardc added this to the v1.1.0 milestone Apr 14, 2022
@obbardc obbardc removed this from the v1.1.0 milestone Apr 27, 2022
@obbardc obbardc added this to the v1.1.2 milestone Oct 12, 2022
@obbardc obbardc force-pushed the wip/obbardc/fix-recipe-cleanup branch from 4d83abc to e711d59 Compare November 2, 2022 18:35
@obbardc

This comment was marked as outdated.

@obbardc obbardc closed this Jul 19, 2023
@obbardc obbardc reopened this Jul 26, 2023
@obbardc obbardc force-pushed the wip/obbardc/fix-recipe-cleanup branch 4 times, most recently from 4c5d534 to db61042 Compare July 26, 2023 15:05
@obbardc obbardc modified the milestones: v1.1.2, v1.1.4 Jan 10, 2024
Rather than panicing on error, bubble up an error so that the execution
may recover correctly and the error shown to the user.

Fixes: go-debos#413
Signed-off-by: Christopher Obbard <[email protected]>
Currently the debos error handling sets an exitcode during the run, then
exits with that code once execution has finished. This paradigm is quite
fragile, so let's switch around the error handling to use context.State
to track errors and check the state of this variable after the run has
completed.

While we are here, rename the checkError function to handleError to better
describe the function's intent and make it return a boolean to show that
an error has been handled. Rework the do_run function to also return a
boolean to show that an error occurred during the run.

Signed-off-by: Christopher Obbard <[email protected]>
Rework the error handling to show where an error occured during the run.
Also print all error messages to the log rather than stdout.

Signed-off-by: Christopher Obbard <[email protected]>
Currently any errors from the cleanup of an action are
discarded and not reported to the user. So let's report
errors during the cleanup to the user and indicate that
the overall execution actually failed by setting the
exitcode appropriately.

Signed-off-by: Christopher Obbard <[email protected]>
@obbardc obbardc force-pushed the wip/obbardc/fix-recipe-cleanup branch from db61042 to 6897f33 Compare January 10, 2024 17:38
Other parts of the codebase will need to use the handleError
function so refactor it into the main debos namespace.

Signed-off-by: Christopher Obbard <[email protected]>
Unify the recipe action to handle action cleanup errors
in the same way as Debos handles the cleanup functions.

Specifically:
- run all cleanup functions, even if a previous one failed,
- run cleanup functions in the reverse order to the action,
- report all errors during cleanup.

Fixes: 74df488 ("actions: Add recipe action")
Signed-off-by: Christopher Obbard <[email protected]>
When running nested recipes using the recipe action,
if an action in the child recipe exits early (e.g. on
failure) the remaining actions do not run but the
cleanup functions for these remaining actions still
run even though there is nothing to cleanup.

This doesn't follow how Debos handles running standalone
recipes, so modify the recipe action to only run action
cleanup functions when the associated action needs to be
cleaned up.

Fixes: 74df488 ("actions: Add recipe action")
Signed-off-by: Christopher Obbard <[email protected]>
@obbardc obbardc force-pushed the wip/obbardc/fix-recipe-cleanup branch from 6897f33 to a9919aa Compare January 13, 2024 13:39
@obbardc obbardc self-assigned this Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review required
Development

Successfully merging this pull request may close these issues.

1 participant