dahlb / ha_hatch

Home Assistant Integration for Hatch Rest Mini
MIT License
77 stars 16 forks source link

Fix package installation for debian and ubuntu containers #40

Closed bmcclure closed 1 year ago

bmcclure commented 1 year ago

This is one solution to resolving the issue discussed in https://github.com/dahlb/ha_hatch/issues/39. When running Home Assistant inside of a Docker container other than Alpine Linux, the setup process would error out due to trying to invoke the apk command and receiving the error that it is unknown.

This PR uses the distro package to maintain the existing functionality for Alpine containers, and adds similar functionality for Ubuntu and Debian. It also outputs a warning for any other Docker-based distro indicating that manual installation of those package might be necessary.

Since many Docker containers also don't run as root, I also added a bit of code that prefixes the command with "sudo" for anyone other than uid 0. An alternative would be to skip package installation if not root. I'm not sure which would be preferable, but that's easy to change.

An alternative solution

All that being said, it feels like installing OS packages should perhaps be out of scope for a custom integration. There's no way to know if the Docker container is a dev box or if it's someone's production HA instance. And the fact that it only runs when it's in a Docker container means that many HA installations won't run that code anyway, so is it really necessary at all?

I wonder if the better solution would be to just remove the OS package installation code, and if those packages are really needed, output a log error if they're missing telling the user to install them. This would be safer, and wouldn't require checking for various distros or making unannounced changes to the host machine. If this would be an acceptable change, I could submit a PR for that instead of this one.

dahlb commented 1 year ago

nice improvement, though try to stick with semantic commit messages for the release logs

bmcclure commented 1 year ago

Thank you! I looked a bit into semantic commit messages, I will make sure to use those for future commits.