openbsd_pkg: Fix when failed, PEP8-compliant (#26018)

This PR includes:
- PEP8 compliancy
- A fix to ensure the module fails when it failed for a package
- Various cosmetic changes to documentation
- Make `state: present` the default (and not required)

lib/ansible/modules/packaging/os/openbsd_pkg.py

@@ -13,111 +13,114 @@ ANSIBLE_METADATA = {'metadata_version': '1.1',
                     'status': ['preview'],
                     'supported_by': 'community'}
 
-
 DOCUMENTATION = '''
 ---
 module: openbsd_pkg
-author: "Patrik Lundin (@eest)"
+author:
+- Patrik Lundin (@eest)
 version_added: "1.1"
-short_description: Manage packages on OpenBSD.
+short_description: Manage packages on OpenBSD
 description:
     - Manage packages on OpenBSD using the pkg tools.
-requirements: [ "python >= 2.5" ]
+requirements:
+- python >= 2.5
 options:
     name:
-        required: true
         description:
         - Name of the package.
+        required: yes
     state:
-        required: true
-        choices: [ present, latest, absent ]
         description:
           - C(present) will make sure the package is installed.
             C(latest) will make sure the latest version of the package is installed.
             C(absent) will make sure the specified package is not installed.
+        choices: [ absent, latest, present ]
+        default: present
     build:
-        required: false
-        choices: [ yes, no ]
-        default: no
         description:
           - Build the package from source instead of downloading and installing
             a binary. Requires that the port source tree is already installed.
             Automatically builds and installs the 'sqlports' package, if it is
             not already installed.
+        type: bool
+        default: 'no'
         version_added: "2.1"
     ports_dir:
-        required: false
-        default: /usr/ports
         description:
-          - When used in combination with the 'build' option, allows overriding
+          - When used in combination with the C(build) option, allows overriding
             the default ports source directory.
+        default: /usr/ports
         version_added: "2.1"
     clean:
-        required: false
-        choices: [ yes, no ]
-        default: no
         description:
           - When updating or removing packages, delete the extra configuration
             file(s) in the old packages which are annotated with @extra in
             the packaging-list.
+        type: bool
+        default: 'no'
         version_added: "2.3"
     quick:
-        required: false
-        choices: [ yes, no ]
-        default: no
         description:
           - Replace or delete packages quickly; do not bother with checksums
             before removing normal files.
+        type: bool
+        default: 'no'
         version_added: "2.3"
 '''
 
 EXAMPLES = '''
-# Make sure nmap is installed
+- name: Make sure nmap is installed
-- openbsd_pkg:
+  openbsd_pkg:
     name: nmap
     state: present
 
-# Make sure nmap is the latest version
+- name: Make sure nmap is the latest version
-- openbsd_pkg:
+  openbsd_pkg:
     name: nmap
     state: latest
 
-# Make sure nmap is not installed
+- name: Make sure nmap is not installed
-- openbsd_pkg:
+  openbsd_pkg:
     name: nmap
     state: absent
 
-# Make sure nmap is installed, build it from source if it is not
+- name: Make sure nmap is installed, build it from source if it is not
-- openbsd_pkg:
+  openbsd_pkg:
     name: nmap
     state: present
     build: yes
 
-# Specify a pkg flavour with '--'
+- name: Specify a pkg flavour with '--'
-- openbsd_pkg:
+  openbsd_pkg:
     name: vim--no_x11
     state: present
 
-# Specify the default flavour to avoid ambiguity errors
+- name: Specify the default flavour to avoid ambiguity errors
-- openbsd_pkg:
+  openbsd_pkg:
     name: vim--
     state: present
 
-# Specify a package branch (requires at least OpenBSD 6.0)
+- name: Specify a package branch (requires at least OpenBSD 6.0)
-- openbsd_pkg:
+  openbsd_pkg:
     name: python%3.5
     state: present
 
-# Update all packages on the system
+- name: Update all packages on the system
-- openbsd_pkg:
+  openbsd_pkg:
     name: '*'
     state: latest
 
-# Purge a package and it's configuration files
+- name: Purge a package and it's configuration files
-- openbsd_pkg: name=mpd clean=yes state=absent
+  openbsd_pkg:
+    name: mpd
+    clean: yes
+    state: absent
 
-# Quickly remove a package without checking checksums
+- name: Quickly remove a package without checking checksums
-- openbsd_pkg: name=qt5 quick=yes state=absent
+  openbsd_pkg:
+    name: qt5
+    quick: yes
+    state: absent
 '''
 
 import os
@@ -128,6 +131,8 @@ import sqlite3
 
 from distutils.version import StrictVersion
 
+from ansible.module_utils.basic import AnsibleModule
+
 
 # Function used for executing commands.
 def execute_command(cmd, module):
@@ -137,6 +142,7 @@ def execute_command(cmd, module):
     cmd_args = shlex.split(cmd)
     return module.run_command(cmd_args)
 
+
 # Function used to find out if a package is currently installed.
 def get_package_state(names, pkg_spec, module):
     info_cmd = 'pkg_info -Iq'
@@ -158,6 +164,7 @@ def get_package_state(names, pkg_spec, module):
         else:
             pkg_spec[name]['installed_state'] = False
 
+
 # Function used to make sure a package is present.
 def package_present(names, pkg_spec, module):
     build = module.params['build']
@@ -247,6 +254,7 @@ def package_present(names, pkg_spec, module):
             pkg_spec[name]['stderr'] = ''
             pkg_spec[name]['changed'] = False
 
+
 # Function used to make sure a package is the latest available version.
 def package_latest(names, pkg_spec, module):
     if module.params['build'] is True:
@@ -306,6 +314,7 @@ def package_latest(names, pkg_spec, module):
         module.debug("package_latest(): calling package_present() to handle leftovers")
         package_present(names, pkg_spec, module)
 
+
 # Function used to make sure a package is not installed.
 def package_absent(names, pkg_spec, module):
     remove_cmd = 'pkg_delete -I'
@@ -335,6 +344,7 @@ def package_absent(names, pkg_spec, module):
             pkg_spec[name]['stderr'] = ''
             pkg_spec[name]['changed'] = False
 
+
 # Function used to parse the package name based on packages-specs(7).
 # The general name structure is "stem-version[-flavors]".
 #
@@ -371,16 +381,8 @@ def parse_package_name(names, pkg_spec, module):
                 pkg_spec[name]['flavor'] = match.group('flavor')
                 pkg_spec[name]['branch'] = match.group('branch')
                 pkg_spec[name]['style'] = 'version'
-                module.debug("version_match: stem: %s, version: %s, flavor_separator: %s, flavor: %s, branch: %s, style: %s" %
+                module.debug("version_match: stem: %(stem)s, version: %(version)s, flavor_separator: %(flavor_separator)s, "
-                    (
+                             "flavor: %(flavor)s, branch: %(branch)s, style: %(style)s" % pkg_spec[name])
-                        pkg_spec[name]['stem'],
-                        pkg_spec[name]['version'],
-                        pkg_spec[name]['flavor_separator'],
-                        pkg_spec[name]['flavor'],
-                        pkg_spec[name]['branch'],
-                        pkg_spec[name]['style']
-                    )
-                )
             else:
                 module.fail_json(msg="unable to parse package name at version_match: " + name)
 
@@ -395,14 +397,7 @@ def parse_package_name(names, pkg_spec, module):
                 pkg_spec[name]['flavor'] = match.group('flavor')
                 pkg_spec[name]['branch'] = match.group('branch')
                 pkg_spec[name]['style'] = 'versionless'
-                module.debug("versionless_match: stem: %s, flavor: %s, branch: %s, style: %s" %
+                module.debug("versionless_match: stem: %(stem)s, flavor: %(flavor)s, branch: %(branch)s, style: %(style)s" % pkg_spec[name])
-                    (
-                        pkg_spec[name]['stem'],
-                        pkg_spec[name]['flavor'],
-                        pkg_spec[name]['branch'],
-                        pkg_spec[name]['style']
-                    )
-                )
             else:
                 module.fail_json(msg="unable to parse package name at versionless_match: " + name)
 
@@ -419,13 +414,7 @@ def parse_package_name(names, pkg_spec, module):
                 pkg_spec[name]['flavor'] = None
                 pkg_spec[name]['branch'] = match.group('branch')
                 pkg_spec[name]['style'] = 'stem'
-                module.debug("stem_match: stem: %s, branch: %s, style: %s" %
+                module.debug("stem_match: stem: %(stem)s, branch: %(branch)s, style: %(style)s" % pkg_spec[name])
-                    (
-                        pkg_spec[name]['stem'],
-                        pkg_spec[name]['branch'],
-                        pkg_spec[name]['style']
-                    )
-                )
             else:
                 module.fail_json(msg="unable to parse package name at else: " + name)
 
@@ -443,6 +432,7 @@ def parse_package_name(names, pkg_spec, module):
             if match:
                 module.fail_json(msg="trailing dash in flavor: " + pkg_spec[name]['flavor'])
 
+
 # Function used for figuring out the port path.
 def get_package_source_path(name, pkg_spec, module):
     pkg_spec[name]['subpackage'] = None
@@ -483,7 +473,7 @@ def get_package_source_path(name, pkg_spec, module):
         if len(results) < 1:
             module.fail_json(msg="could not find a port by the name '%s'" % name)
         if len(results) > 1:
-            matches = map(lambda x:x[1], results)
+            matches = map(lambda x: x[1], results)
             module.fail_json(msg="too many matches, unsure which to build: %s" % ' OR '.join(matches))
 
         # there's exactly 1 match, so figure out the subpackage, if any, then return
@@ -493,6 +483,7 @@ def get_package_source_path(name, pkg_spec, module):
             pkg_spec[name]['subpackage'] = parts[1]
         return parts[0]
 
+
 # Function used for upgrading all installed packages.
 def upgrade_packages(pkg_spec, module):
     if module.check_mode:
@@ -522,20 +513,20 @@ def upgrade_packages(pkg_spec, module):
     else:
         pkg_spec['*']['rc'] = 0
 
+
 # ===========================================
 # Main control flow.
-
 def main():
     module = AnsibleModule(
-        argument_spec = dict(
+        argument_spec=dict(
-            name = dict(required=True, type='list'),
+            name=dict(type='list', required=True),
-            state = dict(required=True, choices=['absent', 'installed', 'latest', 'present', 'removed']),
+            state=dict(type='str', default='present', choices=['absent', 'installed', 'latest', 'present', 'removed']),
-            build = dict(default='no', type='bool'),
+            build=dict(type='bool', default=False),
-            ports_dir = dict(default='/usr/ports'),
+            ports_dir=dict(type='path', default='/usr/ports'),
-            quick = dict(default='no', type='bool'),
+            quick=dict(type='bool', default=False),
-            clean = dict(default='no', type='bool')
+            clean=dict(type='bool', default=False),
         ),
-        supports_check_mode = True
+        supports_check_mode=True
     )
 
     name = module.params['name']
@@ -604,6 +595,10 @@ def main():
     # is changed this is set to True.
     combined_changed = False
 
+    # The combined failed status for all requested packages. If anything
+    # failed this is set to True.
+    combined_failed = False
+
     # We combine all error messages in this comma separated string, for example:
     # "msg": "Can't find nmapp\n, Can't find nmappp\n"
     combined_error_message = ''
@@ -612,6 +607,7 @@ def main():
     # changed.
     for n in name:
         if pkg_spec[n]['rc'] != 0:
+            combined_failed = True
             if pkg_spec[n]['stderr']:
                 if combined_error_message:
                     combined_error_message += ", %s" % pkg_spec[n]['stderr']
@@ -628,15 +624,12 @@ def main():
 
     # If combined_error_message contains anything at least some part of the
     # list of requested package names failed.
-    if combined_error_message:
+    if combined_failed:
-        module.fail_json(msg=combined_error_message)
+        module.fail_json(msg=combined_error_message, **result)
 
     result['changed'] = combined_changed
 
     module.exit_json(**result)
 
-# Import module snippets.
-from ansible.module_utils.basic import *
-
 if __name__ == '__main__':
     main()

test/sanity/pep8/legacy-files.txt

@@ -390,7 +390,6 @@ lib/ansible/modules/packaging/os/homebrew.py
 lib/ansible/modules/packaging/os/homebrew_cask.py
 lib/ansible/modules/packaging/os/layman.py
 lib/ansible/modules/packaging/os/macports.py
-lib/ansible/modules/packaging/os/openbsd_pkg.py
 lib/ansible/modules/packaging/os/opkg.py
 lib/ansible/modules/packaging/os/pacman.py
 lib/ansible/modules/packaging/os/pkg5.py