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

New module: launchd for OS X services #20881

Open
wants to merge 2 commits into
base: devel
from

Conversation

@bcoca
Member

bcoca commented Jan 31, 2017

G

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

launchd

ANSIBLE VERSION
2.3
SUMMARY

moved from ansible/ansible-modules-core#5656

@bcoca bcoca changed the title from new launchd moduele for OS X services to new launchd module for OS X services Jan 31, 2017

@mattclay

This comment has been minimized.

Member

mattclay commented Jan 31, 2017

CI failure due to:

2017-01-31 19:18:00 ERROR: PEP 8: lib/ansible/modules/system/launchd.py:54:161: E501 line too long (177 > 160 characters) (current)
2017-01-31 19:18:00 ERROR: PEP 8: lib/ansible/modules/system/launchd.py:108:17: E126 continuation line over-indented for hanging indent (current)
2017-01-31 19:18:00 ERROR: PEP 8: lib/ansible/modules/system/launchd.py:112:13: E121 continuation line under-indented for hanging indent (current)
2017-01-31 19:18:00 ERROR: PEP 8: lib/ansible/modules/system/launchd.py:113:13: E131 continuation line unaligned for hanging indent (current)
2017-01-31 19:18:00 ERROR: PEP 8: lib/ansible/modules/system/launchd.py:114:13: E131 continuation line unaligned for hanging indent (current)
2017-01-31 19:18:00 ERROR: PEP 8: There are 5 issues which need to be resolved.

@mattclay mattclay added the ci_verified label Feb 1, 2017

@bcoca bcoca removed the needs_triage label Feb 1, 2017

@flounders

This comment has been minimized.

flounders commented Mar 9, 2017

Any progress on this?

@ansibot ansibot added the stale_ci label Apr 11, 2017

@bcoca bcoca added this to In progress in Split Service Module Oct 5, 2017

@Akasurde Akasurde force-pushed the bcoca:launchd branch from 1b46cc1 May 14, 2018

@Akasurde Akasurde changed the title from new launchd module for OS X services to New module: launchd for OS X services May 14, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented May 14, 2018

The test ansible-test sanity --test use-argspec-type-path [explain] failed with 1 error:

lib/ansible/modules/system/launchd.py:104:40: use argspec type="path" instead of type="str" to avoid use of `expanduser`

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/system/launchd.py:0:0: E325 argument_spec for "enabled" defines type="bool" but documentation does not
lib/ansible/modules/system/launchd.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "enabled" does not match the documentation ([True, False])

click here for bot help

@ansibot ansibot added the ci_verified label May 14, 2018

Update launchd module
* Updated launchd module with latest Ansible guidelines
* Added unit testcases

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@ansibot

This comment has been minimized.

Contributor

ansibot commented May 14, 2018

The test ansible-test sanity --test use-argspec-type-path [explain] failed with 1 error:

lib/ansible/modules/system/launchd.py:105:40: use argspec type="path" instead of type="str" to avoid use of `expanduser`

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/system/launchd.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "enabled" does not match the documentation ([True, False])

click here for bot help

@martinm82

This comment has been minimized.

martinm82 commented Jul 11, 2018

Any help needed with this module? I am actually super interested in it and it would be great to get the checks fixed.

launchd:
name: com.spotify.webhelper
state: started
become: yes

This comment has been minimized.

@martinm82

martinm82 Jul 11, 2018

Should be a parameter of the task itself and not the module

break
if not found:
module.fail_json(msg="Unable to find the service %s among active services." % service)

This comment has been minimized.

@martinm82

martinm82 Jul 11, 2018

For debugging purposes it would help if out would be dumped.
Example:

        msg = "Unable to find the service %s among active services." % service
        if module._verbosity >= 1:
          msg += "\r\nRunning services:\r\n%s" % out
        module.fail_json(msg=msg)
result['status']['previous_pid'] = '-'
break
if not found:

This comment has been minimized.

@martinm82

martinm82 Jul 11, 2018

If the service is not found it does not necessarily mean that this is an error. It might be that a user has uploaded a plist file in a previous task (eg. via template/copy).

So in my opinion this condition should be removed as otherwise you are not able to start newly created services

do = None
if action is not None:
if action == 'started' and not running:
do = 'start'

This comment has been minimized.

@martinm82

martinm82 Jul 11, 2018

In case a module was never loaded (launchctl load <path-to-plist>) than start/stop does not work.

So better to use load/unload instead

This comment has been minimized.

@martinm82

martinm82 Jul 11, 2018

Actually this is not entirely true. In fact if a service is stopped it needs to be started since load won't bring it up again.

result['status']['current_state'] = 'stopped'
result['status']['current_pid'] = '-'
break

This comment has been minimized.

@martinm82

martinm82 Jul 11, 2018

I would add here as well some code to set the result['failed'] according to what the module should do.

if action in ['started', 'restarted', 'reloaded']:
          result['failed'] = result['status']['current_state'] != 'running'
        elif action == 'stopped':
          result['failed'] = result['status']['current_state'] != 'stopped'
@martinm82

I was looking for something similar but when using this module with custom services it turned out to be not fully supporting all use cases. I will create a follow-up PR with my changes. Maybe we can integrate those changes to this PR?

@martinm82

This comment has been minimized.

martinm82 commented Sep 12, 2018

I have started to implement this feature to cover a bit more use cases. Here you can find my version: https://github.com/martinm82/ansible/blob/feature/macos-launchd-module/lib/ansible/modules/system/launchd.py.

I still need to implement some test cases and will then create a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment