search

gersilex / apcupsd-docker

apcupsd in a container; can trigger arbitrary commands on the Docker host
https://hub.docker.com/r/gersilex/apcupsd
MIT License
25 stars 13 forks source link

Add mail-wrapper.sh to allow email notifications from within #7

Closed lenkawamoto closed 3 years ago

lenkawamoto commented 3 years ago

I added mail-wrapper.sh which is a wrapper shell script to allow the container's apcupsd to send email notifications using busybox's sendmail.

I created it so that I can get email notifications, so I've only tested this with my email service provider with my particular configuration (using STARTTLS). It might be nice to allow for non-STARTTLS connections but I'm not interested in that right now.

I've included some documentation as comments in the script itself.

gersilex commented 3 years ago

Hi @lenkawamoto . Thank you for your contribution. This looks very mature and clean. Can you help me understand some design decisions you made?

If I understand correctly, apcupsd calls mail to send mails, which is not installed in that image. Instead, you write a wrapper, that can be used like mail but internally uses sendmail which indeed is installed in the image.

I want to object to your contribution in NO way. I'm just really interested in your thoughts. Thanks again and Cheers!

lenkawamoto commented 3 years ago

My thought was to minimize impacting the docker image as much as possible, and with this solution there was no need to build a new docker image.

Since sendmail is already available, I didn't think there's a need to add an extra binary (which might add more configuration/overhead/other dependencies) to accomplish what I needed in enabling email notifications from within the container.

You could install the script into the container, but then you would need a mechanism to get the user defined configuration values for $mailTo, $mailFrom, $mailServer, $mailPort, $mailUsername, $mailPassword.

EDIT: The values might be gotten via ENVIRONMENT, if you want to go that route.

Also, as it stands, NOT having this shell script makes the container behave as is -- not send any email notifications. Adding this shell script into the image will change how the container behaves, and unless the sendmail related configurations are correct, will cause errors. Leaving it outside the container makes it an optional functionality.

EDIT: You could also modify the script to not call sendmail if any of the configuration values are missing -- this would get the container to behave as it currently does and not send email notifications.

You're totally welcome to merge this pull request or disregard it and install the /usr/bin/mail binary into the image. Either way, I will not be offended at all. Installing mail might be better in that it is a well used/mature binary, whereas my shell script is new. However, I think this script is pretty trivial on what it does and (of course) it works for me as is.

gersilex commented 3 years ago

Thank you for your explanation and your great contribution! I added an Add-Ons part to the README that mentions your wrapper.