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

Make start.sh abort and fail if one of its steps failed #120

Merged
merged 3 commits into from
Nov 29, 2022

Conversation

vorburger
Copy link
Contributor

This seems like a good idea, and could reduce confusion?

One example, among many different possible failures obviously, is what I ran into in #115: nginx failed to start, but the shim ran anyway. That's confusing, and it's probably safer to "fail fast" and abort on an emerg from Nginx, or any similar launch failures.

Please note that I HAVE NOT TESTED THIS 😈 at all... Does this project run integration tests on PRs?

Would you need me to rebuild and run with this to make sure it's fine? Or can you test it?

@joaosa
Copy link
Contributor

joaosa commented Nov 25, 2022

This seems like a good idea, and could reduce confusion?

One example, among many different possible failures obviously, is what I ran into in #115: nginx failed to start, but the shim ran anyway. That's confusing, and it's probably safer to "fail fast" and abort on an emerg from Nginx, or any similar launch failures.

Please note that I HAVE NOT TESTED THIS 😈 at all... Does this project run integration tests on PRs?

Would you need me to rebuild and run with this to make sure it's fine? Or can you test it?

Thanks @vorburger! Your approach makes sense and I would even extend it to set -euo pipefail 👍

I'll try to test this when I get a chance. This project does run integration tests, but they do not cover all aspects of the codebase :)

…reviously defined

Signed-off-by: Michael Vorburger ⛑️ <[email protected]>
@vorburger
Copy link
Contributor Author

I would even extend it to set -euo pipefail +1

done!

@DiegoRBaquero DiegoRBaquero merged commit 1375aa9 into filecoin-saturn:main Nov 29, 2022
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.

3 participants