ironsheep / RPi-Reporter-MQTT2HA-Daemon

Linux service to collect and transfer Raspberry Pi data via MQTT to Home Assistant (for RPi Monitoring)
GNU General Public License v3.0
441 stars 62 forks source link

Improved error handling with external commands #107

Closed hostcc closed 2 months ago

hostcc commented 10 months ago
ironsheep commented 2 months ago

Thank you for contributing these changes. They'll really help make this script more robust! -Stephen

ironsheep commented 2 months ago

Oh, I'd like to add documentation on setting MQTT_SENSOR_HOSTNAME in the daemons environment. Can you provide examples of how you are doing it?

ironsheep commented 2 months ago

@hostcc I'm getting an error with the invoke_shell_cmd() using sh as the shell.
--> sh: 0: Illegal option -o pipefail

If I change this to using bash vs. sh then it all works.

Thoughts?

hostcc commented 2 months ago

@hostcc I'm getting an error with the invoke_shell_cmd() using sh as the shell. --> sh: 0: Illegal option -o pipefail

If I change this to using bash vs. sh then it all works.

Thoughts?

Apologies for that, @ironsheep - it indeed might be too bash specific. Will try to address today

hostcc commented 2 months ago

Oh, I'd like to add documentation on setting MQTT_SENSOR_HOSTNAME in the daemons environment. Can you provide examples of how you are doing it?

Sure sir - will do a PR amending the documentation. Basically, it allows to override the hostname discovered from the system, containerized setup are't always allowing to customize that easily

hostcc commented 2 months ago

@hostcc I'm getting an error with the invoke_shell_cmd() using sh as the shell. --> sh: 0: Illegal option -o pipefail If I change this to using bash vs. sh then it all works. Thoughts?

Apologies for that, @ironsheep - it indeed might be too bash specific. Will try to address today

@ironsheep , the solution you introduced in https://github.com/ironsheep/RPi-Reporter-MQTT2HA-Daemon/commit/259e42c1faa349ebcdcfc7106751c5326cd394f1#diff-075607ba753db7556565789706ad37a90055655b26016827c9f79343ddee723bR389 is a good one - it immediate solves the issue, agree. Although it implies the users have bash installed (should be applicable to most of the systems?), could be mentioned in documentation if users report problems

ironsheep commented 2 months ago

@hostcc I was thinking of detecting the presence of bash and using it when it is present. Then, if it is not present, falling back to using sh without the new option would keep older systems working as well as they did in the past, while systems with bash would be improved.

I was thinking of logging a warning message to indicate that the script would be more robust if bash is installed. but I don't want to keep dumping into systems logs. Maybe I only do this if -d is enabled? Or maybe dumping to logs only at startup is really ok?