mongrelion / ansible-role-docker

Ansible role for installing Docker
MIT License
62 stars 39 forks source link

Change location of systemd service file #28

Closed paulfantom closed 6 years ago

paulfantom commented 6 years ago

According to systemd docs and ways of packaging applications you shouldn't overwrite systemd service file in /usr/lib/systemd/system/ since this file will be overwritten by package installer on update. Safe place for storing service files is in /etc/systemd/system and this PR fixes this.

mongrelion commented 6 years ago

Hello, @paulfantom

Thanks for this PR. Did you test this locally to make sure that you didn't break the role? :)

paulfantom commented 6 years ago

Yes, I ran it against fedora 27 server in VM.

If you want I can also create PR with CI pipeline in travis with molecule test framework (like in this role, this one or this one) so you won't have to ask this question again :smile:

mongrelion commented 6 years ago

@paulfantom that would be awesome. I find myself asking this question over and over again on the PRs because we don't have an integration tests suite yet in place :( See https://github.com/mongrelion/ansible-role-docker/issues/13

If you think that you can take care of that, then I'll let you submit a PR for it first, then once that's merged, merge this one. Otherwise you'll have to wait for someone else to be able to pick this up and ensure that your changes don't break backwards compatibility.

Let me know what are your thoughts.

mongrelion commented 6 years ago

Ok, this seems to be going nowhere with the tests. I thought for a second to merge this as it is but it seems like more people are depending on this role and we need to be wary of not breaking compatibility. So, until we don't have a proper test suite in place, I'm going to hold all pull requests back.

Sorry about this.

paulfantom commented 6 years ago

I have totally forgot about it. However look at #30.

mongrelion commented 6 years ago

@paulfantom could you please rebase with master as to trigger the CI pipeline again?

paulfantom commented 6 years ago

Done

mongrelion commented 6 years ago

This looks good. Let's get it merged. Thanks again for your help, @paulfantom