-
Notifications
You must be signed in to change notification settings - Fork 704
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
fix(install): Remove unneeded "start halyard" after install #604
fix(install): Remove unneeded "start halyard" after install #604
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this result in an infinite loop if halyard is never ready (or install_halyard did not attempt to start halyard)?
If the install fails (like we saw before) the install script should have exited already. If halyard has another bug that prevents it from starting, removing this line doesn't introduce that behavior, that would have already happened. I'll add a timeout to wait section as a friendly precaution, though. |
I dont see an update wiht the timeout. |
13fe5b7
to
fc53248
Compare
Sorry got sidetracked & never pushed the timeout guard |
(It's here now) |
h/o don't merge yet |
install/InstallHalyard.sh.gen
Outdated
WAIT_NOW=$(date +%s) | ||
WAIT_TIME=$(( $WAIT_NOW - $WAIT_START )) | ||
|
||
if [ "$WAIT_TIME" > "$HALYARD_STARTUP_TIMEOUT_SECONDS" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I htink this should be $WAIT_TIME -gt $HALYARD_STARTUP_TIMEOUT_SECS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh never mind, this is a string compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are spinning on a sleep, you can just count iterations. Approximation is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you're right it should be -gt
I figured counting approximations would break if someone changed the sleep time per iteration. This felt a little more consistent.
The `start halyard` command was breaking the install path after the `postInstall.sh` now correctly starts Halyard after it's installed.
fc53248
to
034374d
Compare
All set on my end |
WAIT_NOW=$(date +%s) | ||
WAIT_TIME=$(( $WAIT_NOW - $WAIT_START )) | ||
|
||
if [ "$WAIT_TIME" -gt "$HALYARD_STARTUP_TIMEOUT_SECONDS" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-gt is on numbers but you have strings here.
I think you should count iterations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String comparison is lexicographic, and bash variables are untyped. This does an integer comparison.
Why do you prefer counting iterations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline
The
start halyard
command was breaking the install path after thepostInstall.sh
now correctly starts Halyard after it's installed.