clean up and clarify docs/comments

update unit tests

5 files changed, 278 insertions, 44 deletions

diff --git a/roles/lib_openshift/library/oc_user.py b/roles/lib_openshift/library/oc_user.py
index ded594ac6..d64290a2e 100644
--- a/roles/lib_openshift/library/oc_user.py
+++ b/roles/lib_openshift/library/oc_user.py
@@ -33,6 +33,7 @@
 
 from __future__ import print_function
 import atexit
+import copy
 import json
 import os
 import re
@@ -40,7 +41,11 @@ import shutil
 import subprocess
 import tempfile
 # pylint: disable=import-error
-import ruamel.yaml as yaml
+try:
+    import ruamel.yaml as yaml
+except ImportError:
+    import yaml
+
 from ansible.module_utils.basic import AnsibleModule
 
 # -*- -*- -*- End included fragment: lib/import.py -*- -*- -*-
@@ -83,13 +88,13 @@ options:
     aliases: []
   full_name:
     description:
-    - String with the full name/description of th user.
+    - String with the full name/description of the user.
     required: false
     default: None
     aliases: []
   groups:
     description:
-    - List of groups the user should be a member of.
+    - List of groups the user should be a member of. This does not add/update the legacy 'groups' field in the OpenShift user object, but makes user entries into the appropriate OpenShift group object for the given user.
     required: false
     default: []
     aliases: []
@@ -104,16 +109,79 @@ EXAMPLES = '''
     state: present
     username: johndoe
     full_name "John Doe"
+    groups:
+    - dedicated-admins
+  register: user_johndoe
+
+user_johndoe variable will have contents like:
+ok: [ded-int-aws-master-61034] => {
+    "user_johndoe": {
+        "changed": true,
+        "results": {
+            "cmd": "oc -n default get users johndoe -o json",
+            "results": [
+                {
+                    "apiVersion": "v1",
+                    "fullName": "John DOe",
+                    "groups": null,
+                    "identities": null,
+                    "kind": "User",
+                    "metadata": {
+                        "creationTimestamp": "2017-02-28T15:09:21Z",
+                        "name": "johndoe",
+                        "resourceVersion": "848781",
+                        "selfLink": "/oapi/v1/users/johndoe",
+                        "uid": "e23d3300-fdc7-11e6-9e3e-12822d6b7656"
+                    }
+                }
+            ],
+            "returncode": 0
+        },
+        "state": "present"
+    }
+}
+'groups' is empty because this field is the OpenShift user object's 'group' field.
 
 - name: Ensure user does not exist
   oc_user:
     state: absent
     username: johndoe
+
+- name: List user's info
+  oc_user:
+    state: list
+    username: johndoe
+  register: user_johndoe
+
+user_johndoe will have contents similar to:
+ok: [ded-int-aws-master-61034] => {
+    "user_johndoe": {
+        "changed": false,
+        "results": [
+            {
+                "apiVersion": "v1",
+                "fullName": "John Doe",
+                "groups": null,
+                "identities": null,
+                "kind": "User",
+                "metadata": {
+                    "creationTimestamp": "2017-02-28T15:04:44Z",
+                    "name": "johndoe",
+                    "resourceVersion": "848280",
+                    "selfLink": "/oapi/v1/users/johndoe",
+                    "uid": "3d479ad2-fdc7-11e6-9e3e-12822d6b7656"
+                }
+            }
+        ],
+        "state": "list"
+    }
+}
 '''
 
 # -*- -*- -*- End included fragment: doc/user -*- -*- -*-
 
 # -*- -*- -*- Begin included fragment: ../../lib_utils/src/class/yedit.py -*- -*- -*-
+# pylint: disable=undefined-variable,missing-docstring
 # noqa: E301,E302
 
 
@@ -308,11 +376,17 @@ class Yedit(object):
         if self.backup and self.file_exists():
             shutil.copy(self.filename, self.filename + '.orig')
 
-        # pylint: disable=no-member
-        if hasattr(self.yaml_dict, 'fa'):
+        # Try to set format attributes if supported
+        try:
             self.yaml_dict.fa.set_block_style()
+        except AttributeError:
+            pass
 
-        Yedit._write(self.filename, yaml.dump(self.yaml_dict, Dumper=yaml.RoundTripDumper))
+        # Try to use RoundTripDumper if supported.
+        try:
+            Yedit._write(self.filename, yaml.dump(self.yaml_dict, Dumper=yaml.RoundTripDumper))
+        except AttributeError:
+            Yedit._write(self.filename, yaml.safe_dump(self.yaml_dict, default_flow_style=False))
 
         return (True, self.yaml_dict)
 
@@ -352,10 +426,24 @@ class Yedit(object):
         # check if it is yaml
         try:
             if content_type == 'yaml' and contents:
-                self.yaml_dict = yaml.load(contents, yaml.RoundTripLoader)
-                # pylint: disable=no-member
-                if hasattr(self.yaml_dict, 'fa'):
+                # Try to set format attributes if supported
+                try:
+                    self.yaml_dict.fa.set_block_style()
+                except AttributeError:
+                    pass
+
+                # Try to use RoundTripLoader if supported.
+                try:
+                    self.yaml_dict = yaml.safe_load(contents, yaml.RoundTripLoader)
+                except AttributeError:
+                    self.yaml_dict = yaml.safe_load(contents)
+
+                # Try to set format attributes if supported
+                try:
                     self.yaml_dict.fa.set_block_style()
+                except AttributeError:
+                    pass
+
             elif content_type == 'json' and contents:
                 self.yaml_dict = json.loads(contents)
         except yaml.YAMLError as err:
@@ -384,14 +472,16 @@ class Yedit(object):
             return (False, self.yaml_dict)
 
         if isinstance(entry, dict):
-            # pylint: disable=no-member,maybe-no-member
+            # AUDIT:maybe-no-member makes sense due to fuzzy types
+            # pylint: disable=maybe-no-member
             if key_or_item in entry:
                 entry.pop(key_or_item)
                 return (True, self.yaml_dict)
             return (False, self.yaml_dict)
 
         elif isinstance(entry, list):
-            # pylint: disable=no-member,maybe-no-member
+            # AUDIT:maybe-no-member makes sense due to fuzzy types
+            # pylint: disable=maybe-no-member
             ind = None
             try:
                 ind = entry.index(key_or_item)
@@ -459,7 +549,9 @@ class Yedit(object):
         if not isinstance(entry, list):
             return (False, self.yaml_dict)
 
-        # pylint: disable=no-member,maybe-no-member
+        # AUDIT:maybe-no-member makes sense due to loading data from
+        # a serialized format.
+        # pylint: disable=maybe-no-member
         entry.append(value)
         return (True, self.yaml_dict)
 
@@ -472,7 +564,8 @@ class Yedit(object):
             entry = None
 
         if isinstance(entry, dict):
-            # pylint: disable=no-member,maybe-no-member
+            # AUDIT:maybe-no-member makes sense due to fuzzy types
+            # pylint: disable=maybe-no-member
             if not isinstance(value, dict):
                 raise YeditException('Cannot replace key, value entry in ' +
                                      'dict with non-dict type. value=[%s] [%s]' % (value, type(value)))  # noqa: E501
@@ -481,7 +574,8 @@ class Yedit(object):
             return (True, self.yaml_dict)
 
         elif isinstance(entry, list):
-            # pylint: disable=no-member,maybe-no-member
+            # AUDIT:maybe-no-member makes sense due to fuzzy types
+            # pylint: disable=maybe-no-member
             ind = None
             if curr_value:
                 try:
@@ -520,12 +614,20 @@ class Yedit(object):
             return (False, self.yaml_dict)
 
         # deepcopy didn't work
-        tmp_copy = yaml.load(yaml.round_trip_dump(self.yaml_dict,
-                                                  default_flow_style=False),
-                             yaml.RoundTripLoader)
-        # pylint: disable=no-member
-        if hasattr(self.yaml_dict, 'fa'):
+        # Try to use ruamel.yaml and fallback to pyyaml
+        try:
+            tmp_copy = yaml.load(yaml.round_trip_dump(self.yaml_dict,
+                                                      default_flow_style=False),
+                                 yaml.RoundTripLoader)
+        except AttributeError:
+            tmp_copy = copy.deepcopy(self.yaml_dict)
+
+        # set the format attributes if available
+        try:
             tmp_copy.fa.set_block_style()
+        except AttributeError:
+            pass
+
         result = Yedit.add_entry(tmp_copy, path, value, self.separator)
         if not result:
             return (False, self.yaml_dict)
@@ -538,11 +640,20 @@ class Yedit(object):
         ''' create a yaml file '''
         if not self.file_exists():
             # deepcopy didn't work
-            tmp_copy = yaml.load(yaml.round_trip_dump(self.yaml_dict, default_flow_style=False),  # noqa: E501
-                                 yaml.RoundTripLoader)
-            # pylint: disable=no-member
-            if hasattr(self.yaml_dict, 'fa'):
+            # Try to use ruamel.yaml and fallback to pyyaml
+            try:
+                tmp_copy = yaml.load(yaml.round_trip_dump(self.yaml_dict,
+                                                          default_flow_style=False),
+                                     yaml.RoundTripLoader)
+            except AttributeError:
+                tmp_copy = copy.deepcopy(self.yaml_dict)
+
+            # set the format attributes if available
+            try:
                 tmp_copy.fa.set_block_style()
+            except AttributeError:
+                pass
+
             result = Yedit.add_entry(tmp_copy, path, value, self.separator)
             if result:
                 self.yaml_dict = tmp_copy
@@ -698,6 +809,32 @@ class OpenShiftCLIError(Exception):
     pass
 
 
+ADDITIONAL_PATH_LOOKUPS = ['/usr/local/bin', os.path.expanduser('~/bin')]
+
+
+def locate_oc_binary():
+    ''' Find and return oc binary file '''
+    # https://github.com/openshift/openshift-ansible/issues/3410
+    # oc can be in /usr/local/bin in some cases, but that may not
+    # be in $PATH due to ansible/sudo
+    paths = os.environ.get("PATH", os.defpath).split(os.pathsep) + ADDITIONAL_PATH_LOOKUPS
+
+    oc_binary = 'oc'
+
+    # Use shutil.which if it is available, otherwise fallback to a naive path search
+    try:
+        which_result = shutil.which(oc_binary, path=os.pathsep.join(paths))
+        if which_result is not None:
+            oc_binary = which_result
+    except AttributeError:
+        for path in paths:
+            if os.path.exists(os.path.join(path, oc_binary)):
+                oc_binary = os.path.join(path, oc_binary)
+                break
+
+    return oc_binary
+
+
 # pylint: disable=too-few-public-methods
 class OpenShiftCLI(object):
     ''' Class to wrap the command line tools '''
@@ -709,8 +846,9 @@ class OpenShiftCLI(object):
         ''' Constructor for OpenshiftCLI '''
         self.namespace = namespace
         self.verbose = verbose
-        self.kubeconfig = kubeconfig
+        self.kubeconfig = Utils.create_tmpfile_copy(kubeconfig)
         self.all_namespaces = all_namespaces
+        self.oc_binary = locate_oc_binary()
 
     # Pylint allows only 5 arguments to be passed.
     # pylint: disable=too-many-arguments
@@ -907,16 +1045,15 @@ class OpenShiftCLI(object):
 
         stdout, stderr = proc.communicate(input_data)
 
-        return proc.returncode, stdout, stderr
+        return proc.returncode, stdout.decode(), stderr.decode()
 
     # pylint: disable=too-many-arguments,too-many-branches
     def openshift_cmd(self, cmd, oadm=False, output=False, output_type='json', input_data=None):
         '''Base command for oc '''
-        cmds = []
+        cmds = [self.oc_binary]
+
         if oadm:
-            cmds = ['oadm']
-        else:
-            cmds = ['oc']
+            cmds.append('adm')
 
         if self.all_namespaces:
             cmds.extend(['--all-namespaces'])
@@ -932,7 +1069,10 @@ class OpenShiftCLI(object):
         if self.verbose:
             print(' '.join(cmds))
 
