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

Update service.py #34366

Merged
merged 1 commit into from
Jul 1, 2016
Merged

Update service.py #34366

merged 1 commit into from
Jul 1, 2016

Conversation

steverweber
Copy link
Contributor

What issues does this PR fix or reference?

#30660

Previous Behavior

might stop service while in test mode

New Behavior

does not stop service while in test mode

stop service before start after __opts__[test] was checked
@cachedout cachedout merged commit 5ddf417 into saltstack:2015.8 Jul 1, 2016
@brc
Copy link
Contributor

brc commented Jul 6, 2016

@steverweber After reading this PR's description a few times, I don't think it was your intention to imply that my PR was a problem (but it's extremely confusing).

I would have appreciated if you wrote something more clear for the reviewer, such as:

  1. PR Revert service.mod_watch() bug introduced in Issue 21084 #30660 reverted bad code in develop, which is a good thing.
  2. My PR reverts the same bad code in 2015.8. I am also going to complect the matter by re-introducing the same bad code a different way within the same commit (and PS: it still doesn't even do the right thing)!

This should be two commits. It should probably be two separate PRs (one to revert the broken code and one to discuss a solution for handling the particular type of init scripts you're compensating for).

Enjoy your spaghetti. 🍝

@steverweber
Copy link
Contributor Author

Sorry didn't try to imply anything about your code revert. You did a good job fixing that bug!

I also agree the solution I pushed is kinda hacky... Your thoughts on using sig are good however this is not always practical... Think I had issues with some init scripts that preformed a 2xfork doing something like ((sh -c 'some bin') 1>/dev/null 2>/dev/null &)& (kinda forget how the demand went) and or sometimes the process name is unknown or kinda random..

Anyways the current PR could be improved.
perhaps also check that reload is not set.

if verb == 'start' and 'service.stop' in __salt__ and not reload:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants