Merge pull request #1 from dgoodwin/pep8

Pylint fixes and ignores for incoming oo-install code.

9 files changed, 81 insertions, 48 deletions

diff --git a/utils/setup.py b/utils/setup.py
index bc1940636..6e2fdd9c0 100644
--- a/utils/setup.py
+++ b/utils/setup.py
@@ -4,11 +4,6 @@
 
 # Always prefer setuptools over distutils
 from setuptools import setup
-# To use a consistent encoding
-from codecs import open
-from os import path
-
-here = path.abspath(path.dirname(__file__))
 
 setup(
     name='ooinstall',
diff --git a/utils/src/ooinstall/__init__.py b/utils/src/ooinstall/__init__.py
index acaac8d85..944dea3b5 100644
--- a/utils/src/ooinstall/__init__.py
+++ b/utils/src/ooinstall/__init__.py
@@ -1 +1,5 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=missing-docstring
+
 from .oo_config import OOConfig
diff --git a/utils/src/ooinstall/ansible_plugins/facts_callback.py b/utils/src/ooinstall/ansible_plugins/facts_callback.py
index b19e6848e..ea6ed6574 100644
--- a/utils/src/ooinstall/ansible_plugins/facts_callback.py
+++ b/utils/src/ooinstall/ansible_plugins/facts_callback.py
@@ -1,20 +1,25 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name,no-value-for-parameter
+
 import os
 import yaml
-from tempfile import mkstemp
 
 class CallbackModule(object):
-    """
-    """
 
     def __init__(self):
         ######################
         # This is ugly stoopid. This should be updated in the following ways:
-        # 1) it should probably only be used for the openshift_facts.yml playbook, so maybe there's some way to check a variable that's set when that playbook is run?
+        # 1) it should probably only be used for the
+        # openshift_facts.yml playbook, so maybe there's some way to check
+        # a variable that's set when that playbook is run?
         try:
             self.hosts_yaml_name = os.environ['OO_INSTALL_CALLBACK_FACTS_YAML']
         except KeyError:
-            raise ValueError('The OO_INSTALL_CALLBACK_FACTS_YAML environment variable must be set.')
-        self.hosts_yaml = os.open(self.hosts_yaml_name, os.O_CREAT | os.O_WRONLY)
+            raise ValueError('The OO_INSTALL_CALLBACK_FACTS_YAML environment '
+                'variable must be set.')
+        self.hosts_yaml = os.open(self.hosts_yaml_name, os.O_CREAT |
+            os.O_WRONLY)
 
     def on_any(self, *args, **kwargs):
         pass
@@ -62,7 +67,9 @@ class CallbackModule(object):
     def playbook_on_task_start(self, name, is_conditional):
         pass
 
-    def playbook_on_vars_prompt(self, varname, private=True, prompt=None, encrypt=None, confirm=False, salt_size=None, salt=None, default=None):
+    #pylint: disable=too-many-arguments
+    def playbook_on_vars_prompt(self, varname, private=True, prompt=None,
+        encrypt=None, confirm=False, salt_size=None, salt=None, default=None):
         pass
 
     def playbook_on_setup(self):
diff --git a/utils/src/ooinstall/cli_installer.py b/utils/src/ooinstall/cli_installer.py
index a23dc0d53..c2ae00bd1 100644
--- a/utils/src/ooinstall/cli_installer.py
+++ b/utils/src/ooinstall/cli_installer.py
@@ -1,3 +1,7 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name,no-value-for-parameter
+
 import click
 import os
 import re
@@ -5,11 +9,11 @@ import sys
 from ooinstall import install_transactions
 from ooinstall import OOConfig
 from ooinstall.oo_config import Host
-from variants import find_variant, get_variant_version_combos
+from ooinstall.variants import find_variant, get_variant_version_combos
 
 DEFAULT_ANSIBLE_CONFIG = '/usr/share/atomic-openshift-util/ansible.cfg'
 
-def validate_ansible_dir(ctx, param, path):
+def validate_ansible_dir(path):
     if not path:
         raise click.BadParameter('An ansible path must be provided')
     return path
@@ -20,19 +24,10 @@ def is_valid_hostname(hostname):
     if not hostname or len(hostname) > 255:
         return False
     if hostname[-1] == ".":
-        hostname = hostname[:-1] # strip exactly one dot from the right, if present
-    allowed = re.compile("(?!-)[A-Z\d-]{1,63}(?<!-)$", re.IGNORECASE)
+        hostname = hostname[:-1]  # strip exactly one dot from the right, if present
+    allowed = re.compile(r"(?!-)[A-Z\d-]{1,63}(?<!-)$", re.IGNORECASE)
     return all(allowed.match(x) for x in hostname.split("."))
 
-def validate_hostname(ctx, param, hosts):
-    # if '' == hostname or is_valid_hostname(hostname):
-    for hostname in hosts:
-        if not is_valid_hostname(hostname):
-            raise click.BadParameter('"{}" appears to be an invalid hostname. ' \
-                                     'Please double-check this value ' \
-                                     'and re-enter it.'.format(hostname))
-    return hosts
-
 def validate_prompt_hostname(hostname):
     if '' == hostname or is_valid_hostname(hostname):
         return hostname
@@ -402,6 +397,8 @@ def get_hosts_to_run_on(oo_cfg, callback_facts, unattended, force):
               default="/tmp/ansible.log")
 @click.option('--unattended', '-u', is_flag=True, default=False)
 @click.option('--force', '-f', is_flag=True, default=False)
+#pylint: disable=too-many-arguments
+# Main CLI entrypoint, not much we can do about too many arguments.
 def main(configuration, ansible_playbook_directory, ansible_config, ansible_log_path, unattended, force):
     oo_cfg = OOConfig(configuration)
 
@@ -414,7 +411,7 @@ def main(configuration, ansible_playbook_directory, ansible_config, ansible_log_
         # If we're installed by RPM this file should exist and we can use it as our default:
         oo_cfg.settings['ansible_config'] = DEFAULT_ANSIBLE_CONFIG
 
-    validate_ansible_dir(None, None, ansible_playbook_directory)
+    validate_ansible_dir(ansible_playbook_directory)
     oo_cfg.settings['ansible_playbook_directory'] = ansible_playbook_directory
     oo_cfg.ansible_playbook_directory = ansible_playbook_directory
 
@@ -444,7 +441,7 @@ def main(configuration, ansible_playbook_directory, ansible_config, ansible_log_
     # to confirm the settings for new nodes. Look into this once we're distinguishing
     # between new and pre-existing nodes.
     if len(oo_cfg.calc_missing_facts()) > 0:
-        validated_hosts = confirm_hosts_facts(oo_cfg, callback_facts)
+        confirm_hosts_facts(oo_cfg, callback_facts)
 
     oo_cfg.save_to_disk()
 
diff --git a/utils/src/ooinstall/install_transactions.py b/utils/src/ooinstall/install_transactions.py
index ac69bb4b8..cef6662d7 100644
--- a/utils/src/ooinstall/install_transactions.py
+++ b/utils/src/ooinstall/install_transactions.py
@@ -1,7 +1,11 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name,global-statement,global-variable-not-assigned
+
 import subprocess
 import os
 import yaml
-from variants import find_variant
+from ooinstall.variants import find_variant
 
 CFG = None
 
@@ -21,14 +25,19 @@ def generate_inventory(hosts):
         base_inventory.write('ansible_sudo=true\n')
 
     # Find the correct deployment type for ansible:
-    variant, ver = find_variant(CFG.settings['variant'],
-        version=CFG.settings.get('variant_version', None))
+    ver = find_variant(CFG.settings['variant'],
+        version=CFG.settings.get('variant_version', None))[1]
     base_inventory.write('deployment_type={}\n'.format(ver.ansible_key))
 
     if 'OO_INSTALL_DEVEL_REGISTRY' in os.environ:
-        base_inventory.write('oreg_url=rcm-img-docker01.build.eng.bos.redhat.com:5001/openshift3/ose-${component}:${version}\n')
+        base_inventory.write('oreg_url=rcm-img-docker01.build.eng.bos.redhat.com:'
+            '5001/openshift3/ose-${component}:${version}\n')
     if 'OO_INSTALL_PUDDLE_REPO_ENABLE' in os.environ:
-        base_inventory.write("openshift_additional_repos=[{'id': 'ose-devel', 'name': 'ose-devel', 'baseurl': 'http://buildvm-devops.usersys.redhat.com/puddle/build/AtomicOpenShift/3.1/latest/RH7-RHAOS-3.1/$basearch/os', 'enabled': 1, 'gpgcheck': 0}]\n")
+        base_inventory.write("openshift_additional_repos=[{'id': 'ose-devel', "
+            "'name': 'ose-devel', "
+            "'baseurl': 'http://buildvm-devops.usersys.redhat.com"
+            "/puddle/build/AtomicOpenShift/3.1/latest/RH7-RHAOS-3.1/$basearch/os', "
+            "'enabled': 1, 'gpgcheck': 0}]\n")
     if 'OO_INSTALL_STAGE_REGISTRY' in os.environ:
         base_inventory.write('oreg_url=registry.access.stage.redhat.com/openshift3/ose-${component}:${version}\n')
 
@@ -76,8 +85,8 @@ def load_system_facts(inventory_file, os_facts_path, env_vars):
     status = subprocess.call(['ansible-playbook',
                      '--inventory-file={}'.format(inventory_file),
                      os_facts_path],
-                     env=env_vars)
-                         #                     stdout=FNULL)
+                     env=env_vars,
+                     stdout=FNULL)
     if not status == 0:
         return [], 1
     callback_facts_file = open(CFG.settings['ansible_callback_facts_yaml'], 'r')
diff --git a/utils/src/ooinstall/oo_config.py b/utils/src/ooinstall/oo_config.py
index ea9638fe9..a2f53cf78 100644
--- a/utils/src/ooinstall/oo_config.py
+++ b/utils/src/ooinstall/oo_config.py
@@ -1,3 +1,7 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name,too-many-instance-attributes,too-few-public-methods
+
 import os
 import yaml
 from pkg_resources import resource_filename
@@ -123,7 +127,8 @@ class OOConfig(object):
             self.settings['ansible_plugins_directory'] = resource_filename(__name__, 'ansible_plugins')
 
         if 'ansible_callback_facts_yaml' not in self.settings:
-            self.settings['ansible_callback_facts_yaml'] = '{}/callback_facts.yaml'.format(self.settings['ansible_inventory_directory'])
+            self.settings['ansible_callback_facts_yaml'] = '%s/callback_facts.yaml' % \
+                self.settings['ansible_inventory_directory']
 
         if 'ansible_ssh_user' not in self.settings:
             self.settings['ansible_ssh_user'] = ''
diff --git a/utils/src/ooinstall/variants.py b/utils/src/ooinstall/variants.py
index a5159f857..ed98429fc 100644
--- a/utils/src/ooinstall/variants.py
+++ b/utils/src/ooinstall/variants.py
@@ -1,3 +1,7 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name,too-few-public-methods
+
 """
 Defines the supported variants and versions the installer supports, and metadata
 required to run Ansible correctly.
diff --git a/utils/test/cli_installer_tests.py b/utils/test/cli_installer_tests.py
index 78e3ab3bf..076fe5dc9 100644
--- a/utils/test/cli_installer_tests.py
+++ b/utils/test/cli_installer_tests.py
@@ -1,4 +1,7 @@
-import sys
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name
+
 import copy
 import os
 import ConfigParser
@@ -7,7 +10,7 @@ import yaml
 import ooinstall.cli_installer as cli
 
 from click.testing import CliRunner
-from oo_config_tests import OOInstallFixture
+from test.oo_config_tests import OOInstallFixture
 from mock import patch
 
 
@@ -77,14 +80,14 @@ class OOCliFixture(OOInstallFixture):
 
     def assert_result(self, result, exit_code):
         if result.exception is not None or result.exit_code != exit_code:
-            print("Unexpected result from CLI execution")
-            print("Exit code: %s" % result.exit_code)
-            print("Exception: %s" % result.exception)
+            print "Unexpected result from CLI execution"
+            print "Exit code: %s" % result.exit_code
+            print "Exception: %s" % result.exception
             print result.exc_info
             import traceback
             traceback.print_exception(*result.exc_info)
-            print("Output:\n%s" % result.output)
-            self.assertTrue("Exception during CLI execution", False)
+            print "Output:\n%s" % result.output
+            self.fail("Exception during CLI execution")
 
     def _read_yaml(self, config_file_path):
         f = open(config_file_path, 'r')
@@ -261,6 +264,9 @@ class UnattendedCliTests(OOCliFixture):
         self._ansible_config_test(load_facts_mock, run_ansible_mock,
             config, None, ansible_config)
 
+    #pylint: disable=too-many-arguments
+    # This method allows for drastically simpler tests to write, and the args
+    # are all useful.
     def _ansible_config_test(self, load_facts_mock, run_ansible_mock,
         installer_config, ansible_config_cli=None, expected_result=None):
         """
@@ -287,7 +293,7 @@ class UnattendedCliTests(OOCliFixture):
             self.assertFalse('ANSIBLE_CONFIG' in facts_env_vars)
 
         # Test the env vars for main playbook:
-        playbook, inventory, env_vars = run_ansible_mock.call_args[0]
+        env_vars = run_ansible_mock.call_args[0][2]
         if expected_result:
             self.assertEquals(expected_result, env_vars['ANSIBLE_CONFIG'])
         else:
@@ -302,7 +308,9 @@ class AttendedCliTests(OOCliFixture):
         self.config_file = os.path.join(self.work_dir, 'config.yml')
         self.cli_args.extend(["-c", self.config_file])
 
-    def _build_input(self, ssh_user=None, hosts=None, variant_num=None, add_nodes=None, confirm_facts=None):
+    #pylint: disable=too-many-arguments
+    def _build_input(self, ssh_user=None, hosts=None, variant_num=None,
+        add_nodes=None, confirm_facts=None):
         """
         Builds a CLI input string with newline characters to simulate
         the full run.
diff --git a/utils/test/oo_config_tests.py b/utils/test/oo_config_tests.py
index 8459b7c39..01af33fd9 100644
--- a/utils/test/oo_config_tests.py
+++ b/utils/test/oo_config_tests.py
@@ -1,3 +1,7 @@
+# TODO: Temporarily disabled due to importing old code into openshift-ansible
+# repo. We will work on these over time.
+# pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name
+
 import os
 import unittest
 import tempfile
@@ -64,9 +68,9 @@ class OOInstallFixture(unittest.TestCase):
         up in teardown.
         Returns full path to the file.
         """
-        f = open(path, 'w')
-        f.write(config_str)
-        f.close()
+        cfg_file = open(path, 'w')
+        cfg_file.write(config_str)
+        cfg_file.close()
         return path
 
 
@@ -88,7 +92,7 @@ class OOConfigTests(OOInstallFixture):
 
         self.assertEquals('openshift-enterprise', ooconfig.settings['variant'])
 
-    def test_load_complete_validated_facts(self):
+    def test_load_complete_facts(self):
         cfg_path = self.write_config(os.path.join(self.work_dir,
             'ooinstall.conf'), SAMPLE_CONFIG)
         ooconfig = OOConfig(cfg_path)