Linting hardening with pre-commit

- Adds bashate test using v0.6.0
- Bump pre-commit-hooks release to v2.2.4
- Bump ansible-lint release to v4.1.0a0
- Fix some minor flake8 errors

Change-Id: I66b796fab1d8651163226febbc4e99648a9ecc6a
Signed-off-by: Gael Chamoulaud <gchamoul@redhat.com>

.pre-commit-config.yaml

@@ -1,8 +1,9 @@
 ---
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v2.0.0
+    rev: v2.1.0
     hooks:
+      - id: end-of-file-fixer
       - id: trailing-whitespace
       - id: mixed-line-ending
       - id: check-byte-order-marker
@@ -10,20 +11,31 @@ repos:
       - id: check-merge-conflict
       - id: debug-statements
       - id: flake8
-        additional_dependencies:
-          - hacking<1.2.0,>=1.1.0
+        entry: flake8 --ignore=E123,E125,W503,W504,W605
       - id: check-yaml
         files: .*\.(yaml|yml)$
   - repo: https://github.com/adrienverge/yamllint.git
-    rev: v1.13.0
+    rev: v1.15.0
     hooks:
       - id: yamllint
         files: \.(yaml|yml)$
         types: [file, yaml]
         entry: yamllint --strict -f parsable
   - repo: https://github.com/ansible/ansible-lint
-    rev: v4.1.0
+    rev: v4.1.0a0
     hooks:
       - id: ansible-lint
         files: \.(yaml|yml)$
         entry: ansible-lint --force-color -v
+  - repo: https://github.com/openstack-dev/bashate.git
+    rev: 0.6.0
+    hooks:
+      - id: bashate
+        entry: bashate --error . --verbose --ignore=E006,E040
+        # Run bashate check for all bash scripts
+        # Ignores the following rules:
+        # E006: Line longer than 79 columns (as many scripts use jinja
+        #       templating, this is very difficult)
+        # E040: Syntax error determined using `bash -n` (as many scripts
+        #       use jinja templating, this will often fail and the syntax
+        #       error will be discovered in execution anyway)

bindep.txt

@@ -0,0 +1,33 @@
+# This file facilitates OpenStack-CI package installation
+# before the execution of any tests.
+#
+# See the following for details:
+#  - https://docs.openstack.org/infra/bindep/
+#  - https://opendev.org/opendev/bindep/
+#
+# Even if the role does not make use of this facility, it
+# is better to have this file empty, otherwise OpenStack-CI
+# will fall back to installing its default packages which
+# will potentially be detrimental to the tests executed.
+
+# The gcc compiler
+gcc
+
+# Base requirements for RPM distros
+gcc-c++           [platform:rpm]
+git               [platform:rpm]
+libffi-devel      [platform:rpm]
+openssl-devel     [platform:rpm]
+python-devel      [platform:rpm]
+python2-dnf       [platform:fedora]
+python-virtualenv [platform:rpm]
+
+# For SELinux
+libselinux-python  [platform:rpm]
+libsemanage-python [platform:redhat]
+
+# Required for compressing collected log files in CI
+gzip
+
+# Required to build language docs
+gettext

doc/requirements.txt

@@ -0,0 +1,7 @@
+# this is required for the docs build jobs
+sphinx!=1.6.6,!=1.6.7,>=1.6.2,<2.0.0;python_version=='2.7'  # BSD
+sphinx!=1.6.6,!=1.6.7,!=2.1.0,>=1.6.2;python_version>='3.4'  # BSD
+openstackdocstheme>=1.29.2 # Apache-2.0
+reno>=2.11.3 # Apache-2.0
+doc8>=0.8.0 # Apache-2.0
+bashate>=0.6.0 # Apache-2.0

doc/source/_exts/generate_validations_doc.py

@@ -58,10 +58,14 @@ def get_validation_parameters(validation):
 
 
 def build_summary(group, validations):
