Skip to content

start.sh: fix quoting, kill underlying process on exit#1784

Merged
landism merged 5 commits into
masterfrom
matt/start.sh
Jul 1, 2019
Merged

start.sh: fix quoting, kill underlying process on exit#1784
landism merged 5 commits into
masterfrom
matt/start.sh

Conversation

@landism

@landism landism commented Jun 27, 2019

Copy link
Copy Markdown
Member
  1. we want "$@" instead of $* to ensure that we run the command with the same quotes as the original command
  2. while I was testing that, it turned out that killing the script didn't kill the underlying process, which might not matter for k8s purposes, but is nice to do for local testing

@landism
landism requested review from jazzdan and nicks June 27, 2019 15:51

@jazzdan jazzdan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@jazzdan

jazzdan commented Jun 27, 2019

Copy link
Copy Markdown
Contributor

By the way @landism there are multiple copies of this file in the repo

Comment thread integration/onewatch/start.sh Outdated
echo "$process_id" > process.txt
wait $process_id
EXIT_CODE=$?
wait $process_id && EXIT_CODE=$? || EXIT_CODE=$?

@landism landism Jun 27, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I broke this - when we killed the process to restart, wait returned a non-0 exit, so the script exited because of set -e

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would it make sense to just set +e before the wait?

Comment thread integration/onewatch/start.sh Outdated
echo "$process_id" > process.txt
wait $process_id
EXIT_CODE=$?
wait $process_id && EXIT_CODE=$? || EXIT_CODE=$?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would it make sense to just set +e before the wait?

@landism
landism merged commit b374770 into master Jul 1, 2019
@landism
landism deleted the matt/start.sh branch July 1, 2019 21:00
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