voxpupuli / puppet-homeassistant

Puppet Module for Home Assistant - home automation platform. https://home-assistant.io/
Apache License 2.0
8 stars 9 forks source link

Expand PATH variable in service file #29

Closed lordievader closed 4 years ago

lordievader commented 4 years ago

Pull Request (PR) description

Systemd doesn't do variable expansion in its unit definition. From the systemd.exec man page:

Variable expansion is not performed inside the strings, however, specifier expansion is possible.

Effectively this breaks finding of binaries if Home Assistant is set up with this module. (In my case demonstrated by the wake-on-lan component not being able to find ping.)

So I propose to replace '$VIRTUAL_ENV' with '<%= @home %>'. And '$PATH' with the PATH variable from /etc/profile (in this PR it is from Raspbian).

This Pull Request (PR) fixes the following issues

I did not make an issue for this PR. There is no open issue describing the same.

bastelfreak commented 4 years ago

Hi @lordievader, thanks for the PR! can you please add a test that verifies the Environment=... line is correct?

lordievader commented 4 years ago

Hi @bastelfreak. What would be a good way to test this? I didn't see a testing framework that is used in this project.

bastelfreak commented 4 years ago

Hi @lordievader, this module uses rspec-puppet: https://github.com/voxpupuli/puppet-homeassistant/blob/master/spec/classes/init_spec.rb

if you've questions about it it might be a good idea to join our IRC channel #voxpupuli on freenode or our slack at https://slack.puppet.com/. That probably makes it easier to discuss the test setup.

lordievader commented 4 years ago

@bastelfreak With some help from @alexjfisher on IRC I was able to run the rspec-puppet. I've added a line to the init_spec.rb to check the Environment. Is this sufficient?

alexjfisher commented 4 years ago

@lordievader Thanks for adding a test. I don't know that much about homeassistant, but given your detailed explanation, this does look correct to me. Thanks!