{"action":"opened","issue":{"url":"https://api.github.com/repos/saltstack/salt/issues/20915","labels_url":"https://api.github.com/repos/saltstack/salt/issues/20915/labels{/name}","comments_url":"https://api.github.com/repos/saltstack/salt/issues/20915/comments","events_url":"https://api.github.com/repos/saltstack/salt/issues/20915/events","html_url":"https://github.com/saltstack/salt/issues/20915","id":58504692,"number":20915,"title":"Logic conflict with `changes` in service.running and service.dead?","user":{"login":"lorengordon","id":8457307,"avatar_url":"https://avatars.githubusercontent.com/u/8457307?v=3","gravatar_id":"","url":"https://api.github.com/users/lorengordon","html_url":"https://github.com/lorengordon","followers_url":"https://api.github.com/users/lorengordon/followers","following_url":"https://api.github.com/users/lorengordon/following{/other_user}","gists_url":"https://api.github.com/users/lorengordon/gists{/gist_id}","starred_url":"https://api.github.com/users/lorengordon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lorengordon/subscriptions","organizations_url":"https://api.github.com/users/lorengordon/orgs","repos_url":"https://api.github.com/users/lorengordon/repos","events_url":"https://api.github.com/users/lorengordon/events{/privacy}","received_events_url":"https://api.github.com/users/lorengordon/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2015-02-22T14:42:17Z","updated_at":"2015-02-22T14:42:17Z","closed_at":null,"body":"I was reviewing the code for the service.py state, and I think I stumbled into a conflict with the `changes` logic when `enable` is set. It has to do with how the `_enable()` and `_disable()` functions handle the `started` parameter.\r\n\r\nThe `_enable()` and `_disable()` functions use the `started` parameter to gate whether `changes=True`. But `service.running` and `service.dead` both use `started` to report whether the service is running (regardless of whether they changed that state).\r\n\r\nThere are two commits flip-flopping how `changes` is reported right now. https://github.com/saltstack/salt/commit/4b1413fc8ad953cabc04fb489042bd09f9bfdd03 and https://github.com/saltstack/salt/commit/2426c224a4eb1a01f3eb4f14c2f201c8c6e49468\r\n\r\nIn the first, `service.running` will start the service, but will return the output of `_enable()` and if the service is already enabled then `_enable()` will return no changes.\r\n\r\nIn the second, `service.dead` may not make changes (because it failed to stop the service), but it will return the output of `_enable()` saying that it did.\r\n\r\nI'm thinking that `service.running` and `service.dead` should not directly return the output of `_enable()` or `_disable()`, but should instead update the `ret` variable and then return `ret`."}}