-    entries = ["* :ref:`{}`: {}".format(group + '_' + validation['id'], validation['name'])
-               for validation in validations]
+    entries = [
+        "* :ref:`{}`: {}".format(group + '_' + validation['id'],
+                                 validation['name'])
+        for validation in validations
+    ]
     with open('doc/source/validations-{}.rst'.format(group), 'w') as f:
         f.write("\n".join(entries))
+        f.write("\n")
 
 
 def format_dict(my_dict):
@@ -83,13 +87,15 @@ def build_detail(group, validations):
 
 - **hosts**: {hosts}
 - **groups**: {groups}
-- **metadata**: {metadata}
-- **parameters**: {parameters}
+- **parameters**:{parameters}
 - **roles**: {roles}
 
+Role documentation
+
 .. toctree::
 
    roles/role-{roles}
+
 """
         .format(label=(group + '_' + validation['id']),
                 title=validation['id'],

doc/source/conf.py

@@ -1,3 +1,4 @@
+#!/usr/bin/env python
 # -*- coding: utf-8 -*-
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.

doc/source/index.rst

@@ -4,7 +4,7 @@
    contain the root `toctree` directive.
 
 Welcome to tripleo-validations's documentation!
-========================================================
+===============================================
 
 Contents:
 
@@ -13,7 +13,6 @@ Contents:
 
    readme
    installation
-   usage
    contributing
    roles
    modules
@@ -38,4 +37,3 @@ Indices and tables
 
 * :ref:`genindex`
 * :ref:`search`
-

doc/source/readme.rst

@@ -69,7 +69,7 @@ introspection.
 Pre Deployment
 ~~~~~~~~~~~~~~
 
-Validation that are run right before deploying the overcloud.
+Validations that are run right before deploying the overcloud.
 
 .. include:: validations-pre-deployment.rst
 
@@ -94,6 +94,20 @@ Validations that are run right before a major upgrade of the undercloud or overc
 
 .. include:: validations-pre-upgrade.rst
 
+Post Upgrade
+~~~~~~~~~~~~
+
+Validations that are run right after a major upgrade of the undercloud or overcloud.
+
+.. include:: validations-post-upgrade.rst
+
+OpenShift On OpenStack
+~~~~~~~~~~~~~~~~~~~~~~
+
+Validations that are run right after the undercloud is installed.
+
+.. include:: validations-openshift-on-openstack.rst
+
 
 Writing Validations
 -------------------

doc/source/roles/role-check-latest-minor-version.rst

@@ -1,7 +0,0 @@
-==========================
-check-latest-minor-version
-==========================
-
-.. ansibleautoplugin::
-   :role: roles/check-latest-minor-version
-

doc/source/roles/role-check-latest-packages-version.rst

@@ -0,0 +1,6 @@
+=============================
+check-latest-packages-version
+=============================
+
+.. ansibleautoplugin::
+   :role: roles/check-latest-packages-version

library/check_ironic_boot_config.py

@@ -84,6 +84,7 @@ def _all_possible_names(arch, platform, image_name_base):
         yield _name_helper(image_name_base, arch=arch)
     yield _name_helper(image_name_base)
 
+
 MISMATCH = (
     "\nNode {} has an incorrectly configured driver_info/deploy_{}. Expected "
     "{} but got {}."
@@ -182,5 +183,6 @@ def main():
     else:
         module.exit_json()
 
+
 if __name__ == '__main__':
     main()

library/docker_facts.py

@@ -17,6 +17,10 @@
 from __future__ import absolute_import
 from __future__ import division
 from __future__ import print_function
+import itertools
+
+from ansible.module_utils.basic import AnsibleModule
+
 import six
 six.add_metaclass(type)
 
@@ -114,10 +118,6 @@ docker:
         type: list
 """
 
