homebrew: Move repeated logic from homebrew modules into module_utils (#8324)

* gomebrew: Move repeated logic from homebrew modules into module_utils

Fixes #8323.

* ghangelog + unit test improvement

* Update changelogs/fragments/8323-refactor-homebrew-logic-module-utils.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>

changelogs/fragments/8323-refactor-homebrew-logic-module-utils.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - "homebrew, homebrew_cask - refactor common argument validation logic into a dedicated ``homebrew`` module utils (https://github.com/ansible-collections/community.general/issues/8323, https://github.com/ansible-collections/community.general/pull/8324)."

plugins/module_utils/homebrew.py

@@ -0,0 +1,115 @@
+# -*- coding: utf-8 -*-
+# Copyright (c) Ansible project
+# Simplified BSD License (see LICENSES/BSD-2-Clause.txt or https://opensource.org/licenses/BSD-2-Clause)
+# SPDX-License-Identifier: BSD-2-Clause
+
+from __future__ import absolute_import, division, print_function
+
+__metaclass__ = type
+
+import os
+import re
+from ansible.module_utils.six import string_types
+
+
+def _create_regex_group_complement(s):
+    lines = (line.strip() for line in s.split("\n") if line.strip())
+    chars = filter(None, (line.split("#")[0].strip() for line in lines))
+    group = r"[^" + r"".join(chars) + r"]"
+    return re.compile(group)
+
+
+class HomebrewValidate(object):
+    # class regexes ------------------------------------------------ {{{
+    VALID_PATH_CHARS = r"""
+        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
+        \s                  # spaces
+        :                   # colons
+        {sep}               # the OS-specific path separator
+        .                   # dots
+        \-                  # dashes
+    """.format(
+        sep=os.path.sep
+    )
+
+    VALID_BREW_PATH_CHARS = r"""
+        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
+        \s                  # spaces
+        {sep}               # the OS-specific path separator
+        .                   # dots
+        \-                  # dashes
+    """.format(
+        sep=os.path.sep
+    )
+
+    VALID_PACKAGE_CHARS = r"""
+        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
+        .                   # dots
+        /                   # slash (for taps)
+        \+                  # plusses
+        \-                  # dashes
+        :                   # colons (for URLs)
+        @                   # at-sign
+    """
+
+    INVALID_PATH_REGEX = _create_regex_group_complement(VALID_PATH_CHARS)
+    INVALID_BREW_PATH_REGEX = _create_regex_group_complement(VALID_BREW_PATH_CHARS)
+    INVALID_PACKAGE_REGEX = _create_regex_group_complement(VALID_PACKAGE_CHARS)
+    # /class regexes ----------------------------------------------- }}}
+
+    # class validations -------------------------------------------- {{{
+    @classmethod
+    def valid_path(cls, path):
+        """
+        `path` must be one of:
+         - list of paths
+         - a string containing only:
+             - alphanumeric characters
+             - dashes
+             - dots
+             - spaces
+             - colons
+             - os.path.sep
+        """
+
+        if isinstance(path, string_types):
+            return not cls.INVALID_PATH_REGEX.search(path)
+
+        try:
+            iter(path)
+        except TypeError:
+            return False
+        else:
+            paths = path
+            return all(cls.valid_brew_path(path_) for path_ in paths)
+
+    @classmethod
+    def valid_brew_path(cls, brew_path):
+        """
+        `brew_path` must be one of:
+         - None
+         - a string containing only:
+             - alphanumeric characters
+             - dashes
+             - dots
+             - spaces
+             - os.path.sep
+        """
+
+        if brew_path is None:
+            return True
+
+        return isinstance(
+            brew_path, string_types
+        ) and not cls.INVALID_BREW_PATH_REGEX.search(brew_path)
+
+    @classmethod
+    def valid_package(cls, package):
+        """A valid package is either None or alphanumeric."""
+
+        if package is None:
+            return True
+
+        return isinstance(
+            package, string_types
+        ) and not cls.INVALID_PACKAGE_REGEX.search(package)

plugins/modules/homebrew.py

@@ -179,9 +179,10 @@ changed_pkgs:
 '''
 
 import json
-import os.path
 import re
 
+from ansible_collections.community.general.plugins.module_utils.homebrew import HomebrewValidate
+
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.six import iteritems, string_types
 
@@ -208,98 +209,7 @@ def _check_package_in_json(json_output, package_type):
 class Homebrew(object):
     '''A class to manage Homebrew packages.'''
 
-    # class regexes ------------------------------------------------ {{{
-    VALID_PATH_CHARS = r'''
-        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
-        \s                  # spaces
-        :                   # colons
-        {sep}               # the OS-specific path separator
-        .                   # dots
-        \-                  # dashes
-    '''.format(sep=os.path.sep)
-
-    VALID_BREW_PATH_CHARS = r'''
-        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
-        \s                  # spaces
-        {sep}               # the OS-specific path separator
-        .                   # dots
-        \-                  # dashes
-    '''.format(sep=os.path.sep)
-
-    VALID_PACKAGE_CHARS = r'''
-        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
-        .                   # dots
-        /                   # slash (for taps)
-        \+                  # plusses
-        \-                  # dashes
-        :                   # colons (for URLs)
-        @                   # at-sign
-    '''
-
-    INVALID_PATH_REGEX = _create_regex_group_complement(VALID_PATH_CHARS)
-    INVALID_BREW_PATH_REGEX = _create_regex_group_complement(VALID_BREW_PATH_CHARS)
-    INVALID_PACKAGE_REGEX = _create_regex_group_complement(VALID_PACKAGE_CHARS)
-    # /class regexes ----------------------------------------------- }}}
-
     # class validations -------------------------------------------- {{{
-    @classmethod
-    def valid_path(cls, path):
-        '''
-        `path` must be one of:
-         - list of paths
-         - a string containing only:
-             - alphanumeric characters
-             - dashes
-             - dots
-             - spaces
-             - colons
-             - os.path.sep
-        '''
-
-        if isinstance(path, string_types):
-            return not cls.INVALID_PATH_REGEX.search(path)
-
-        try:
-            iter(path)
-        except TypeError:
-            return False
-        else:
-            paths = path
-            return all(cls.valid_brew_path(path_) for path_ in paths)
-
-    @classmethod
-    def valid_brew_path(cls, brew_path):
-        '''
-        `brew_path` must be one of:
-         - None
-         - a string containing only:
-             - alphanumeric characters
-             - dashes
-             - dots
-             - spaces
-             - os.path.sep
-        '''
-
-        if brew_path is None:
-            return True
-
-        return (
-            isinstance(brew_path, string_types)
-            and not cls.INVALID_BREW_PATH_REGEX.search(brew_path)
-        )
-
-    @classmethod
-    def valid_package(cls, package):
-        '''A valid package is either None or alphanumeric.'''
-
-        if package is None:
-            return True
-
-        return (
-            isinstance(package, string_types)
-            and not cls.INVALID_PACKAGE_REGEX.search(package)
-        )
-
     @classmethod
     def valid_state(cls, state):
         '''
@@ -359,7 +269,7 @@ class Homebrew(object):
 
     @path.setter
     def path(self, path):
-        if not self.valid_path(path):
+        if not HomebrewValidate.valid_path(path):
             self._path = []
             self.failed = True
             self.message = 'Invalid path: {0}.'.format(path)
@@ -379,7 +289,7 @@ class Homebrew(object):
 
     @brew_path.setter
     def brew_path(self, brew_path):
-        if not self.valid_brew_path(brew_path):
+        if not HomebrewValidate.valid_brew_path(brew_path):
             self._brew_path = None
             self.failed = True
             self.message = 'Invalid brew_path: {0}.'.format(brew_path)
@@ -404,7 +314,7 @@ class Homebrew(object):
 
     @current_package.setter
     def current_package(self, package):
-        if not self.valid_package(package):
+        if not HomebrewValidate.valid_package(package):
             self._current_package = None
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(package)
@@ -491,7 +401,7 @@ class Homebrew(object):
 
     # checks ------------------------------------------------------- {{{
     def _current_package_is_installed(self):
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(self.current_package)
             raise HomebrewException(self.message)
@@ -514,7 +424,7 @@ class Homebrew(object):
         return _check_package_in_json(data, "formulae") or _check_package_in_json(data, "casks")
 
     def _current_package_is_outdated(self):
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             return False
 
         rc, out, err = self.module.run_command([
@@ -526,7 +436,7 @@ class Homebrew(object):
         return rc != 0
 
     def _current_package_is_installed_from_head(self):
-        if not Homebrew.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             return False
         elif not self._current_package_is_installed():
             return False
@@ -624,7 +534,7 @@ class Homebrew(object):
 
     # installed ------------------------------ {{{
     def _install_current_package(self):
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(self.current_package)
             raise HomebrewException(self.message)
@@ -685,7 +595,7 @@ class Homebrew(object):
     def _upgrade_current_package(self):
         command = 'upgrade'
 
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(self.current_package)
             raise HomebrewException(self.message)
@@ -756,7 +666,7 @@ class Homebrew(object):
 
     # uninstalled ---------------------------- {{{
     def _uninstall_current_package(self):
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(self.current_package)
             raise HomebrewException(self.message)
@@ -805,7 +715,7 @@ class Homebrew(object):
 
     # linked --------------------------------- {{{
     def _link_current_package(self):
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(self.current_package)
             raise HomebrewException(self.message)
@@ -852,7 +762,7 @@ class Homebrew(object):
 
     # unlinked ------------------------------- {{{
     def _unlink_current_package(self):
-        if not self.valid_package(self.current_package):
+        if not HomebrewValidate.valid_package(self.current_package):
             self.failed = True
             self.message = 'Invalid package: {0}.'.format(self.current_package)
             raise HomebrewException(self.message)

plugins/modules/homebrew_cask.py

@@ -158,6 +158,7 @@ import re
 import tempfile
 
 from ansible_collections.community.general.plugins.module_utils.version import LooseVersion
+from ansible_collections.community.general.plugins.module_utils.homebrew import HomebrewValidate
 
 from ansible.module_utils.common.text.converters import to_bytes
 from ansible.module_utils.basic import AnsibleModule
@@ -183,23 +184,6 @@ class HomebrewCask(object):
     '''A class to manage Homebrew casks.'''
 
     # class regexes ------------------------------------------------ {{{
-    VALID_PATH_CHARS = r'''
-        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
-        \s                  # spaces
-        :                   # colons
-        {sep}               # the OS-specific path separator
-        .                   # dots
-        \-                  # dashes
-    '''.format(sep=os.path.sep)
-
-    VALID_BREW_PATH_CHARS = r'''
-        \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
-        \s                  # spaces
-        {sep}               # the OS-specific path separator
-        .                   # dots
-        \-                  # dashes
-    '''.format(sep=os.path.sep)
-
     VALID_CASK_CHARS = r'''
         \w                  # alphanumeric characters (i.e., [a-zA-Z0-9_])
         .                   # dots
@@ -208,58 +192,10 @@ class HomebrewCask(object):
         @                   # at symbol
     '''
 
-    INVALID_PATH_REGEX = _create_regex_group_complement(VALID_PATH_CHARS)
-    INVALID_BREW_PATH_REGEX = _create_regex_group_complement(VALID_BREW_PATH_CHARS)
     INVALID_CASK_REGEX = _create_regex_group_complement(VALID_CASK_CHARS)
     # /class regexes ----------------------------------------------- }}}
 
     # class validations -------------------------------------------- {{{
-    @classmethod
-    def valid_path(cls, path):
-        '''
-        `path` must be one of:
-         - list of paths
-         - a string containing only:
-             - alphanumeric characters
-             - dashes
-             - dots
-             - spaces
-             - colons
-             - os.path.sep
-        '''
-
-        if isinstance(path, (string_types)):
-            return not cls.INVALID_PATH_REGEX.search(path)
-
-        try:
-            iter(path)
-        except TypeError:
-            return False
-        else:
-            paths = path
-            return all(cls.valid_brew_path(path_) for path_ in paths)
-
-    @classmethod
-    def valid_brew_path(cls, brew_path):
-        '''
-        `brew_path` must be one of:
-         - None
-         - a string containing only:
-             - alphanumeric characters
-             - dashes
-             - dots
-             - spaces
-             - os.path.sep
-        '''
-
-        if brew_path is None:
-            return True
-
-        return (
-            isinstance(brew_path, string_types)
-            and not cls.INVALID_BREW_PATH_REGEX.search(brew_path)
-        )
-
     @classmethod
     def valid_cask(cls, cask):
         '''A valid cask is either None or alphanumeric + backslashes.'''
@@ -321,7 +257,7 @@ class HomebrewCask(object):
 
     @path.setter
     def path(self, path):
-        if not self.valid_path(path):
+        if not HomebrewValidate.valid_path(path):
             self._path = []
             self.failed = True
             self.message = 'Invalid path: {0}.'.format(path)
@@ -341,7 +277,7 @@ class HomebrewCask(object):
 
     @brew_path.setter
     def brew_path(self, brew_path):
-        if not self.valid_brew_path(brew_path):
+        if not HomebrewValidate.valid_brew_path(brew_path):
             self._brew_path = None
             self.failed = True
             self.message = 'Invalid brew_path: {0}.'.format(brew_path)

tests/unit/plugins/modules/test_homebrew.py

@@ -2,23 +2,28 @@
 # GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
 # SPDX-License-Identifier: GPL-3.0-or-later
 
-from __future__ import (absolute_import, division, print_function)
+from __future__ import absolute_import, division, print_function
 
 __metaclass__ = type
 
 from ansible_collections.community.general.tests.unit.compat import unittest
-from ansible_collections.community.general.plugins.modules.homebrew import Homebrew
+from ansible_collections.community.general.plugins.module_utils.homebrew import HomebrewValidate
 
 
 class TestHomebrewModule(unittest.TestCase):
 
     def setUp(self):
-        self.brew_app_names = [
+        self.brew_app_names = ["git-ssh", "awscli@1", "bash"]
-            'git-ssh',
+
-            'awscli@1',
+        self.invalid_names = [
-            'bash'
+            "git ssh",
+            "git*",
         ]
 
     def test_valid_package_names(self):
         for name in self.brew_app_names:
-            self.assertTrue(Homebrew.valid_package(name))
+            self.assertTrue(HomebrewValidate.valid_package(name))
+
+    def test_invalid_package_names(self):
+        for name in self.invalid_names:
+            self.assertFalse(HomebrewValidate.valid_package(name))