Add method get_bin_path to module_common and update modules to use it

[sfromm]

Following up on #903, this adds get_bin_path to module_common and updates modules to use it.

Please let me know if you have any comments/questions. Thanks. :-)

[mpdehaan]

A few small things I found that would reduce the amount of boilerplate involved:

In a few places we have code like this in the module:

def get_bin_path(module, arg):
-    if os.path.exists('/usr/sbin/%s' % arg):
-        return '/usr/sbin/%s' % arg
-    elif os.path.exists('/sbin/%s' % arg):
-        return '/sbin/%s' % arg
-    else:
+    bin = module.get_bin_path(arg)  
+    if bin is None:
         module.fail_json(msg="Cannot find %s" % arg)
+    else:
+        return bin

Here, rather than just calling the get bin path inside another function, I think we'd use to add a module.get_bin_path(..., required=True) or something, such that we can eliminate this boilerplate as well across the board. If the path is not found it could automatically fail, which eliminates the need for the wrapper function as well as the "if bin is None" logic.

In the supervisorctl module:

  SUPERVISORCTL = _find_supervisorctl()
+    SUPERVISORCTL = module.get_bin_path('supervisorctl')
+    if SUPERVISORCTL is None:
+        module.fail_json(msg='supervisorctl is not installed')

It looks like the _find_supervisorctl function should be removed.

Other than that, looks solid.

[sfromm]

On Thu, Aug 30, 2012 at 5:16 AM, Michael DeHaan notifications@github.comwrote:

A few small things I found that would reduce the amount of boilerplate
involved:

In a few places we have code like this in the module:

def get_bin_path(module, arg):

• if os.path.exists('/usr/sbin/%s' % arg):
• return '/usr/sbin/%s' % arg
• elif os.path.exists('/sbin/%s' % arg):
• return '/sbin/%s' % arg
• else:
• bin = module.get_bin_path(arg)
• if bin is None: module.fail_json(msg="Cannot find %s" % arg)
• else:
• return bin

Here, rather than just calling the get bin path inside another function, I
think we'd use to add a module.get_bin_path(..., required=True) or
something, such that we can eliminate this boilerplate as well across the
board. If the path is not found it could automatically fail, which
eliminates the need for the wrapper function as well as the "if bin is
None" logic.

Sure, that will make the modules a bit cleaner. :-)

In the supervisorctl module:

SUPERVISORCTL = _find_supervisorctl()

• SUPERVISORCTL = module.get_bin_path('supervisorctl')
• if SUPERVISORCTL is None:
• module.fail_json(msg='supervisorctl is not installed')

It looks like the _find_supervisorctl function should be removed.

Other than that, looks solid.

I'll prune that from the supervisorctl module. I tried to tread lightly
through some of the modules; this was one of them. :-/

sf

[sfromm]

Commit 6742e9c should address the above issues.