Bugfix for Ubuntu & reload over restart for faster config updates #48

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@bitgangsta

Hi,

Two changes are in this pull. We had made this fork a while ago, so I'm happy to update / rebase if need be. However, I wanted to see if this would be friendly before I do.

a) We updated the config updates to cause a reload rather than restart, for performance reasons. This should be fine, and I would think is preferable, but let me know if you disagree.

b) Ubuntu's default shell isn't bash (see discussion on that commit). The best practice should be to use /usr/bin/env to find the bash location.

Any concerns/ comments?

@bitgangsta

This comment has been minimized.

Show comment Hide comment
@bitgangsta

bitgangsta Dec 17, 2013

Member

@hdanak why did you change the interpreter? Is there a benefit in using Bash? Was it broken in some way with just sh?

Member

bitgangsta commented on 6a6afc8 Dec 17, 2013

@hdanak why did you change the interpreter? Is there a benefit in using Bash? Was it broken in some way with just sh?

This comment has been minimized.

Show comment Hide comment
@hdanak

hdanak Dec 17, 2013

@bitgangsta sh is symlinked to different shells in different versions of Ubuntu, and the switch statement used in this init file depends on bash. Unfortunately different versions of linux userland also place bash differently, so the /usr/bin/env call is for generality (keep in mind that #! lines only take a single command and single argument, so you can't add a parameter to bash this way -- a sort of unexpected trade-off).

@bitgangsta sh is symlinked to different shells in different versions of Ubuntu, and the switch statement used in this init file depends on bash. Unfortunately different versions of linux userland also place bash differently, so the /usr/bin/env call is for generality (keep in mind that #! lines only take a single command and single argument, so you can't add a parameter to bash this way -- a sort of unexpected trade-off).

This comment has been minimized.

Show comment Hide comment
@bitgangsta

bitgangsta Dec 17, 2013

Member

@hdanak thanks for the quick reply. So if I was to submit a pull request to the author, do you think it would be favorable? Ubuntu should use bash always? Any idea why /bin/sh was in the ubuntu file anyway?

Member

bitgangsta replied Dec 17, 2013

@hdanak thanks for the quick reply. So if I was to submit a pull request to the author, do you think it would be favorable? Ubuntu should use bash always? Any idea why /bin/sh was in the ubuntu file anyway?

This comment has been minimized.

Show comment Hide comment
@hdanak

hdanak Dec 18, 2013

Yeah, bash should be explicit, since Ubuntu's default shell is dash or something. I bet it was /bin/sh either out of habit (people forget that /bin/sh is not bash, but actually some POSIX shell, which may be bash in sh-mode), or it was copied from some old script. I think a pull request would be fine, as far as I know the /usr/bin/env convention is universal (while /bin/bash isn't, although it's usually symlinked).

Yeah, bash should be explicit, since Ubuntu's default shell is dash or something. I bet it was /bin/sh either out of habit (people forget that /bin/sh is not bash, but actually some POSIX shell, which may be bash in sh-mode), or it was copied from some old script. I think a pull request would be fine, as far as I know the /usr/bin/env convention is universal (while /bin/bash isn't, although it's usually symlinked).

@yourabi

This comment has been minimized.

Show comment Hide comment
@yourabi

yourabi Sep 10, 2014

Collaborator

It was probably /bin/sh out of habit and that /bin/sh is the system (posix compliant) shell...

Have you tested this out? Glancing at recipes/default.rb I wonder how monit would handle "reload" vs "start" on a node's firstboot?

In principal I think a version of this with reduced scope could be interested (reload vs restart on template render / config change)

Collaborator

yourabi commented Sep 10, 2014

It was probably /bin/sh out of habit and that /bin/sh is the system (posix compliant) shell...

Have you tested this out? Glancing at recipes/default.rb I wonder how monit would handle "reload" vs "start" on a node's firstboot?

In principal I think a version of this with reduced scope could be interested (reload vs restart on template render / config change)

@daanemanz

This comment has been minimized.

Show comment Hide comment
@daanemanz

daanemanz Jan 22, 2015

Would it be possible to just have reload included as in https://github.com/apsoto/monit/pull/48/files#diff-55bf87238c9b8af164c5133a60721f12R15? That would probably be helpful by itself.

Would it be possible to just have reload included as in https://github.com/apsoto/monit/pull/48/files#diff-55bf87238c9b8af164c5133a60721f12R15? That would probably be helpful by itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment