dale3h
commented
7 years ago Thank you for sharing your thoughts!
-
I will update the binary locations to be absolute in the next release.
-
The colon was intentionally left off in line 66, as this allows the user to configure
VIRTUAL_ENV=in their config file if they are not running a venv.- From http://www.tldp.org/LDP/abs/html/parameter-substitution.html:
The extra : makes a difference only when parameter has been declared, but is null.
- From http://www.tldp.org/LDP/abs/html/parameter-substitution.html:
-
I will give this some extra thought.
And lastly, thank you for your kind words!
Wanted to open an issue on this for the discussion rather than jumping in with a PR.
Might be a good idea to hard code binary locations. Ie, /usr/bin/curl instead of curl. Since you are running some of these with sudo, there's some vulnerability there. I realize that might introduce a distro dependency problem, but I'm pretty sure all put curl at same place. Also, if someone tries to run hassctl from an environment without a path variable (maybe from crontab), full path will work while shortname will fail.
Line 66
VIRTUAL_ENV="${VIRTUAL_ENV-/srv/homeassistant}", I'm not 100% on my bash syntax, but shouldn't it beVIRTUAL_ENV="${VIRTUAL_ENV:-/srv/homeassistant}"?Last one - and big for me personally, what if script calling user does not have sudo privs? I run HA under the user "hass" which does not have sudo access (on purpose). Might want to either redesign to be called like 'sudo hassctl whatever' and check for root privs at startup (and then not use sudo inside) or include some sort of mechanism if the sudo fails.
Anyway. Cool tool. I know a lot of people on the gitter like it.