Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change location of systemd service file #28

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

paulfantom
Copy link
Collaborator

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
Copy link
Owner

Hello, @paulfantom

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

@paulfantom
Copy link
Collaborator Author

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 😄

@mongrelion
Copy link
Owner

@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 #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
Copy link
Owner

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
Copy link
Collaborator Author

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

@mongrelion
Copy link
Owner

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

@paulfantom
Copy link
Collaborator Author

Done

@mongrelion
Copy link
Owner

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

@mongrelion mongrelion merged commit 84d3b07 into mongrelion:master Apr 4, 2018
@paulfantom paulfantom deleted the patch-1 branch April 4, 2018 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants