Skip to content

[script-exit-on-error] disable for init-hooks #1493

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

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Sep 19, 2023

Summary

@Lagoja identified an issue with scripts-exit-on-error. We source init-hooks into the host shell. So, set -e will get set in the host shell. Any subsequent error will cause the shell to exit (error may be from the init-hook, or later in the shell).

For now, this PR disables this feature entirely for init hooks. We'll revisit this later: #1494

Also, this PR undoes the previous change to restrict this feature to fish-shells, since that only applied to init-hooks. Regular Devbox scripts always run in sh.

How was it tested?

testscript unit-tests

did a sanity check that regular init_hooks work.

Copy link
Collaborator Author

savil commented Sep 19, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Comment on lines +103 to +105
// NOTE: Devbox scripts will run using `sh` for consistency.
// However, we need to disable this for `fish` shell if/when we allow this for init_hooks,
// since init_hooks run in the host shell, and not `sh`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll never be able to add this to init hooks unless we decide not to source.

@savil savil merged commit 2b8c495 into main Sep 20, 2023
@savil savil deleted the savil/disable-script-err-exit-init-hooks branch September 20, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants