8000 add rc service module and sysrc utils by thnee · Pull Request #26251 · ansible/ansible · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add rc service module and sysrc utils #26251

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

add rc service module and sysrc utils #26251

wants to merge 4 commits into from

Conversation

thnee
Copy link
Contributor
@thnee thnee commented Jun 29, 2017
SUMMARY

Adds a new module rc_service. This follows along the lines of separating modules from service.py into new service modules, as previously done with the systemd module.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • modules/system/rc_service
  • module_utils/sysrc
ANSIBLE VERSION
ansible 2.4.0 (rc_service e00e6fa58d) last updated 2017/06/29 16:50:44 (GMT +200)
  config file = /path/to/project/playbooks/ansible.cfg
  configured module search path = [u'/path/to/project/library']
  ansible python module location = /path/to/ansible/lib/ansible
  executable location = /path/to/ansible/bin/ansible
  python version = 2.7.13 (default, Jun 26 2017, 13:38:23) [GCC 4.2.1 Compatible FreeBSD Clang 3.8.0 (tags/RELEASE_380/final 262564)]
ADDITIONAL INFORMATION

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 c:module_utils/ community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Jun 29, 2017
@thnee
Copy link
Contributor Author
thnee commented Jun 29, 2017

@bcoca Pinging you for an initial review as this is based on your previous work. This is an initial version, hoping it can be a starting point for a discussion. I called it rc_service for now as that is what you suggested. But the way I see it, this should be named specifically after FreeBSD as the other BSD's work in different ways. However, I see in module_utils/facts/system/service_mgr.py:114 that there is no differentiation at all between the different BSD's at this point. Shall we make it part of this pull request to fix that too?

@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Jun 29, 2017
@mattclay
Copy link
Member

Closing and re-opening to trigger CI.

@mattclay mattclay closed this Jun 29, 2017
@mattclay mattclay reopened this Jun 29, 2017
@mattclay
Copy link
Member

@thnee Can you include integration tests in your PR so we have test coverage for this?

@ansibot
Copy link
Contributor
ansibot commented Jun 30, 2017

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/system/rc_service.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Jun 30, 2017
@bcoca
Copy link
Member
bcoca commented Jun 30, 2017

@mattclay actually, the 'service' tests on BSD will already cover this as the action plugin should select this module automatically

@bcoca
Copy link
Member
bcoca commented Jun 30, 2017

@thnee if the name is the issue we have 2 options, port the rest of 'bsd classes' that the old service module uses or alter the facts make the init system BSD flavor specific. I've engaged BUGs in the past about this topic and it seems pretty split with a slight majority in favor of the latter.

bcoca
bcoca requested changes Jun 30, 2017
View reviewed changes
lib/ansible/modules/system/rc_service.py


def run_service(module, args):
bin_path = module.get_bin_path('service', required=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not all versions of FreeBSD have the 'service' command, we should fallback to the rc script itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which versions are you thinking of specifically? The service command was added in 7.3, which is not supported any more. Falling back to calling the rc script directly would mean that we would have to re-implement and maintain duplicated logic for finding local rc scripts in various directories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had 8 and 9 installs w/o it ... but that was mostly cause previous admin avoided it as a 'linuxism', we do have the code for rc, but we can always add later of someone protests.

so ignore this point for now.

lib/ansible/modules/system/rc_service.py
default: null
choices: ['yes', 'no']
description:
- Whether the service should be enabled using sysrc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some services take extra arguments, should we implement that field here?

Copy link
Contributor Author
@thnee thnee Jul 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to suggest a different approach: We remove support for enabled from this service module, and do not attempt to set any sysrc variables at all. Instead we add another module called sysrc letting the user set any configuration they like, as sysrc is only used on FreeBSD. This will simplify the modules and separate the concerns. This is what I am doing locally now, as this makes the most sense for me. What do you think?

In contrast: on OpenBSD, there is the rcctl utility, which does support enable and disable actions, if someone makes a module for that, it would make more sense to support enabled argument in that module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would make it incompatible with the service interface

@mattclay
Copy link
Member
mattclay commented Jul 4, 2017

@bcoca Unfortunately we're not running the service tests on Shippable for FreeBSD. They weren't passing last time I checked, so they've been marked skip/freebsd.

@bcoca
Copy link
Member
bcoca commented Jul 5, 2017

@mattclay well, we need those to work if we want BSD test coverage.

@mattclay
Copy link
Member

The service integration test only has support for sysv, systemd and upstart currently.

@thnee Could you update the test to support bsd-init as part of your PR so we can test in CI? Here's where the init system specific setup occurs:

https://github.com/ansible/ansible/blob/devel/test/integration/targets/service/tasks/main.yml#L13

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 18, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Nov 19, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Feb 6, 2018
@ansibot ansibot removed the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Sep 19, 2018
@dagwieers dagwieers added the bsd BSD community label Jan 18, 2019
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Feb 8000 3, 2019
@ansibot ansibot added the system System category label Feb 19, 2019
@ansibot ansibot added collection Related to Ansible Collections work collection:community.general support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 29, 2020
@mkrizek
Copy link
Contributor
mkrizek commented Aug 26, 2020

Thank you for your interest in Ansible! We are no longer accepting new plugins to ansible/ansible directly. Please re-file this pull request against https://www.github.com/ansible-collections/community.general.

If you have further questions please stop by IRC or the mailing list:

@mkrizek mkrizek closed this Aug 26, 2020
@ansible ansible locked and limited conversation to collaborators Sep 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bcoca bcoca bcoca requested changes

Assignees
No one assigned
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bsd BSD community c:module_utils/ ci_verified Changes made in this PR are causing tests to fail. collection:community.general collection Related to Ansible Collections work module_utils/ module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. system System category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@thnee @mattclay @ansibot @bcoca @mkrizek @dagwieers @nitzmahone
0