-        returncode, stdout, stderr = self._run(cmds, input_data)
+        try:
+            returncode, stdout, stderr = self._run(cmds, input_data)
+        except OSError as ex:
+            returncode, stdout, stderr = 1, '', 'Failed to execute {}: {}'.format(subprocess.list2cmdline(cmds), ex)
 
         rval = {"returncode": returncode,
                 "results": results,
@@ -984,7 +1124,13 @@ class Utils(object):
         tmp = Utils.create_tmpfile(prefix=rname)
 
         if ftype == 'yaml':
-            Utils._write(tmp, yaml.dump(data, Dumper=yaml.RoundTripDumper))
+            # AUDIT:no-member makes sense here due to ruamel.YAML/PyYAML usage
+            # pylint: disable=no-member
+            if hasattr(yaml, 'RoundTripDumper'):
+                Utils._write(tmp, yaml.dump(data, Dumper=yaml.RoundTripDumper))
+            else:
+                Utils._write(tmp, yaml.safe_dump(data, default_flow_style=False))
+
         elif ftype == 'json':
             Utils._write(tmp, json.dumps(data))
         else:
@@ -995,7 +1141,18 @@ class Utils(object):
         return tmp
 
     @staticmethod
-    def create_tmpfile(prefix=None):
+    def create_tmpfile_copy(inc_file):
+        '''create a temporary copy of a file'''
+        tmpfile = Utils.create_tmpfile('lib_openshift-')
+        Utils._write(tmpfile, open(inc_file).read())
+
+        # Cleanup the tmpfile
+        atexit.register(Utils.cleanup, [tmpfile])
+
+        return tmpfile
+
+    @staticmethod
+    def create_tmpfile(prefix='tmp'):
         ''' Generates and returns a temporary file name '''
 
         with tempfile.NamedTemporaryFile(prefix=prefix, delete=False) as tmp:
@@ -1055,7 +1212,12 @@ class Utils(object):
             contents = sfd.read()
 
         if sfile_type == 'yaml':
-            contents = yaml.load(contents, yaml.RoundTripLoader)
+            # AUDIT:no-member makes sense here due to ruamel.YAML/PyYAML usage
+            # pylint: disable=no-member
+            if hasattr(yaml, 'RoundTripLoader'):
+                contents = yaml.load(contents, yaml.RoundTripLoader)
+            else:
+                contents = yaml.safe_load(contents)
         elif sfile_type == 'json':
             contents = json.loads(contents)
 
@@ -1298,14 +1460,14 @@ class OCUser(OpenShiftCLI):
 
     @property
     def user(self):
-        ''' property function service'''
+        ''' property function user'''
         if not self._user:
             self.get()
         return self._user
 
     @user.setter
     def user(self, data):
-        ''' setter function for yedit var '''
+        ''' setter function for user '''
         self._user = data
 
     def exists(self):
diff --git a/roles/lib_openshift/src/class/oc_user.py b/roles/lib_openshift/src/class/oc_user.py
index 17b679289..d9e4eac13 100644
--- a/roles/lib_openshift/src/class/oc_user.py
+++ b/roles/lib_openshift/src/class/oc_user.py
@@ -19,14 +19,14 @@ class OCUser(OpenShiftCLI):
 
     @property
     def user(self):
-        ''' property function service'''
+        ''' property function user'''
         if not self._user:
             self.get()
         return self._user
 
     @user.setter
     def user(self, data):
-        ''' setter function for yedit var '''
+        ''' setter function for user '''
         self._user = data
 
     def exists(self):
diff --git a/roles/lib_openshift/src/doc/user b/roles/lib_openshift/src/doc/user
index cae108415..65ee01eb7 100644
--- a/roles/lib_openshift/src/doc/user
+++ b/roles/lib_openshift/src/doc/user
@@ -37,13 +37,13 @@ options:
     aliases: []
   full_name:
     description:
-    - String with the full name/description of th user.
+    - String with the full name/description of the user.
     required: false
     default: None
     aliases: []
   groups:
     description:
-    - List of groups the user should be a member of.
+    - List of groups the user should be a member of. This does not add/update the legacy 'groups' field in the OpenShift user object, but makes user entries into the appropriate OpenShift group object for the given user.
     required: false
     default: []
     aliases: []
@@ -58,9 +58,71 @@ EXAMPLES = '''
     state: present
     username: johndoe
     full_name "John Doe"
+    groups:
+    - dedicated-admins
+  register: user_johndoe
+
+user_johndoe variable will have contents like:
+ok: [ded-int-aws-master-61034] => {
+    "user_johndoe": {
+        "changed": true,
+        "results": {
+            "cmd": "oc -n default get users johndoe -o json",
+            "results": [
+                {
+                    "apiVersion": "v1",
+                    "fullName": "John DOe",
+                    "groups": null,
+                    "identities": null,
+                    "kind": "User",
+                    "metadata": {
+                        "creationTimestamp": "2017-02-28T15:09:21Z",
+                        "name": "johndoe",
+                        "resourceVersion": "848781",
+                        "selfLink": "/oapi/v1/users/johndoe",
+                        "uid": "e23d3300-fdc7-11e6-9e3e-12822d6b7656"
+                    }
+                }
+            ],
+            "returncode": 0
+        },
+        "state": "present"
+    }
+}
+'groups' is empty because this field is the OpenShift user object's 'group' field.
 
 - name: Ensure user does not exist
   oc_user:
     state: absent
     username: johndoe
+
+- name: List user's info
+  oc_user:
+    state: list
+    username: johndoe
+  register: user_johndoe
+
+user_johndoe will have contents similar to:
+ok: [ded-int-aws-master-61034] => {
+    "user_johndoe": {
+        "changed": false,
+        "results": [
+            {
+                "apiVersion": "v1",
+                "fullName": "John Doe",
+                "groups": null,
+                "identities": null,
+                "kind": "User",
+                "metadata": {
+                    "creationTimestamp": "2017-02-28T15:04:44Z",
+                    "name": "johndoe",
+                    "resourceVersion": "848280",
+                    "selfLink": "/oapi/v1/users/johndoe",
+                    "uid": "3d479ad2-fdc7-11e6-9e3e-12822d6b7656"
+                }
+            }
+        ],
+        "state": "list"
+    }
+}
 '''
diff --git a/roles/lib_openshift/src/test/integration/oc_user.yml b/roles/lib_openshift/src/test/integration/oc_user.yml
index 7d6221e64..ad1f9d188 100755
--- a/roles/lib_openshift/src/test/integration/oc_user.yml
+++ b/roles/lib_openshift/src/test/integration/oc_user.yml
@@ -142,8 +142,8 @@
     register: user_out
   - name: assert test group created
     assert:
-      that: user_out['results'][0]['metadata']['name'] == "integration-test-group" and
-            user_out['results'][0]['users'] is not defined
+      that: user_out['results']['results'][0]['metadata']['name'] == "integration-test-group"
+      msg: "{{ user_out }}"
 
   - name: create user with group membership
     oc_user:
diff --git a/roles/lib_openshift/src/test/unit/oc_user.py b/roles/lib_openshift/src/test/unit/test_oc_user.py
index 920b06d0e..933e96ae2 100755
--- a/roles/lib_openshift/src/test/unit/oc_user.py
+++ b/roles/lib_openshift/src/test/unit/test_oc_user.py
@@ -24,7 +24,7 @@ import mock
 # place class in our python path
 module_path = os.path.join('/'.join(os.path.realpath(__file__).split('/')[:-4]), 'library')  # noqa: E501
 sys.path.insert(0, module_path)
-from oc_user import OCUser  # noqa: E402
+from oc_user import OCUser, locate_oc_binary  # noqa: E402
 
 
 class OCUserTest(unittest.TestCase):
@@ -36,8 +36,9 @@ class OCUserTest(unittest.TestCase):
         ''' setup method will create a file and set to known configuration '''
         pass
 
+    @mock.patch('oc_user.Utils.create_tmpfile_copy')
     @mock.patch('oc_user.OCUser._run')
-    def test_state_list(self, mock_cmd):
+    def test_state_list(self, mock_cmd, mock_tmpfile_copy):
         ''' Testing a user list '''
         params = {'username': 'testuser@email.com',
                   'state': 'list',
@@ -65,13 +66,18 @@ class OCUserTest(unittest.TestCase):
             (0, user, ''),
         ]
 
+        mock_tmpfile_copy.side_effect = [
+            '/tmp/mocked_kubeconfig',
+        ]
+
         results = OCUser.run_ansible(params, False)
 
         self.assertFalse(results['changed'])
         self.assertTrue(results['results'][0]['metadata']['name'] == "testuser@email.com")
 
+    @mock.patch('oc_user.Utils.create_tmpfile_copy')
     @mock.patch('oc_user.OCUser._run')
-    def test_state_present(self, mock_cmd):
+    def test_state_present(self, mock_cmd, mock_tmpfile_copy):
         ''' Testing a user list '''
         params = {'username': 'testuser@email.com',
                   'state': 'present',
@@ -102,6 +108,10 @@ class OCUserTest(unittest.TestCase):
             (0, created_user, ''),  # get
         ]
 
+        mock_tmpfile_copy.side_effect = [
+            '/tmp/mocked_kubeconfig',
+        ]
+
         results = OCUser.run_ansible(params, False)
 
         self.assertTrue(results['changed'])