Exit with exit code 1 when there's an exception

[ankatiyar]

Description

Small regression from #4075

Development notes

Reminder to self: Will have to revert kedro-org/kedro-plugins#813 after the next release

Note to reviewers:
Please verify if #4073 is still resolved!

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[lrcouto]

Was the program not returning an 1 already in the case of failure? It should be failing both unit and e2e tests if it wasn't.

[ankatiyar]

Was the program not returning an 1 already in the case of failure? It should be failing both unit and e2e tests if it wasn't.

The kedro-docker tests were failing because of this kedro-org/kedro-plugins#800. I think this branch of logic is executed when the exceptions come from outside like the kedro project itself, which I don't know if we test for.

[ElenaKhaustova]

Now it looks like we don't need hook_called flag anymore as all the branches that set it to true now have sys.exit(). So maybe we can just move self._cli_hook_manager.hook.after_command_run to the try block and get rid of finally, so the logic is a bit cleaner?

[ElenaKhaustova]

I don't want to block the PR, so we can pay attention to this in a separate one.

[ankatiyar]

Actually, I think you're right we don't need the finally block at all. If the the command executes successfully or the command doesn't exist - it goes into the first except block otherwise if there's an exception it is caught by the next except. Hook gets called either way.

[lrcouto]

Check if it doesn't get called twice on a successful case too, it's why that finally block was introduced.

[merelcht]

Tested and the behaviour is as expected and still solves #4073

[ElenaKhaustova]

Tested the changes, the #4075 is still solved but the traceback view changed:

Before changes:

After changes:

[ankatiyar]

Did a little more investigation - the problem just turned out to be the indentation of sys.exit(), it was inside the if block so KedroCliError was exiting with 0.

When there's a CLI error such as was the case with docker e2e test - kedro-org/kedro-plugins#800 it goes into the first except SystemExit block and when there's any other kind of exceptions it should go into the except Exception block. In either cases we now have after_command_hook called so we don't need the finally block.

Please take another look @ElenaKhaustova @merelcht @lrcouto

[merelcht]

Tried the revised implementation, and looks all good 👍

[ElenaKhaustova]

Thank you, @ankatiyar!

Tested the updates - all works as expected 🚀