fix(servers_config): remove service depedency

saltstack-formulas / nginx-formula

Nginx Salt Formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html

Other

163 stars 421 forks source link

fix(servers_config): remove service depedency #277

Closed toanju closed 3 years ago

toanju commented 3 years ago

servers_config should run without service dependency. As documented service with config is managed in servers.sls.

myii commented 3 years ago

This would be effectively reverting #177, so it would be good to look at the reason(s) it was included in the first place.

@amontalban @noelmcloughlin Any particulars you can share?

javierbertoli commented 3 years ago

I think https://github.com/saltstack-formulas/nginx-formula/pull/177#issuecomment-384756097 explains why that require_in is there. Am I right?

toanju commented 3 years ago

From my understanding nginx_service_reload will do a reload hence I don't see the require_in for the service required at this stage. Maybe extending the actual required cases in servers.sls might be a better solution than having a service dependency in this config only state. From looking at #177 this might be only file: nginx_server_enabled_dir and file: nginx_server_available_dir.

Looking forward to get a better understanding from the feedback of @amontalban / @noelmcloughlin.

noelmcloughlin commented 3 years ago

I merged #177 because it was open for 9 months, but had no other involvement. Two people reviewed #177 and another person reviewed related #176. If custom configuration files are provided we probably want (a) Nginx service to wait for require_in OR (b) Nginx service to restart due to onchanges_in. That is my interpretation of #176 and #177.

toanju commented 3 years ago

K, instead of having a separate nginx_service_reload I now extended nginx_service in servers.sls to have the same behavior as in the original PR. Let me know if this is acceptable.

toanju commented 3 years ago

in some cases nginx was started before some config files were deleted and on gentoo and arch a symlink was created without having the dir available hence added:

wait for all file states (create and delete)
create dirs for symlinked files

toanju commented 3 years ago

@myii, @javierbertoli, @amontalban, @noelmcloughlin are there any open questions or can this be merged?

toanju commented 3 years ago

@javierbertoli, just removed it. Couldn't see a reason either.

myii commented 3 years ago

@sticky-note Will you be reviewing this before merge? @javierbertoli Or have you already reviewed it?

javierbertoli commented 3 years ago

@myii, @toanju I think we're OK to merge these changes.

myii commented 3 years ago

@toanju Thanks for the PR -- merged. Appreciate the review, @javierbertoli.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 2.6.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

toanju commented 3 years ago

Thanks a lot all!

myii commented 3 years ago

@toanju Would you mind responding to this query?

https://github.com/saltstack-formulas/nginx-formula/commit/5d7fc8d5ba4828654c41e9f13f128260f8cf2869#r48859525

toanju commented 3 years ago

done