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

set __virtualname__ to 'service' #36330

Merged
merged 1 commit into from
Sep 15, 2016
Merged

Conversation

silenius
Copy link
Contributor

@silenius silenius commented Sep 15, 2016

What does this PR do?

It sets __virtualname__ to 'service'.

In the states/service.py file the check is made through the _available function:

def _available(name, ret):
    '''
    Check if the service is available
    '''
    avail = False
    if 'service.available' in __salt__:
        avail = __salt__['service.available'](name)
    elif 'service.get_all' in __salt__:
        avail = name in __salt__['service.get_all']()
    if not avail:
        ret['result'] = False
        ret['comment'] = 'The named service {0} is not available'.format(name)
    return avail

So currently it doesn't work with the daemontools module as the __virtualname__ is not service

Also: not sure if provider: daemontools is still supported as I haven't found any reference in the service state file ... and it is not mentionned in the documentation

What issues does this PR fix or reference?

None

Tests written?

No

@rallytime
Copy link
Contributor

Thanks for fixing this @silenius

@rallytime rallytime merged commit 9451141 into saltstack:2016.3 Sep 15, 2016
gitebra pushed a commit to gitebra/salt that referenced this pull request Sep 19, 2016
* upstream/develop: (30 commits)
  correct key check
  Create a set from the minion list
  Skip extra traversals of minion cache for auth
  Whitespace fix
  Add -V option to work with --versions-report
  Update usage docs for Thorium (saltstack#36368)
  Fix calc docstrings (saltstack#36366)
  Fix a minor typo in docs (saltstack#36365)
  salt/modules/apk.py: Add version() function support (saltstack#36362)
  postgres_extension: report changes when an extension was installed (saltstack#36335)
  Schema test requires jsonschema 2.5.0 or above
  Check for Ign/Hit membership instead of == in aptpkg.refresh_db
  Quote postgres privilege target names (saltstack#36249)
  Fixed arguments to VPC peering function (saltstack#36268)
  set __virtualname__ to 'service' (saltstack#36330)
  Use infoblox_* values if present in arguments (saltstack#36339)
  Return None when find_file identifies the path as a directory (saltstack#36342)
  remove help message from glance module (saltstack#36345)
  Clean up libcloud stack trace (saltstack#36314)
  Add resize2fs unit test from blockdev_test to disk_test (saltstack#36346)
  ...
@PChambino
Copy link

Hi there @silenius and @rallytime ! Can you take a look at #37498 (comment)?

This PR makes salt assume that because daemontools is installed, it means that it is the system-wide init process. However, I think daemontools is more than a system-wide init process. It can be used under upstart/systemd to manage some directories of services, or to use some of its tools independently, in my case its envdir. I would say salt shouldn't default the service provider to daemontools just because it is installed. What do you think?

@silenius
Copy link
Contributor Author

silenius commented Nov 17, 2016

Hello, I think you are right, it should only be active when - provider: daemontools has been set in the service state. The docs for salt.modules.daemontools says that:

This module is compatible with the service states, so it can be used to maintain services using the provider argument

but I don't find any mention of provider in salt.states.service, perhaps the __virtual__ function should be fixed, currently it is:

def __virtual__():
    '''
    Only make these states available if a service provider has been detected or
    assigned for this minion
    '''
    if 'service.start' in __salt__:
        return __virtualname__
    else:
        return (False, 'No service execution module loaded: '
                'check support for service management on {0} '
                ''.format(__grains__.get('osfinger', __grains__['os']))
               )

@silenius
Copy link
Contributor Author

Can you check if #37748 fixes the issue?

@PChambino
Copy link

Yes, it does fix the issue. I believe that is a better heuristic. 👍

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