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

Abort bank demo on error #59

Closed
wants to merge 13 commits into from
Closed

Abort bank demo on error #59

wants to merge 13 commits into from

Conversation

wrp
Copy link
Contributor

@wrp wrp commented Oct 30, 2019

I was gettin failures that were hard to track down because docker commands early in the script would fail, but the error messages are obscured because the script blindly continues. By aborting early on a failure, it is much easier to notice errors.

@ymatsuda
Copy link
Collaborator

Please keep the format of messages of the action result consistent.
I used
.... blah blah blah [some id or name] yada yada yada
Now some are in above format and others.
My preference is to prefix the result messages with "..." and keep ids/names in brackets.

@wrp
Copy link
Contributor Author

wrp commented Oct 31, 2019

I (mostly) reverted the messages. Moved some to only be before the call, so that it is more clear what has failed if a failure occurs. Is this what you were thinking?

@wrp wrp changed the title Cleanup local test Abort bank demo on error Oct 31, 2019
@wrp
Copy link
Contributor Author

wrp commented Nov 7, 2019

This test needs to fail early (for exampe, if dockerd is not running!). If the modifications I've made to the comments are a concern, please be specific.

@ymatsuda
Copy link
Collaborator

ymatsuda commented Nov 7, 2019

Is there any particular reason to switch some of if [...] to if test ...? The coding style became inconsistent.
I prefer if [...] to if test ... because the test condition stands out in the syntax.
If you like to use flags like -z and -n, I suggest using [ -z ...] and [ -n ...].

@wrp
Copy link
Contributor Author

wrp commented Nov 7, 2019

No particular reason, just habit. I've found that the [ spelling of test has caused a great deal of confusion in its history, and strongly prefer test. But I'll change it back for consistency. This is not a relevant change here.

Copy link
Collaborator

@biplap-sarkar biplap-sarkar left a comment

Choose a reason for hiding this comment

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

Given that the demo bank app doesn't check whether dependent containers are running, the app can still seem to be stuck if it runs without starting the waltz cluster.

@wrp wrp closed this Oct 28, 2021
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.

None yet

4 participants