-import itertools
-
-from ansible.module_utils.basic import AnsibleModule
-
 DOCKER_SUBCOMMAND_LOOKUP = [
     ('images', 'images', '-q'),
     ('volumes', 'volume ls', '-q'),

library/haproxy_conf.py

@@ -56,14 +56,14 @@ def generic_ini_style_conf_parser(file_path, section_regex, option_regex):
                 config[current_section] = {}
             match_option = re.match(option_regex, line)
             if match_option and current_section:
-                option = re.sub('\s+', ' ', match_option.group(1))
+                option = re.sub(r'\s+', ' ', match_option.group(1))
                 config[current_section][option] = match_option.group(2)
     return config
 
 
 def parse_haproxy_conf(file_path):
-    section_regex = '^(\w+)'
-    option_regex = '^(?:\s+)(\w+(?:\s+\w+)*?)\s+([\w/]*)$'
+    section_regex = r'^(\w+)'
+    option_regex = r'^(?:\s+)(\w+(?:\s+\w+)*?)\s+([\w/]*)$'
     return generic_ini_style_conf_parser(file_path, section_regex,
                                          option_regex)
 

library/ini.py

@@ -79,6 +79,7 @@ def get_result(path, section, key):
         ret = ReturnValue.KEY_NOT_FOUND
         return (ret, msg, value)
 
+
 DOCUMENTATION = '''
 ---
 module: ini

lookup_plugins/ironic_nodes.py

@@ -15,6 +15,10 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from ansible.plugins.lookup import LookupBase
+
+from tripleo_validations import utils
+
 DOCUMENTATION = """
     lookup: ironic_nodes
     description: Retrieve node information from Ironic
@@ -65,10 +69,6 @@ _raw:
     description: A Python list with results from the API call.
 """
 
-from ansible.plugins.lookup import LookupBase
-
-from tripleo_validations import utils
-
 
 class LookupModule(LookupBase):
 

lookup_plugins/nova_servers.py

@@ -15,6 +15,12 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from ansible.plugins.lookup import LookupBase
+from novaclient.exceptions import NotFound
+
+from tripleo_validations import utils
+
+
 DOCUMENTATION = """
     lookup: nova_servers
     description: Retrieve server information from Nova
@@ -51,11 +57,6 @@ _raw:
     description: A Python list with results from the API call.
 """
 
-from ansible.plugins.lookup import LookupBase
-from novaclient.exceptions import NotFound
-
-from tripleo_validations import utils
-
 
 class LookupModule(LookupBase):
 

molecule-requirements.txt

@@ -0,0 +1,10 @@
+# this is required for the molecule jobs
+ansible
+ansi2html
+docker
+pytest
+pytest-cov
+pytest-html
+pytest-xdist
+mock
+molecule>=2.22rc1

playbooks/nova-status.yaml

@@ -14,10 +14,8 @@
         The nova-status upgrade check command has three standard return codes:
 
         0 -> All upgrade readiness checks passed successfully and there is nothing to do.
-        1 -> At least one check encountered an issue and requires further investigation.
-             This is considered a warning but the upgrade may be OK.
-        2 -> There was an upgrade status check failure that needs to be investigated.
-             This should be considered something that stops an upgrade.
+        1 -> At least one check encountered an issue and requires further investigation. This is considered a warning but the upgrade may be OK.
+        2 -> There was an upgrade status check failure that needs to be investigated. This should be considered something that stops an upgrade.
       groups:
         - pre-upgrade
   roles:

playbooks/openshift-hw-requirements.yaml

@@ -7,15 +7,10 @@
         Check if there are enough resources for an OpenShift deployment on top
         of Openstack
         deployment:
-        - Is there a flavor that meets the minimum requirements for a test
-          environment?
-          (4GB RAM, 40GB disk)
-        - Is there a flavor that meets the minimum requirements for a
-          production environment?
-          (16GB RAM, 40GB disk, 4 VCPUs)
+        - Is there a flavor that meets the minimum requirements for a test environment? (4GB RAM, 40GB disk)
+        - Is there a flavor that meets the minimum requirements for a production environment? (16GB RAM, 40GB disk, 4 VCPUs)
         - Are images named centos or rhel available?
-        - Are there sufficient compute resources available for a default setup?
-          (1 Master node, 1 Infra node, 2 App nodes)
+        - Are there sufficient compute resources available for a default setup? (1 Master node, 1 Infra node, 2 App nodes)
       groups:
       - openshift-on-openstack
     min_total_ram_testing: 16384    # 4 per node
@@ -33,4 +28,4 @@
   tasks:
     - include_role:
         name: openshift-on-openstack
-        tasks_from: openshift-hw-requirements
+        tasks_from: openshift-hw-requirements.yaml

playbooks/openshift-nw-requirements.yaml

@@ -11,4 +11,4 @@
   tasks:
     - include_role:
         name: openshift-on-openstack
-        tasks_from: openshift-nw-requirements
+        tasks_from: openshift-nw-requirements.yaml

playbooks/stack-health.yaml

@@ -4,7 +4,7 @@
     metadata:
       name: Stack Health Check
       description: >
-        Check if all stack resources are in a *_COMPLETE state before starting
+        Check if all stack resources are in a 'COMPLETE' state before starting
         an upgrade.
       groups:
         - pre-upgrade

releasenotes/notes/ip-range-validation-result-daddc8c015dd34c0.yaml

@@ -2,5 +2,5 @@
 fixes:
   - |
     Instead of only outputting a warning, the IP range validation now fails if
-    the number of available addresses is lower than the recommended minimum. 
+    the number of available addresses is lower than the recommended minimum.
     (Fixes https://bugs.launchpad.net/tripleo/+bug/1713483)

roles/openshift-on-openstack/tasks/openshift-hw-requirements.yaml

@@ -132,14 +132,16 @@
     warning_msg: |
       {{ lookup('template', './templates/openshift-hw-requirements-warnings.j2') }}
 
-- name: Fail if minimum requirements aren't met
-  fail: msg="{{ warning_msg }}"
+- name: Fail if minimum requirements are not met
+  fail:
+    msg: "{{ warning_msg }}"
   when: not matching_flavors_testing
         or not matching_image
         or not resource_reqs_testing
 
-- name: Warn if production requirements aren't met
-  warn: msg="{{ warning_msg }}"
+- name: Warn if production requirements are not met
+  warn:
+    msg: "{{ warning_msg }}"
   when: not matching_flavors_prod
         or not matching_image
         or not resource_reqs_prod

roles/tls-everywhere/tasks/pre-deployment-containerized.yaml

@@ -3,6 +3,7 @@
 - name: Verify that join.conf exists (containzerized)
   command: "{{ command_prefix }} exec novajoin_server test -e /etc/novajoin/join.conf"
   register: containerized_join_conf_st
+  changed_when: False
   become: true
 
 - name: Fail if join.conf is not present (containerized)
@@ -87,9 +88,10 @@
         - containerized_novajoin_principal_usable_result is succeeded
 
     - name: Clean up temporary kerberos cache on novajoin_server container
-      shell: "{{ command_prefix }} exec novajoin_server kdestroy -c {{ item }}"
+      command: "{{ command_prefix }} exec novajoin_server kdestroy -c {{ item }}"
       with_items: "{{ temp_krb_caches }}"
       ignore_errors: false
+      changed_when: False
       become: true
       when:
         - containerized_novajoin_krb5_keytab_stat.rc == 0
@@ -199,4 +201,3 @@
         report_status: "{{ service_running_status }}"
         report_reason: "{{ service_running_reason }}"
         report_recommendations: "{{ service_running_recommendations }}"
-

test-requirements.txt

@@ -15,3 +15,4 @@ testscenarios>=0.4 # Apache-2.0/BSD
 testtools>=2.2.0 # MIT
 reno>=2.5.0 # Apache-2.0
 netaddr>=0.7.18 # BSD
+pre-commit # MIT

tools/releasenotes_tox.sh

@@ -11,8 +11,7 @@ BUILD_RESULT=$?
 UNCOMMITTED_NOTES=$(git status --porcelain | \
     awk '$1 == "M" && $2 ~ /releasenotes\/notes/ {print $2}')
 
-if [ "${UNCOMMITTED_NOTES}" ]
-then
+if [ "${UNCOMMITTED_NOTES}" ]; then
     cat <<EOF
 
 REMINDER: The following changes to release notes have not been committed:

tools/validate-files.py

@@ -48,6 +48,7 @@ def parse_args():
 
     return p.parse_args()
 
+
 args = parse_args()
 path_args = args.path_args
 quiet = args.quiet

tox.ini

@@ -1,63 +1,128 @@
 [tox]
-minversion = 3.8
-envlist = pep8,py27,py37,molecule
-skipsdist = True
+minversion = 2.0
+envlist = linters,docs,py27,py37,molecule
+skipdist = True
 
 [testenv]
 usedevelop = True
-install_command = pip install -c{env:UPPER_CONSTRAINTS_FILE:https://opendev.org/openstack/requirements/raw/branch/master/upper-constraints.txt} {opts} {packages} "ansible>=2" "pre-commit>=1.10"
-setenv = VIRTUAL_ENV={envdir}
-deps = -r{toxinidir}/test-requirements.txt
+install_command = pip install -c{env:UPPER_CONSTRAINTS_FILE:https://opendev.org/openstack/requirements/raw/branch/master/upper-constraints.txt} {opts} {packages}
+passenv = *
+setenv =
+  VIRTUAL_ENV={envdir}
+  ANSIBLE_CALLBACK_PLUGINS={toxinidir}/callback_plugins
+  ANSIBLE_LOOKUP_PLUGINS={toxinidir}/lookup_plugins
+  ANSIBLE_LIBRARY={toxinidir}/library
+  ANSIBLE_ROLES_PATH={toxinidir}/roles
+  ANSIBLE_NOCOWS=1
+  ANSIBLE_RETRY_FILES_ENABLED=0
+  ANSIBLE_STDOUT_CALLBACK=debug
+  ANSIBLE_LOG_PATH={envlogdir}/ansible-execution.log
+  VIRTUAL_ENV={envdir}
+  # pip: Avoid 2020-01-01 warnings: https://github.com/pypa/pip/issues/6207
+  # paramiko CryptographyDeprecationWarning: https://github.com/ansible/ansible/issues/52598
+  PYTHONWARNINGS=ignore:DEPRECATION::pip._internal.cli.base_command,ignore::UserWarning
+  PIP_DISABLE_PIP_VERSION_CHECK=1
 commands = python setup.py test --slowest --testr-args='{posargs}'
+sitepackages = True
+deps =
+    -r {toxinidir}/requirements.txt
+    -r {toxinidir}/test-requirements.txt
+    -r {toxinidir}/molecule-requirements.txt
 whitelist_externals = bash
 
-[testenv:releasenotes]
-basepython = python3
-whitelist_externals = bash
-commands = bash -c tools/releasenotes_tox.sh
-
-[testenv:pep8]
-basepython = python3
-commands =
-    {[testenv:ansible-lint]commands}
-    flake8 {posargs}
-    python ./tools/validate-files.py .
-
-[testenv:venv]
-basepython = python3
-commands = {posargs}
-
-[testenv:cover]
-basepython = python3
-commands = python setup.py test --coverage --testr-args='{posargs}'
-
-[testenv:docs]
-basepython = python3
-commands = python setup.py build_sphinx
+[testenv:bindep]
+# Do not install any requirements. We want this to be fast and work even if
+# system dependencies are missing, since it's used to tell you what system
+# dependencies are missing! This also means that bindep must be installed
+# separately, outside of the requirements files.
+deps = bindep
+commands = bindep test
 
 [testenv:debug]
 basepython = python3
 commands = oslo_debug_helper {posargs}
 
-[testenv:ansible-lint]
-basepython = python3
+[testenv:pep8]
+envdir = {toxworkdir}/linters
 commands =
+    python -m pre_commit run flake8 -a
+
+[testenv:ansible-lint]
+envdir = {toxworkdir}/linters
+deps =
+  {[testenv:linters]deps}
+commands =
+  bash -c "ANSIBLE_ROLES_PATH='{toxinidir}/roles'"
+  bash -c "ANSIBLE_LIBRARY='{toxinidir}/library'"
   python -m pre_commit run ansible-lint -a
 
-[flake8]
-# E123, E125 skipped as they are invalid PEP-8.
+[testenv:yamllint]
+envdir = {toxworkdir}/linters
+deps = {[testenv:linters]deps}
+commands =
+    python -m pre_commit run yamllint -a
 
-show-source = True
-ignore = E123,E125
-builtins = _
-exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build
+[testenv:bashate]
+envdir = {toxworkdir}/linters
+deps = {[testenv:linters]deps}
+commands =
+    python -m pre_commit run bashate -a
 
-[testenv:lower-constraints]
+[testenv:whitespace]
+envdir = {toxworkdir}/linters
+deps = {[testenv:linters]deps}
+commands =
+    python -m pre_commit run trailing-whitespace -a
+
+[testenv:shebangs]
+envdir = {toxworkdir}/linters
+deps = {[testenv:linters]deps}
+commands =
+    python -m pre_commit run check-executables-have-shebangs -a
+
+[testenv:linters]
+deps =
+    -r {toxinidir}/requirements.txt
+    -r {toxinidir}/test-requirements.txt
+    -r {toxinidir}/molecule-requirements.txt
+commands =
+    {[testenv:pep8]commands}
+    {[testenv:ansible-lint]commands}
+    {[testenv:bashate]commands}
+    {[testenv:whitespace]commands}
+    {[testenv:shebangs]commands}
+#    {[testenv:yamllint]commands}
+
+[testenv:releasenotes]
+basepython = python3
+deps = -r{toxinidir}/doc/requirements.txt
+commands =
+    sphinx-build -a -E -W -d releasenotes/build/doctrees -b html releasenotes/source releasenotes/build/html
+
+[testenv:cover]
 basepython = python3
 deps =
-  -c{toxinidir}/lower-constraints.txt
-  -r{toxinidir}/test-requirements.txt
-  -r{toxinidir}/requirements.txt
+    -r {toxinidir}/requirements.txt
+    -r {toxinidir}/test-requirements.txt
+    -r {toxinidir}/molecule-requirements.txt
+commands = python setup.py test --coverage --testr-args='{posargs}'
+
+[testenv:docs]
+basepython = python3
+deps =
+  -r {toxinidir}/doc/requirements.txt
+  -r {toxinidir}/molecule-requirements.txt
+commands=
+    sphinx-build -a -E -W -d doc/build/doctrees -b html doc/source doc/build/html -T
+    doc8 doc
+
+[doc8]
+# Settings for doc8:
+extensions = .rst
+ignore = D001
+
+[testenv:venv]
+commands = {posargs}
 
 [testenv:molecule]
 deps =
@@ -73,3 +138,11 @@ deps =
     selinux
 commands =
     python -m pytest --color=yes --html={envlogdir}/reports.html --self-contained-html {tty:-s} {posargs:roles}
+
+[testenv:lower-constraints]
+basepython = python3
+deps =
+  -c{toxinidir}/lower-constraints.txt
+  -r{toxinidir}/test-requirements.txt
+  -r{toxinidir}/requirements.txt
+  -r{toxinidir}/molecule-requirements.txt

zuul.d/layout.yaml

@@ -3,7 +3,6 @@
       - tripleo-multinode-container-minimal
       - openstack-python-jobs
       - openstack-python3-train-jobs
-      - openstack-tox-molecule
       - check-requirements
       - publish-openstack-docs-pti
       - release-notes-jobs-python3