search

capistrano-plugins / capistrano-unicorn-nginx

Capistrano tasks for automatic and sensible unicorn + nginx configuration
MIT License
175 stars 81 forks source link

add systemd support #98

Closed BenoitHiller closed 7 years ago

BenoitHiller commented 7 years ago

This fixes #70 by adding an :init_system variable which takes values of either :sysv or :systemd.

The code should not produce any changes in behaviour when run without setting :init_system except that I called publish action for unicorn run_latest, so the print will be different.

I'm not sure if that is a change you approve of, however the semantics of systemd are different and we have to be careful to actually start the services. Also supporting the reload action with a signal is apparently very much not recommended(it wouldn't be that hard to do though if we really wanted to).

I see that there are other pull requests like #45 that are having trouble getting merged that do similar tasks. It might make sense to have separate flags for init system, service installer, and service reloader. That seems like it would cover all of the different cases if you combined it with making more of the paths configurable. Then the whole thing could be wrapped up in some presets for common configurations. That way you would have decent support for both common OS's and arbitrary OS's.

This code is tested on my arch system right now. I should be putting it through its paces on ubuntu 16.04 soon and could probably give 14.04 a try at the same time. Seems like we are going to need something to run automated tests on different images though or it is going to be impossible to maintain extensions like this in the future.

griffithac commented 7 years ago

I am assuming this is why I am having service restart uses after my upgrade to Ubuntu 16.04. I would love to see this merged.

BenoitHiller commented 7 years ago

Yes, the current gem doesn't work at all on 16.04. It is unlikely that this will be merged anytime soon(as there are a number of other modifications competing to expand support of the plugin).

If you want to use my branch you can just put the following in your gemfile:

gem 'capistrano-unicorn-nginx', github: 'BenoitHiller/capistrano-unicorn-nginx', branch: 'systemd'
griffithac commented 7 years ago

@BenoitHiller thanks much. I will give it a try. I have some crazy hacks in place right now to force restarting of my unicorn workers. If you saw it you would laugh.

griffithac commented 7 years ago

@BenoitHiller thanks so much for your input. I will read up on the documentation you included and work towards implementing the correct solution.

I mean to merge all your changes when final into the parent master. I was just using my fork to experiment with my own production app. I think once we resolve these issues you can add the code to your branch and we can merge it. This is mostly your work anyway. I am just trying to make sure it works for me and that it will be a benefit to the community as a whole.

griffithac commented 7 years ago

@BenoitHiller, you mentioned me being added as a contributor. Honestly I have no idea why I was invited. But, I have always wanted a chance to work on something open source that I also used and I guess this is my chance. You will have to excuse me as I am very new to the process. Any help or advice you have for my is greatly appreciated.

BenoitHiller commented 7 years ago

@griffithac I would assume that the owner of the repository has added you because they read one of the articles floating around that talked about how just adding people with pull requests as collaborators had turned out well on projects they had little time for.

As for advice I would suggest being cautious. There appear to be a fairly sizeable number of people using this gem in its current form, so the number one priority in merging this code in is making sure nothing breaks for the people on ubuntu 14.04.

Also I would strongly suggest contacting @rhomeister directly to get a better idea of what they were hoping for in adding you. They are also likely the best source of advice on addressing the existing issues and pull requests for the project so long as you are willing to do the legwork.

As for this pull request now that I have had a lot more time to think about how this should be structured I think that instead of switching directly between different behaviour in a case statement the correct approach is to extract different sub-tasks for the different init system and run those conditionally based on the target environment you specify.

If you want to do more work on this I can change the target branch to be something like systemd instead of master and merge it in. Then you will have commit access to it, as you seem to have more time to work on this than I do.

griffithac commented 7 years ago

@BenoitHiller thanks for the words of caution. I intend to be careful.

I will also reach out to @rhomeister, so far I have not been able to contact him, but I will give it another shot.

I think you are right about changing your approach to sub-tasks. Are you interested and writing that code, if not I can probably work on it.

And yes, please change the target branch to systemd, then I will start using it instead of my fork to work on changes.

BenoitHiller commented 7 years ago

K. I have merged it into systemd. If you have the time to work on the improvements I described I would suggest you give it a shot as it would be a while before I have any time to do it. Otherwise let me know and I will pick it up when I can.

griffithac commented 7 years ago

Will do, thanks.