From 8b8a90372058205496f42abf2a3d0dc04c7eab3f Mon Sep 17 00:00:00 2001
From: Brent Baude <bbaude@redhat.com>
Date: Thu, 14 May 2015 14:29:42 -0500
Subject: This patch adds a cloud-init plugin for helping users register and
 subscribe their RHEL based systems.  As inputs, it can take:

- user and password OR activation key and org | requires on of the
    two pair
- auto-attach: True or False | optional
- service-level: <string>  | optional
- add-pool [list, of, pool, ids] | optional
- enable-repos [list, of, yum, repos, to, enable] | optional
- disable-repos [list, of, yum, repos, to, disable] | optional

You can also pass the following to influence your registration via rhsm.conf:

- rhsm-baseurl | optional
- server-hostname | optional
---
 cloudinit/config/cc_rh_subscription.py | 399 +++++++++++++++++++++++++++++++++
 1 file changed, 399 insertions(+)
 create mode 100644 cloudinit/config/cc_rh_subscription.py

diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
new file mode 100644
index 00000000..b8056dbb
--- /dev/null
+++ b/cloudinit/config/cc_rh_subscription.py
@@ -0,0 +1,399 @@
+# vi: ts=4 expandtab
+#
+#    Copyright (C) Red Hat, Inc.
+#
+#    Author: Brent Baude <bbaude@redhat.com>
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU General Public License version 3, as
+#    published by the Free Software Foundation.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU General Public License for more details.
+#
+#    You should have received a copy of the GNU General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import subprocess
+import itertools
+
+
+def handle(_name, cfg, _cloud, log, _args):
+    sm = SubscriptionManager(cfg)
+    sm.log = log
+
+    if not sm.is_registered:
+        try:
+            verify, verify_msg = sm._verify_keys()
+            if verify is not True:
+                raise SubscriptionError(verify_msg)
+            cont = sm.rhn_register()
+            if not cont:
+                raise SubscriptionError("Registration failed or did not "
+                                        "run completely")
+
+            # Splitting up the registration, auto-attach, and servicelevel
+            # commands because the error codes, messages from subman are not
+            # specific enough.
+
+            # Attempt to change the service level
+            if sm.auto_attach and sm.servicelevel is not None:
+                    if not sm._set_service_level():
+                        raise SubscriptionError("Setting of service-level "
+                                                "failed")
+                    else:
+                        sm.log.info("Completed auto-attach with service level")
+            elif sm.auto_attach:
+                if not sm._set_auto_attach():
+                    raise SubscriptionError("Setting auto-attach failed")
+                else:
+                    sm.log.info("Completed auto-attach")
+
+            if sm.pools is not None:
+                if type(sm.pools) is not list:
+                    raise SubscriptionError("Pools must in the format of a "
+                                            "list.")
+                return_stat = sm.addPool(sm.pools)
+                if not return_stat:
+                    raise SubscriptionError("Unable to attach pools {0}"
+                                            .format(sm.pools))
+            if (sm.enable_repo is not None) or (sm.disable_repo is not None):
+                return_stat = sm.update_repos(sm.enable_repo, sm.disable_repo)
+                if not return_stat:
+                    raise SubscriptionError("Unable to add or remove repos")
+            sm.log.info("rh_subscription plugin completed successfully")
+        except SubscriptionError as e:
+            sm.log.warn(e)
+            sm.log.info("rh_subscription plugin did not complete successfully")
+    else:
+        sm.log.info("System is already registered")
+
+
+class SubscriptionError(Exception):
+    pass
+
+
+class SubscriptionManager(object):
+    def __init__(self, cfg):
+        self.cfg = cfg
+        self.rhel_cfg = self.cfg.get('rh_subscription', {})
+        self.rhsm_baseurl = self.rhel_cfg.get('rhsm-baseurl')
+        self.server_hostname = self.rhel_cfg.get('server-hostname')
+        self.pools = self.rhel_cfg.get('add-pool')
+        self.activation_key = self.rhel_cfg.get('activation-key')
+        self.org = self.rhel_cfg.get('org')
+        self.userid = self.rhel_cfg.get('username')
+        self.password = self.rhel_cfg.get('password')
+        self.auto_attach = self.rhel_cfg.get('auto-attach')
+        self.enable_repo = self.rhel_cfg.get('enable-repo')
+        self.disable_repo = self.rhel_cfg.get('disable-repo')
+        self.servicelevel = self.rhel_cfg.get('service-level')
+        self.subman = ['/bin/subscription-manager']
+        self.valid_rh_keys = ['org', 'activation-key', 'username', 'password',
+                              'disable-repo', 'enable-repo', 'add-pool',
+                              'rhsm-baseurl', 'server-hostname',
+                              'auto-attach', 'service-level']
+        self.is_registered = self._is_registered()
+
+    def _verify_keys(self):
+        '''
+        Checks that the keys in the rh_subscription dict from the user-data
+        are what we expect.
+        '''
+
+        for k in self.rhel_cfg:
+            if k not in self.valid_rh_keys:
+                bad_key = "{0} is not a valid key for rh_subscription. "\
+                          "Valid keys are: "\
+                          "{1}".format(k, ', '.join(self.valid_rh_keys))
+                return False, bad_key
+
+        # Check for bad auto-attach value
+        if (self.auto_attach is not None) and \
+                (str(self.auto_attach).upper() not in ['TRUE', 'FALSE']):
+            not_bool = "The key auto-attach must be a value of "\
+                       "either True or False"
+            return False, not_bool
+
+        if (self.servicelevel is not None) and \
+            ((not self.auto_attach) or
+                (str(self.auto_attach).upper() == "FALSE")):
+
+            no_auto = "The service-level key must be used in conjunction with "\
+                      "the auto-attach key.  Please re-run with auto-attach: "\
+                      "True"
+            return False, no_auto
+        return True, None
+
+    def _is_registered(self):
+        '''
+        Checks if the system is already registered and returns
+        True if so, else False
+        '''
+        cmd = list(itertools.chain(self.subman, ['identity']))
+
+        if subprocess.call(cmd, stdout=open(os.devnull, 'wb'),
+                           stderr=open(os.devnull, 'wb')) == 1:
+            return False
+        else:
+            return True
+
+    def rhn_register(self):
+        '''
+        Registers the system by userid and password or activation key
+        and org.  Returns True when successful False when not.
+        '''
+
+        if (self.activation_key is not None) and (self.org is not None):
+            # register by activation key
+            cmd = list(itertools.chain(self.subman, ['register',
+                                       '--activationkey={0}'.
+                       format(self.activation_key),
+                       '--org={0}'.format(self.org)]))
+
+            # If the baseurl and/or server url are passed in, we register
+            # with them.
+
+            if self.rhsm_baseurl is not None:
+                cmd.append("--baseurl={0}".format(self.rhsm_baseurl))
+
+            if self.server_hostname is not None:
+                cmd.append("--serverurl={0}".format(self.server_hostname))
+
+            return_msg, return_code = self._captureRun(cmd)
+
+            if return_code is not 0:
+                self.log.warn("Registration with {0} and {1} failed.".format(
+                              self.activation_key, self.org))
+                return False
+
+        elif (self.userid is not None) and (self.password is not None):
+            # register by username and password
+            cmd = list(itertools.chain(self.subman, ['register',
+                       '--username={0}'.format(self.userid),
+                       '--password={0}'.format(self.password)]))
+
+            # If the baseurl and/or server url are passed in, we register
+            # with them.
+
+            if self.rhsm_baseurl is not None:
+                cmd.append("--baseurl={0}".format(self.rhsm_baseurl))
+
+            if self.server_hostname is not None:
+                cmd.append("--serverurl={0}".format(self.server_hostname))
+
+            # Attempting to register the system only
+            return_msg, return_code = self._captureRun(cmd)
+
+            if return_code is not 0:
+                # Return message is in a set
+                if return_msg[0] == "":
+                    self.log.warn("Registration failed")
+                    if return_msg[1] is not "":
+                        self.log.warn(return_msg[1])
+                return False
+
+        else:
+            self.log.warn("Unable to register system due to incomplete "
+                          "information.")
+            self.log.warn("Use either activationkey and org *or* userid "
+                          "and password")
+            return False
+
+        reg_id = return_msg[0].split("ID: ")[1].rstrip()
+        self.log.info("Registered successfully with ID {0}".format(reg_id))
+        return True
+
+    def _set_service_level(self):
+        cmd = list(itertools.chain(self.subman,
+                                   ['attach', '--auto', '--servicelevel={0}'
+                                    .format(self.servicelevel)]))
+
+        return_msg, return_code = self._captureRun(cmd)
+
+        if return_code is not 0:
+            self.log.warn("Setting the service level failed with: "
+                          "{0}".format(return_msg[1].strip()))
+            return False
+        else:
+            for line in return_msg[0].split("\n"):
+                if line is not "":
+                    self.log.info(line)
+            return True
+
+    def _set_auto_attach(self):
+        cmd = list(itertools.chain(self.subman, ['attach', '--auto']))
+        return_msg, return_code = self._captureRun(cmd)
+
+        if return_code is not 0:
+            self.log.warn("Auto-attach failed with: "
+                          "{0}]".format(return_msg[1].strip()))
+            return False
+        else:
+            for line in return_msg[0].split("\n"):
+                if line is not "":
+                    self.log.info(line)
+            return True
+
+    def _captureRun(self, cmd):
+        '''
+        Subprocess command that captures and returns the output and
+        return code.
+        '''
+
+        r = subprocess.Popen(cmd, stdout=subprocess.PIPE,
+                             stderr=subprocess.PIPE)
+        return r.communicate(), r.returncode
+
+    def _getPools(self):
+        '''
+        Gets the list pools for the active subscription and returns them
+        in list form.
+        '''
+        available = []
+        consumed = []
+
+        # Get all available pools
+        cmd = list(itertools.chain(self.subman, ['list', '--available',
+                                                 '--pool-only']))
+        results = subprocess.check_output(cmd)
+        available = (results.rstrip()).split("\n")
+
+        # Get all available pools
+        cmd = list(itertools.chain(self.subman, ['list', '--consumed',
+                                                 '--pool-only']))
+        results = subprocess.check_output(cmd)
+        consumed = (results.rstrip()).split("\n")
+        return available, consumed
+
+    def _getRepos(self):
+        '''
+        Obtains the current list of active yum repositories and returns
+        them in list form.
+        '''
+
+        cmd = list(itertools.chain(self.subman, ['repos', '--list-enabled']))
+        result, return_code = self._captureRun(cmd)
+
+        active_repos = []
+        for repo in result[0].split("\n"):
+            if "Repo ID:" in repo:
+                active_repos.append((repo.split(':')[1]).strip())
+
+        cmd = list(itertools.chain(self.subman, ['repos', '--list-disabled']))
+        result, return_code = self._captureRun(cmd)
+
+        inactive_repos = []
+        for repo in result[0].split("\n"):
+            if "Repo ID:" in repo:
+                inactive_repos.append((repo.split(':')[1]).strip())
+
+        return active_repos, inactive_repos
+
+    def addPool(self, pools):
+        '''
+        Takes a list of subscription pools and "attaches" them to the
+        current subscription
+        '''
+
+        # An empty list was passed
+        if len(pools) == 0:
+            self.log.info("No pools to attach")
+            return True
+
+        pool_available, pool_consumed = self._getPools()
+        pool_list = []
+        cmd = list(itertools.chain(self.subman, ['attach']))
+        for pool in pools:
+            if (pool not in pool_consumed) and (pool in pool_available):
+                pool_list.append('--pool={0}'.format(pool))
+            else:
+                self.log.warn("Pool {0} is not available".format(pool))
+        if len(pool_list) > 0:
+            cmd.extend(pool_list)
+            try:
+                self._captureRun(cmd)
+                self.log.info("Attached the following pools to your "
+                              "system: %s" % (", ".join(pool_list))
+                              .replace('--pool=', ''))
+                return True
+            except subprocess.CalledProcessError:
+                self.log.warn("Unable to attach pool {0}".format(pool))
+                return False
+
+    def update_repos(self, erepos, drepos):
+        '''
+        Takes a list of yum repo ids that need to be disabled or enabled; then
+        it verifies if they are already enabled or disabled and finally
+        executes the action to disable or enable
+        '''
+
+        if (erepos is not None) and (type(erepos) is not list):
+            self.log.warn("Repo IDs must in the format of a list.")
+            return False
+
+        if (drepos is not None) and (type(drepos) is not list):
+            self.log.warn("Repo IDs must in the format of a list.")
+            return False
+
+        # Bail if both lists are not populated
+        if (len(erepos) == 0) and (len(drepos) == 0):
+            self.log.info("No repo IDs to enable or disable")
+            return True
+
+        active_repos, inactive_repos = self._getRepos()
+        # Creating a list of repoids to be enabled
+        enable_list = []
+        enable_list_fail = []
+        for repoid in erepos:
+            if (repoid in inactive_repos):
+                enable_list.append("--enable={0}".format(repoid))
+            else:
+                enable_list_fail.append(repoid)
+
+        # Creating a list of repoids to be disabled
+        disable_list = []
+        disable_list_fail = []
+        for repoid in drepos:
+            if repoid in active_repos:
+                disable_list.append("--disable={0}".format(repoid))
+            else:
+                disable_list_fail.append(repoid)
+
+        # Logging any repos that are already enabled or disabled
+        if len(enable_list_fail) > 0:
+            for fail in enable_list_fail:
+                # Check if the repo exists or not
+                if fail in active_repos:
+                    self.log.info("Repo {0} is already enabled".format(fail))
+                else:
+                    self.log.warn("Repo {0} does not appear to "
+                                  "exist".format(fail))
+        if len(disable_list_fail) > 0:
+            for fail in disable_list_fail:
+                self.log.info("Repo {0} not disabled "
+                              "because it is not enabled".format(fail))
+
+        cmd = list(itertools.chain(self.subman, ['repos']))
+        if enable_list > 0:
+            cmd.extend(enable_list)
+        if disable_list > 0:
+            cmd.extend(disable_list)
+
+        try:
+            return_msg, return_code = self._captureRun(cmd)
+
+        except subprocess.CalledProcessError as e:
+            self.log.warn("Unable to alter repos due to {0}".format(e))
+            return False
+
+        if enable_list > 0:
+            self.log.info("Enabled the following repos: %s" %
+                          (", ".join(enable_list)).replace('--enable=', ''))
+        if disable_list > 0:
+            self.log.info("Disabled the following repos: %s" %
+                          (", ".join(disable_list)).replace('--disable=', ''))
+        return True
-- 
cgit v1.2.3


From 7ea6b43ea863c2e0774be3ef562b47fc21dc510d Mon Sep 17 00:00:00 2001
From: Brent Baude <bbaude@redhat.com>
Date: Thu, 14 May 2015 16:12:48 -0500
Subject: Adding an example file for rh_subscription in doc/examples

---
 doc/examples/cloud-config-rh_subscription.txt | 49 +++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 doc/examples/cloud-config-rh_subscription.txt

diff --git a/doc/examples/cloud-config-rh_subscription.txt b/doc/examples/cloud-config-rh_subscription.txt
new file mode 100644
index 00000000..be121338
--- /dev/null
+++ b/doc/examples/cloud-config-rh_subscription.txt
@@ -0,0 +1,49 @@
+#cloud-config
+
+# register your Red Hat Enterprise Linux based operating system
+#
+# this cloud-init plugin is capable of registering by username 
+# and password *or* activation and org.  Following a successfully
+# registration you can:
+#   - auto-attach subscriptions
+#   - set the service level
+#   - add subscriptions based on its pool ID
+#   - enable yum repositories based on its repo id
+#   - disable yum repositories based on its repo id
+#   - alter the rhsm_baseurl and server-hostname in the
+#     /etc/rhsm/rhs.conf file
+
+rh_subscription:
+    username: joe@foo.bar
+
+    ## Quote your password if it has symbols to be safe
+    password: '1234abcd'
+
+    ## If you prefer, you can use the activation key and 
+    ## org instead of username and password. Be sure to
+    ## comment out username and password
+
+    #activation-key: foobar
+    #org: 12345
+
+    ## Uncomment to auto-attach subscriptions to your system 
+    #auto-attach: True
+
+    ## Uncomment to set the service level for your 
+    ##   subscriptions
+    #service-level: self-support
+
+    ## Uncomment to add pools (needs to be a list of IDs)
+    #add-pool: []
+
+    ## Uncomment to add or remove yum repos
+    ##   (needs to be a list of repo IDs)
+    #enable-repo: []
+    #disable-repo: []
+
+    ## Uncomment to alter the baseurl in /etc/rhsm/rhsm.conf
+    #rhsm-baseurl: http://url
+
+    ## Uncomment to alter the server hostname in 
+    ##  /etc/rhsm/rhsm.conf
+    #server-hostname: foo.bar.com
-- 
cgit v1.2.3


From cf2b017c8bf2adb02a2c7a9c9f03754402cb73c4 Mon Sep 17 00:00:00 2001
From: Brent Baude <bbaude@redhat.com>
Date: Thu, 21 May 2015 13:32:30 -0500
Subject: This commit consists of three things based on feedback from smosher:

cc_rh_subscription: Use of self.log.info limited, uses the util.subp for subprocesses, removed full path for subscription-manager

cloud-config-rh_subscription.txt: A heavily commented example file on how to use rh_subscription and its main keys

test_rh_subscription.py: a set of unittests for rh_subscription
---
 cloudinit/config/cc_rh_subscription.py  | 181 +++++++++++++------------
 tests/unittests/test_rh_subscription.py | 231 ++++++++++++++++++++++++++++++++
 2 files changed, 323 insertions(+), 89 deletions(-)
 create mode 100644 tests/unittests/test_rh_subscription.py

diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
index b8056dbb..00a88456 100644
--- a/cloudinit/config/cc_rh_subscription.py
+++ b/cloudinit/config/cc_rh_subscription.py
@@ -16,15 +16,13 @@
 #    You should have received a copy of the GNU General Public License
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import os
-import subprocess
 import itertools
+from cloudinit import util
 
 
 def handle(_name, cfg, _cloud, log, _args):
     sm = SubscriptionManager(cfg)
     sm.log = log
-
     if not sm.is_registered:
         try:
             verify, verify_msg = sm._verify_keys()
@@ -41,21 +39,22 @@ def handle(_name, cfg, _cloud, log, _args):
 
             # Attempt to change the service level
             if sm.auto_attach and sm.servicelevel is not None:
-                    if not sm._set_service_level():
-                        raise SubscriptionError("Setting of service-level "
-                                                "failed")
-                    else:
-                        sm.log.info("Completed auto-attach with service level")
+                if not sm._set_service_level():
+                    raise SubscriptionError("Setting of service-level "
+                                            "failed")
+                else:
+                    sm.log.debug("Completed auto-attach with service level")
             elif sm.auto_attach:
                 if not sm._set_auto_attach():
                     raise SubscriptionError("Setting auto-attach failed")
                 else:
-                    sm.log.info("Completed auto-attach")
+                    sm.log.debug("Completed auto-attach")
 
             if sm.pools is not None:
                 if type(sm.pools) is not list:
-                    raise SubscriptionError("Pools must in the format of a "
-                                            "list.")
+                    pool_fail = "Pools must in the format of a list"
+                    raise SubscriptionError(pool_fail)
+                                            
                 return_stat = sm.addPool(sm.pools)
                 if not return_stat:
                     raise SubscriptionError("Unable to attach pools {0}"
@@ -66,8 +65,8 @@ def handle(_name, cfg, _cloud, log, _args):
                     raise SubscriptionError("Unable to add or remove repos")
             sm.log.info("rh_subscription plugin completed successfully")
         except SubscriptionError as e:
-            sm.log.warn(e)
-            sm.log.info("rh_subscription plugin did not complete successfully")
+            sm.log.warn(str(e))
+            sm.log.warn("rh_subscription plugin did not complete successfully")
     else:
         sm.log.info("System is already registered")
 
@@ -91,7 +90,7 @@ class SubscriptionManager(object):
         self.enable_repo = self.rhel_cfg.get('enable-repo')
         self.disable_repo = self.rhel_cfg.get('disable-repo')
         self.servicelevel = self.rhel_cfg.get('service-level')
-        self.subman = ['/bin/subscription-manager']
+        self.subman = ['subscription-manager']
         self.valid_rh_keys = ['org', 'activation-key', 'username', 'password',
                               'disable-repo', 'enable-repo', 'add-pool',
                               'rhsm-baseurl', 'server-hostname',
@@ -135,11 +134,22 @@ class SubscriptionManager(object):
         '''
         cmd = list(itertools.chain(self.subman, ['identity']))
 
-        if subprocess.call(cmd, stdout=open(os.devnull, 'wb'),
-                           stderr=open(os.devnull, 'wb')) == 1:
+        try:
+            self._sub_man_cli(cmd)
+        except util.ProcessExecutionError:
             return False
-        else:
-            return True
+
+        return True
+
+    def _sub_man_cli(self, cmd, logstring_val=False):
+        '''
+        Uses the prefered cloud-init subprocess def of util.subp
+        and runs subscription-manager.  Breaking this to a
+        separate function for later use in mocking and unittests
+        '''
+        return_out, return_err = util.subp(cmd, logstring=logstring_val)
+
+        return return_out, return_err
 
     def rhn_register(self):
         '''
@@ -163,11 +173,13 @@ class SubscriptionManager(object):
             if self.server_hostname is not None:
                 cmd.append("--serverurl={0}".format(self.server_hostname))
 
-            return_msg, return_code = self._captureRun(cmd)
-
-            if return_code is not 0:
-                self.log.warn("Registration with {0} and {1} failed.".format(
-                              self.activation_key, self.org))
+            try:
+                return_out, return_err = self._sub_man_cli(cmd,
+                                                           logstring_val=True)
+            except util.ProcessExecutionError as e:
+                if e.stdout == "":
+                    self.log.warn("Registration failed due "
+                                  "to: {0}".format(e.stderr))
                 return False
 
         elif (self.userid is not None) and (self.password is not None):
@@ -186,14 +198,13 @@ class SubscriptionManager(object):
                 cmd.append("--serverurl={0}".format(self.server_hostname))
 
             # Attempting to register the system only
-            return_msg, return_code = self._captureRun(cmd)
-
-            if return_code is not 0:
-                # Return message is in a set
-                if return_msg[0] == "":
-                    self.log.warn("Registration failed")
-                    if return_msg[1] is not "":
-                        self.log.warn(return_msg[1])
+            try:
+                return_out, return_err = self._sub_man_cli(cmd,
+                                                           logstring_val=True)
+            except util.ProcessExecutionError as e:
+                if e.stdout == "":
+                    self.log.warn("Registration failed due "
+                                  "to: {0}".format(e.stderr))
                 return False
 
         else:
@@ -203,8 +214,8 @@ class SubscriptionManager(object):
                           "and password")
             return False
 
-        reg_id = return_msg[0].split("ID: ")[1].rstrip()
-        self.log.info("Registered successfully with ID {0}".format(reg_id))
+        reg_id = return_out.split("ID: ")[1].rstrip()
+        self.log.debug("Registered successfully with ID {0}".format(reg_id))
         return True
 
     def _set_service_level(self):
@@ -212,41 +223,34 @@ class SubscriptionManager(object):
                                    ['attach', '--auto', '--servicelevel={0}'
                                     .format(self.servicelevel)]))
 
-        return_msg, return_code = self._captureRun(cmd)
-
-        if return_code is not 0:
-            self.log.warn("Setting the service level failed with: "
-                          "{0}".format(return_msg[1].strip()))
+        try:
+            return_out, return_err = self._sub_man_cli(cmd)
+        except util.ProcessExecutionError as e:
+            if e.stdout.rstrip() != '':
+                for line in e.stdout.split("\n"):
+                    if line is not '':
+                        self.log.warn(line)
+            else:
+                self.log.warn("Setting the service level failed with: "
+                              "{0}".format(e.stderr.strip()))
             return False
-        else:
-            for line in return_msg[0].split("\n"):
-                if line is not "":
-                    self.log.info(line)
-            return True
+        for line in return_out.split("\n"):
+            if line is not "":
+                self.log.debug(line)
+        return True
 
     def _set_auto_attach(self):
         cmd = list(itertools.chain(self.subman, ['attach', '--auto']))
-        return_msg, return_code = self._captureRun(cmd)
-
-        if return_code is not 0:
+        try:
+            return_out, return_err = self._sub_man_cli(cmd)
+        except util.ProcessExecutionError:
             self.log.warn("Auto-attach failed with: "
-                          "{0}]".format(return_msg[1].strip()))
+                          "{0}]".format(return_err.strip()))
             return False
-        else:
-            for line in return_msg[0].split("\n"):
-                if line is not "":
-                    self.log.info(line)
-            return True
-
-    def _captureRun(self, cmd):
-        '''
-        Subprocess command that captures and returns the output and
-        return code.
-        '''
-
-        r = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                             stderr=subprocess.PIPE)
-        return r.communicate(), r.returncode
+        for line in return_out.split("\n"):
+            if line is not "":
+                self.log.debug(line)
+        return True
 
     def _getPools(self):
         '''
@@ -259,14 +263,15 @@ class SubscriptionManager(object):
         # Get all available pools
         cmd = list(itertools.chain(self.subman, ['list', '--available',
                                                  '--pool-only']))
-        results = subprocess.check_output(cmd)
+        results, errors = self._sub_man_cli(cmd)
         available = (results.rstrip()).split("\n")
 
-        # Get all available pools
+        # Get all consumed pools
         cmd = list(itertools.chain(self.subman, ['list', '--consumed',
                                                  '--pool-only']))
-        results = subprocess.check_output(cmd)
+        results, errors = self._sub_man_cli(cmd)
         consumed = (results.rstrip()).split("\n")
+
         return available, consumed
 
     def _getRepos(self):
@@ -276,21 +281,19 @@ class SubscriptionManager(object):
         '''
 
         cmd = list(itertools.chain(self.subman, ['repos', '--list-enabled']))
-        result, return_code = self._captureRun(cmd)
-
+        return_out, return_err = self._sub_man_cli(cmd)
         active_repos = []
-        for repo in result[0].split("\n"):
+        for repo in return_out.split("\n"):
             if "Repo ID:" in repo:
                 active_repos.append((repo.split(':')[1]).strip())
 
         cmd = list(itertools.chain(self.subman, ['repos', '--list-disabled']))
-        result, return_code = self._captureRun(cmd)
+        return_out, return_err = self._sub_man_cli(cmd)
 
         inactive_repos = []
-        for repo in result[0].split("\n"):
+        for repo in return_out.split("\n"):
             if "Repo ID:" in repo:
                 inactive_repos.append((repo.split(':')[1]).strip())
-
         return active_repos, inactive_repos
 
     def addPool(self, pools):
@@ -301,7 +304,7 @@ class SubscriptionManager(object):
 
         # An empty list was passed
         if len(pools) == 0:
-            self.log.info("No pools to attach")
+            self.log.debug("No pools to attach")
             return True
 
         pool_available, pool_consumed = self._getPools()
@@ -315,13 +318,14 @@ class SubscriptionManager(object):
         if len(pool_list) > 0:
             cmd.extend(pool_list)
             try:
-                self._captureRun(cmd)
-                self.log.info("Attached the following pools to your "
-                              "system: %s" % (", ".join(pool_list))
-                              .replace('--pool=', ''))
+                self._sub_man_cli(cmd)
+                self.log.debug("Attached the following pools to your "
+                               "system: %s" % (", ".join(pool_list))
+                               .replace('--pool=', ''))
                 return True
-            except subprocess.CalledProcessError:
-                self.log.warn("Unable to attach pool {0}".format(pool))
+            except util.ProcessExecutionError as e:
+                self.log.warn("Unable to attach pool {0} "
+                              "due to {1}".format(pool, e))
                 return False
 
     def update_repos(self, erepos, drepos):
@@ -341,7 +345,7 @@ class SubscriptionManager(object):
 
         # Bail if both lists are not populated
         if (len(erepos) == 0) and (len(drepos) == 0):
-            self.log.info("No repo IDs to enable or disable")
+            self.log.debug("No repo IDs to enable or disable")
             return True
 
         active_repos, inactive_repos = self._getRepos()
@@ -368,14 +372,14 @@ class SubscriptionManager(object):
             for fail in enable_list_fail:
                 # Check if the repo exists or not
                 if fail in active_repos:
-                    self.log.info("Repo {0} is already enabled".format(fail))
+                    self.log.debug("Repo {0} is already enabled".format(fail))
                 else:
                     self.log.warn("Repo {0} does not appear to "
                                   "exist".format(fail))
         if len(disable_list_fail) > 0:
             for fail in disable_list_fail:
-                self.log.info("Repo {0} not disabled "
-                              "because it is not enabled".format(fail))
+                self.log.debug("Repo {0} not disabled "
+                               "because it is not enabled".format(fail))
 
         cmd = list(itertools.chain(self.subman, ['repos']))
         if enable_list > 0:
@@ -384,16 +388,15 @@ class SubscriptionManager(object):
             cmd.extend(disable_list)
 
         try:
-            return_msg, return_code = self._captureRun(cmd)
-
-        except subprocess.CalledProcessError as e:
+            self._sub_man_cli(cmd)
+        except util.ProcessExecutionError as e:
             self.log.warn("Unable to alter repos due to {0}".format(e))
             return False
 
         if enable_list > 0:
-            self.log.info("Enabled the following repos: %s" %
-                          (", ".join(enable_list)).replace('--enable=', ''))
+            self.log.debug("Enabled the following repos: %s" %
+                           (", ".join(enable_list)).replace('--enable=', ''))
         if disable_list > 0:
-            self.log.info("Disabled the following repos: %s" %
-                          (", ".join(disable_list)).replace('--disable=', ''))
+            self.log.debug("Disabled the following repos: %s" %
+                           (", ".join(disable_list)).replace('--disable=', ''))
         return True
diff --git a/tests/unittests/test_rh_subscription.py b/tests/unittests/test_rh_subscription.py
new file mode 100644
index 00000000..ba9181ec
--- /dev/null
+++ b/tests/unittests/test_rh_subscription.py
@@ -0,0 +1,231 @@
+from cloudinit import util
+from cloudinit.config import cc_rh_subscription
+import logging
+import mock
+import unittest
+
+
+class GoodTests(unittest.TestCase):
+    def setUp(self):
+        super(GoodTests, self).setUp()
+        self.name = "cc_rh_subscription"
+        self.cloud_init = None
+        self.log = logging.getLogger("good_tests")
+        self.args = []
+        self.handle = cc_rh_subscription.handle
+        self.SM = cc_rh_subscription.SubscriptionManager
+
+        self.config = {'rh_subscription':
+                       {'username': 'scooby@do.com',
+                        'password': 'scooby-snacks'
+                        }}
+        self.config_full = {'rh_subscription':
+                            {'username': 'scooby@do.com',
+                             'password': 'scooby-snacks',
+                             'auto-attach': True,
+                             'service-level': 'self-support',
+                             'add-pool': ['pool1', 'pool2', 'pool3'],
+                             'enable-repo': ['repo1', 'repo2', 'repo3'],
+                             'disable-repo': ['repo4', 'repo5']
+                             }}
+
+    def test_already_registered(self):
+        '''
+        Emulates a system that is already registered. Ensure it gets
+        a non-ProcessExecution error from is_registered()
+        '''
+        good_message = 'System is already registered'
+        with mock.patch.object(cc_rh_subscription.SubscriptionManager,
+                               '_sub_man_cli') as mockobj:
+            self.log.info = mock.MagicMock(wraps=self.log.info)
+            self.handle(self.name, self.config, self.cloud_init,
+                        self.log, self.args)
+            self.assertEqual(mockobj.call_count, 1)
+            self.log.info.assert_called_with(good_message)
+
+    def test_simple_registration(self):
+        '''
+        Simple registration with username and password
+        '''
+        good_message = 'rh_subscription plugin completed successfully'
+        self.log.info = mock.MagicMock(wraps=self.log.info)
+        reg = "The system has been registered with ID:" \
+              " 12345678-abde-abcde-1234-1234567890abc"
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError, (reg, 'bar')])
+        self.handle(self.name, self.config, self.cloud_init,
+                    self.log, self.args)
+        self.SM._sub_man_cli.assert_called_with_once(['subscription-manager',
+                                                      'identity'])
+        self.SM._sub_man_cli.assert_called_with_once(
+            ['subscription-manager', 'register', '--username=scooby@do.com',
+             '--password=scooby-snacks'], logstring_val=True)
+
+        self.log.info.assert_called_with(good_message)
+        self.assertEqual(self.SM._sub_man_cli.call_count, 2)
+
+    def test_full_registration(self):
+        '''
+        Registration with auto-attach, service-level, adding pools,
+        and enabling and disabling yum repos
+        '''
+        pool_message = 'Pool pool2 is not available'
+        repo_message1 = 'Repo repo1 is already enabled'
+        repo_message2 = 'Enabled the following repos: repo2, repo3'
+        good_message = 'rh_subscription plugin completed successfully'
+        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.log.debug = mock.MagicMock(wraps=self.log.debug)
+        reg = "The system has been registered with ID:" \
+              " 12345678-abde-abcde-1234-1234567890abc"
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError, (reg, 'bar'),
+                         ('Service level set to: self-support', ''),
+                         ('pool1\npool3\n', ''), ('pool2\n', ''), ('', ''),
+                         ('Repo ID: repo1\nRepo ID: repo5\n', ''),
+                         ('Repo ID: repo2\nRepo ID: repo3\nRepo ID: '
+                          'repo4', ''),
+                         ('', '')])
+        self.handle(self.name, self.config_full, self.cloud_init,
+                    self.log, self.args)
+        self.log.warn.assert_any_call(pool_message)
+        self.log.debug.assert_any_call(repo_message1)
+        self.log.debug.assert_any_call(repo_message2)
+        self.log.info.assert_any_call(good_message)
+        self.SM._sub_man_cli.assert_called_with_once(['subscription-manager',
+                                                      'attach', '-pool=pool1',
+                                                      '--pool=pool33'])
+        self.assertEqual(self.SM._sub_man_cli.call_count, 9)
+
+
+class BadTests(unittest.TestCase):
+    def setUp(self):
+        super(BadTests, self).setUp()
+        self.name = "cc_rh_subscription"
+        self.cloud_init = None
+        self.log = logging.getLogger("bad_tests")
+        self.orig = self.log
+        self.args = []
+        self.handle = cc_rh_subscription.handle
+        self.SM = cc_rh_subscription.SubscriptionManager
+        self.reg = "The system has been registered with ID:" \
+                   " 12345678-abde-abcde-1234-1234567890abc"
+
+        self.config_no_password = {'rh_subscription':
+                                   {'username': 'scooby@do.com'
+                                    }}
+
+        self.config_no_key = {'rh_subscription':
+                              {'activation-key': '1234abcde',
+                               }}
+
+        self.config_service = {'rh_subscription':
+                               {'username': 'scooby@do.com',
+                                'password': 'scooby-snacks',
+                                'service-level': 'self-support'
+                                }}
+
+        self.config_badpool = {'rh_subscription':
+                               {'username': 'scooby@do.com',
+                                'password': 'scooby-snacks',
+                                'add-pool': 'not_a_list'
+                                }}
+        self.config_badrepo = {'rh_subscription':
+                               {'username': 'scooby@do.com',
+                                'password': 'scooby-snacks',
+                                'enable-repo': 'not_a_list'
+                                }}
+        self.config_badkey = {'rh_subscription':
+                              {'activation_key': 'abcdef1234',
+                               'org': '123',
+                               }}
+
+    def test_no_password(self):
+        '''
+        Attempt to register without the password key/value
+        '''
+        self.missing_info(self.config_no_password)
+
+    def test_no_org(self):
+        '''
+        Attempt to register without the org key/value
+        '''
+        self.missing_info(self.config_no_key)
+
+    def test_service_level_without_auto(self):
+        '''
+        Attempt to register using service-level without the auto-attach key
+        '''
+        good_message = 'The service-level key must be used in conjunction'\
+                       ' with the auto-attach key.  Please re-run with '\
+                       'auto-attach: True'
+
+        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+        self.handle(self.name, self.config_service, self.cloud_init,
+                    self.log, self.args)
+        self.log.warn.assert_any_call(good_message)
+        self.assertRaises(cc_rh_subscription.SubscriptionError)
+        self.assertEqual(self.SM._sub_man_cli.call_count, 1)
+
+    def test_pool_not_a_list(self):
+        '''
+        Register with pools that are not in the format of a list
+        '''
+        good_message = "Pools must in the format of a list"
+        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+        self.handle(self.name, self.config_badpool, self.cloud_init,
+                    self.log, self.args)
+        self.log.warn.assert_any_call(good_message)
+        self.assertRaises(cc_rh_subscription.SubscriptionError)
+        self.assertEqual(self.SM._sub_man_cli.call_count, 2)
+
+    def test_repo_not_a_list(self):
+        '''
+        Register with repos that are not in the format of a list
+        '''
+        good_message = "Repo IDs must in the format of a list."
+        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+        self.handle(self.name, self.config_badrepo, self.cloud_init,
+                    self.log, self.args)
+        self.log.warn.assert_any_call(good_message)
+        self.assertRaises(cc_rh_subscription.SubscriptionError)
+        self.assertEqual(self.SM._sub_man_cli.call_count, 2)
+
+    def test_bad_key_value(self):
+        '''
+        Attempt to register with a key that we don't know
+        '''
+        good_message = 'activation_key is not a valid key for rh_subscription.'\
+                       ' Valid keys are: org, activation-key, username, '\
+                       'password, disable-repo, enable-repo, add-pool,'\
+                       ' rhsm-baseurl, server-hostname, auto-attach, '\
+                       'service-level'
+        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+        self.handle(self.name, self.config_badkey, self.cloud_init,
+                    self.log, self.args)
+        self.assertRaises(cc_rh_subscription.SubscriptionError)
+        self.log.warn.assert_any_call(good_message)
+        self.assertEqual(self.SM._sub_man_cli.call_count, 1)
+
+    def missing_info(self, config):
+        '''
+        Helper def for tests that having missing information
+        '''
+        good_message = "Unable to register system due to incomplete "\
+                       "information."
+        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM._sub_man_cli = mock.MagicMock(
+            side_effect=[util.ProcessExecutionError])
+        self.handle(self.name, config, self.cloud_init,
+                    self.log, self.args)
+        self.SM._sub_man_cli.assert_called_with(['subscription-manager',
+                                                 'identity'])
+        self.log.warn.assert_any_call(good_message)
+        self.assertEqual(self.SM._sub_man_cli.call_count, 1)
-- 
cgit v1.2.3


From d9470a429935d4a5e12a5a3d1f57867362f92c57 Mon Sep 17 00:00:00 2001
From: Brent Baude <bbaude@redhat.com>
Date: Wed, 27 May 2015 13:01:35 -0500
Subject: Updated files with upstream review comments thanks to Dan and Scott

---
 cloudinit/config/cc_rh_subscription.py  | 108 ++++++++++----------
 tests/unittests/test_rh_subscription.py | 169 ++++++++++++++------------------
 2 files changed, 128 insertions(+), 149 deletions(-)

diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
index 00a88456..db3d5525 100644
--- a/cloudinit/config/cc_rh_subscription.py
+++ b/cloudinit/config/cc_rh_subscription.py
@@ -1,6 +1,6 @@
 # vi: ts=4 expandtab
 #
-#    Copyright (C) Red Hat, Inc.
+#    Copyright (C) 2015 Red Hat, Inc.
 #
 #    Author: Brent Baude <bbaude@redhat.com>
 #
@@ -16,7 +16,6 @@
 #    You should have received a copy of the GNU General Public License
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import itertools
 from cloudinit import util
 
 
@@ -51,10 +50,10 @@ def handle(_name, cfg, _cloud, log, _args):
                     sm.log.debug("Completed auto-attach")
 
             if sm.pools is not None:
-                if type(sm.pools) is not list:
+                if not isinstance(sm.pools, (list)):
                     pool_fail = "Pools must in the format of a list"
                     raise SubscriptionError(pool_fail)
-                                            
+
                 return_stat = sm.addPool(sm.pools)
                 if not return_stat:
                     raise SubscriptionError("Unable to attach pools {0}"
@@ -63,12 +62,12 @@ def handle(_name, cfg, _cloud, log, _args):
                 return_stat = sm.update_repos(sm.enable_repo, sm.disable_repo)
                 if not return_stat:
                     raise SubscriptionError("Unable to add or remove repos")
-            sm.log.info("rh_subscription plugin completed successfully")
+            sm.log_sucess("rh_subscription plugin completed successfully")
         except SubscriptionError as e:
-            sm.log.warn(str(e))
-            sm.log.warn("rh_subscription plugin did not complete successfully")
+            sm.log_warn(str(e))
+            sm.log_warn("rh_subscription plugin did not complete successfully")
     else:
-        sm.log.info("System is already registered")
+        sm.log_sucess("System is already registered")
 
 
 class SubscriptionError(Exception):
@@ -76,6 +75,11 @@ class SubscriptionError(Exception):
 
 
 class SubscriptionManager(object):
+    valid_rh_keys = ['org', 'activation-key', 'username', 'password',
+                     'disable-repo', 'enable-repo', 'add-pool',
+                     'rhsm-baseurl', 'server-hostname',
+                     'auto-attach', 'service-level']
+
     def __init__(self, cfg):
         self.cfg = cfg
         self.rhel_cfg = self.cfg.get('rh_subscription', {})
@@ -91,12 +95,16 @@ class SubscriptionManager(object):
         self.disable_repo = self.rhel_cfg.get('disable-repo')
         self.servicelevel = self.rhel_cfg.get('service-level')
         self.subman = ['subscription-manager']
-        self.valid_rh_keys = ['org', 'activation-key', 'username', 'password',
-                              'disable-repo', 'enable-repo', 'add-pool',
-                              'rhsm-baseurl', 'server-hostname',
-                              'auto-attach', 'service-level']
         self.is_registered = self._is_registered()
 
+    def log_sucess(self, msg):
+        '''Simple wrapper for logging info messages. Useful for unittests'''
+        self.log.info(msg)
+
+    def log_warn(self, msg):
+        '''Simple wrapper for logging warning messages. Useful for unittests'''
+        self.log.warn(msg)
+
     def _verify_keys(self):
         '''
         Checks that the keys in the rh_subscription dict from the user-data
@@ -112,14 +120,15 @@ class SubscriptionManager(object):
 
         # Check for bad auto-attach value
         if (self.auto_attach is not None) and \
-                (str(self.auto_attach).upper() not in ['TRUE', 'FALSE']):
+                not (util.is_true(self.auto_attach) or
+                     util.is_false(self.auto_attach)):
             not_bool = "The key auto-attach must be a value of "\
                        "either True or False"
             return False, not_bool
 
         if (self.servicelevel is not None) and \
-            ((not self.auto_attach) or
-                (str(self.auto_attach).upper() == "FALSE")):
+                ((not self.auto_attach)
+                 or (util.is_false(str(self.auto_attach)))):
 
             no_auto = "The service-level key must be used in conjunction with "\
                       "the auto-attach key.  Please re-run with auto-attach: "\
@@ -132,7 +141,7 @@ class SubscriptionManager(object):
         Checks if the system is already registered and returns
         True if so, else False
         '''
-        cmd = list(itertools.chain(self.subman, ['identity']))
+        cmd = ['identity']
 
         try:
             self._sub_man_cli(cmd)
@@ -147,9 +156,8 @@ class SubscriptionManager(object):
         and runs subscription-manager.  Breaking this to a
         separate function for later use in mocking and unittests
         '''
-        return_out, return_err = util.subp(cmd, logstring=logstring_val)
-
-        return return_out, return_err
+        cmd = self.subman + cmd
+        return util.subp(cmd, logstring=logstring_val)
 
     def rhn_register(self):
         '''
@@ -159,10 +167,8 @@ class SubscriptionManager(object):
 
         if (self.activation_key is not None) and (self.org is not None):
             # register by activation key
-            cmd = list(itertools.chain(self.subman, ['register',
-                                       '--activationkey={0}'.
-                       format(self.activation_key),
-                       '--org={0}'.format(self.org)]))
+            cmd = ['register', '--activationkey={0}'.
+                   format(self.activation_key), '--org={0}'.format(self.org)]
 
             # If the baseurl and/or server url are passed in, we register
             # with them.
@@ -178,15 +184,14 @@ class SubscriptionManager(object):
                                                            logstring_val=True)
             except util.ProcessExecutionError as e:
                 if e.stdout == "":
-                    self.log.warn("Registration failed due "
+                    self.log_warn("Registration failed due "
                                   "to: {0}".format(e.stderr))
                 return False
 
         elif (self.userid is not None) and (self.password is not None):
             # register by username and password
-            cmd = list(itertools.chain(self.subman, ['register',
-                       '--username={0}'.format(self.userid),
-                       '--password={0}'.format(self.password)]))
+            cmd = ['register', '--username={0}'.format(self.userid),
+                   '--password={0}'.format(self.password)]
 
             # If the baseurl and/or server url are passed in, we register
             # with them.
@@ -203,14 +208,14 @@ class SubscriptionManager(object):
                                                            logstring_val=True)
             except util.ProcessExecutionError as e:
                 if e.stdout == "":
-                    self.log.warn("Registration failed due "
+                    self.log_warn("Registration failed due "
                                   "to: {0}".format(e.stderr))
                 return False
 
         else:
-            self.log.warn("Unable to register system due to incomplete "
+            self.log_warn("Unable to register system due to incomplete "
                           "information.")
-            self.log.warn("Use either activationkey and org *or* userid "
+            self.log_warn("Use either activationkey and org *or* userid "
                           "and password")
             return False
 
@@ -219,9 +224,8 @@ class SubscriptionManager(object):
         return True
 
     def _set_service_level(self):
-        cmd = list(itertools.chain(self.subman,
-                                   ['attach', '--auto', '--servicelevel={0}'
-                                    .format(self.servicelevel)]))
+        cmd = ['attach', '--auto', '--servicelevel={0}'
+               .format(self.servicelevel)]
 
         try:
             return_out, return_err = self._sub_man_cli(cmd)
@@ -229,9 +233,9 @@ class SubscriptionManager(object):
             if e.stdout.rstrip() != '':
                 for line in e.stdout.split("\n"):
                     if line is not '':
-                        self.log.warn(line)
+                        self.log_warn(line)
             else:
-                self.log.warn("Setting the service level failed with: "
+                self.log_warn("Setting the service level failed with: "
                               "{0}".format(e.stderr.strip()))
             return False
         for line in return_out.split("\n"):
@@ -240,11 +244,11 @@ class SubscriptionManager(object):
         return True
 
     def _set_auto_attach(self):
-        cmd = list(itertools.chain(self.subman, ['attach', '--auto']))
+        cmd = ['attach', '--auto']
         try:
             return_out, return_err = self._sub_man_cli(cmd)
         except util.ProcessExecutionError:
-            self.log.warn("Auto-attach failed with: "
+            self.log_warn("Auto-attach failed with: "
                           "{0}]".format(return_err.strip()))
             return False
         for line in return_out.split("\n"):
@@ -261,14 +265,12 @@ class SubscriptionManager(object):
         consumed = []
 
         # Get all available pools
-        cmd = list(itertools.chain(self.subman, ['list', '--available',
-                                                 '--pool-only']))
+        cmd = ['list', '--available', '--pool-only']
         results, errors = self._sub_man_cli(cmd)
         available = (results.rstrip()).split("\n")
 
         # Get all consumed pools
-        cmd = list(itertools.chain(self.subman, ['list', '--consumed',
-                                                 '--pool-only']))
+        cmd = ['list', '--consumed', '--pool-only']
         results, errors = self._sub_man_cli(cmd)
         consumed = (results.rstrip()).split("\n")
 
@@ -280,14 +282,14 @@ class SubscriptionManager(object):
         them in list form.
         '''
 
-        cmd = list(itertools.chain(self.subman, ['repos', '--list-enabled']))
+        cmd = ['repos', '--list-enabled']
         return_out, return_err = self._sub_man_cli(cmd)
         active_repos = []
         for repo in return_out.split("\n"):
             if "Repo ID:" in repo:
                 active_repos.append((repo.split(':')[1]).strip())
 
-        cmd = list(itertools.chain(self.subman, ['repos', '--list-disabled']))
+        cmd = ['repos', '--list-disabled']
         return_out, return_err = self._sub_man_cli(cmd)
 
         inactive_repos = []
@@ -309,12 +311,12 @@ class SubscriptionManager(object):
 
         pool_available, pool_consumed = self._getPools()
         pool_list = []
-        cmd = list(itertools.chain(self.subman, ['attach']))
+        cmd = ['attach']
         for pool in pools:
             if (pool not in pool_consumed) and (pool in pool_available):
                 pool_list.append('--pool={0}'.format(pool))
             else:
-                self.log.warn("Pool {0} is not available".format(pool))
+                self.log_warn("Pool {0} is not available".format(pool))
         if len(pool_list) > 0:
             cmd.extend(pool_list)
             try:
@@ -324,7 +326,7 @@ class SubscriptionManager(object):
                                .replace('--pool=', ''))
                 return True
             except util.ProcessExecutionError as e:
-                self.log.warn("Unable to attach pool {0} "
+                self.log_warn("Unable to attach pool {0} "
                               "due to {1}".format(pool, e))
                 return False
 
@@ -335,12 +337,12 @@ class SubscriptionManager(object):
         executes the action to disable or enable
         '''
 
-        if (erepos is not None) and (type(erepos) is not list):
-            self.log.warn("Repo IDs must in the format of a list.")
+        if (erepos is not None) and (not isinstance(erepos, (list))):
+            self.log_warn("Repo IDs must in the format of a list.")
             return False
 
-        if (drepos is not None) and (type(drepos) is not list):
-            self.log.warn("Repo IDs must in the format of a list.")
+        if (drepos is not None) and (not isinstance(drepos, (list))):
+            self.log_warn("Repo IDs must in the format of a list.")
             return False
 
         # Bail if both lists are not populated
@@ -374,14 +376,14 @@ class SubscriptionManager(object):
                 if fail in active_repos:
                     self.log.debug("Repo {0} is already enabled".format(fail))
                 else:
-                    self.log.warn("Repo {0} does not appear to "
+                    self.log_warn("Repo {0} does not appear to "
                                   "exist".format(fail))
         if len(disable_list_fail) > 0:
             for fail in disable_list_fail:
                 self.log.debug("Repo {0} not disabled "
                                "because it is not enabled".format(fail))
 
-        cmd = list(itertools.chain(self.subman, ['repos']))
+        cmd = ['repos']
         if enable_list > 0:
             cmd.extend(enable_list)
         if disable_list > 0:
@@ -390,7 +392,7 @@ class SubscriptionManager(object):
         try:
             self._sub_man_cli(cmd)
         except util.ProcessExecutionError as e:
-            self.log.warn("Unable to alter repos due to {0}".format(e))
+            self.log_warn("Unable to alter repos due to {0}".format(e))
             return False
 
         if enable_list > 0:
diff --git a/tests/unittests/test_rh_subscription.py b/tests/unittests/test_rh_subscription.py
index ba9181ec..2f813f41 100644
--- a/tests/unittests/test_rh_subscription.py
+++ b/tests/unittests/test_rh_subscription.py
@@ -34,34 +34,33 @@ class GoodTests(unittest.TestCase):
         Emulates a system that is already registered. Ensure it gets
         a non-ProcessExecution error from is_registered()
         '''
-        good_message = 'System is already registered'
         with mock.patch.object(cc_rh_subscription.SubscriptionManager,
                                '_sub_man_cli') as mockobj:
-            self.log.info = mock.MagicMock(wraps=self.log.info)
+            self.SM.log_sucess = mock.MagicMock()
             self.handle(self.name, self.config, self.cloud_init,
                         self.log, self.args)
+            self.assertEqual(self.SM.log_sucess.call_count, 1)
             self.assertEqual(mockobj.call_count, 1)
-            self.log.info.assert_called_with(good_message)
 
     def test_simple_registration(self):
         '''
         Simple registration with username and password
         '''
-        good_message = 'rh_subscription plugin completed successfully'
-        self.log.info = mock.MagicMock(wraps=self.log.info)
+        self.SM.log_sucess = mock.MagicMock()
         reg = "The system has been registered with ID:" \
               " 12345678-abde-abcde-1234-1234567890abc"
         self.SM._sub_man_cli = mock.MagicMock(
             side_effect=[util.ProcessExecutionError, (reg, 'bar')])
         self.handle(self.name, self.config, self.cloud_init,
                     self.log, self.args)
-        self.SM._sub_man_cli.assert_called_with_once(['subscription-manager',
-                                                      'identity'])
-        self.SM._sub_man_cli.assert_called_with_once(
-            ['subscription-manager', 'register', '--username=scooby@do.com',
-             '--password=scooby-snacks'], logstring_val=True)
-
-        self.log.info.assert_called_with(good_message)
+        self.assertIn(mock.call(['identity']),
+                      self.SM._sub_man_cli.call_args_list)
+        self.assertIn(mock.call(['register', '--username=scooby@do.com',
+                                 '--password=scooby-snacks'],
+                                logstring_val=True),
+                      self.SM._sub_man_cli.call_args_list)
+
+        self.assertEqual(self.SM.log_sucess.call_count, 1)
         self.assertEqual(self.SM._sub_man_cli.call_count, 2)
 
     def test_full_registration(self):
@@ -69,12 +68,12 @@ class GoodTests(unittest.TestCase):
         Registration with auto-attach, service-level, adding pools,
         and enabling and disabling yum repos
         '''
-        pool_message = 'Pool pool2 is not available'
-        repo_message1 = 'Repo repo1 is already enabled'
-        repo_message2 = 'Enabled the following repos: repo2, repo3'
-        good_message = 'rh_subscription plugin completed successfully'
-        self.log.warn = mock.MagicMock(wraps=self.log.warn)
-        self.log.debug = mock.MagicMock(wraps=self.log.debug)
+        call_lists = []
+        call_lists.append(['attach', '--pool=pool1', '--pool=pool3'])
+        call_lists.append(['repos', '--enable=repo2', '--enable=repo3',
+                           '--disable=repo5'])
+        call_lists.append(['attach', '--auto', '--servicelevel=self-support'])
+        self.SM.log_sucess = mock.MagicMock()
         reg = "The system has been registered with ID:" \
               " 12345678-abde-abcde-1234-1234567890abc"
         self.SM._sub_man_cli = mock.MagicMock(
@@ -87,145 +86,123 @@ class GoodTests(unittest.TestCase):
                          ('', '')])
         self.handle(self.name, self.config_full, self.cloud_init,
                     self.log, self.args)
-        self.log.warn.assert_any_call(pool_message)
-        self.log.debug.assert_any_call(repo_message1)
-        self.log.debug.assert_any_call(repo_message2)
-        self.log.info.assert_any_call(good_message)
-        self.SM._sub_man_cli.assert_called_with_once(['subscription-manager',
-                                                      'attach', '-pool=pool1',
-                                                      '--pool=pool33'])
+        for call in call_lists:
+            self.assertIn(mock.call(call), self.SM._sub_man_cli.call_args_list)
+        self.assertEqual(self.SM.log_sucess.call_count, 1)
         self.assertEqual(self.SM._sub_man_cli.call_count, 9)
 
 
-class BadTests(unittest.TestCase):
+class TestBadInput(unittest.TestCase):
+    name = "cc_rh_subscription"
+    cloud_init = None
+    log = logging.getLogger("bad_tests")
+    args = []
+    SM = cc_rh_subscription.SubscriptionManager
+    reg = "The system has been registered with ID:" \
+          " 12345678-abde-abcde-1234-1234567890abc"
+
+    config_no_password = {'rh_subscription':
+                          {'username': 'scooby@do.com'
+                           }}
+
+    config_no_key = {'rh_subscription':
+                     {'activation-key': '1234abcde',
+                      }}
+
+    config_service = {'rh_subscription':
+                      {'username': 'scooby@do.com',
+                       'password': 'scooby-snacks',
+                       'service-level': 'self-support'
+                       }}
+
+    config_badpool = {'rh_subscription':
+                      {'username': 'scooby@do.com',
+                       'password': 'scooby-snacks',
+                       'add-pool': 'not_a_list'
+                       }}
+    config_badrepo = {'rh_subscription':
+                      {'username': 'scooby@do.com',
+                       'password': 'scooby-snacks',
+                       'enable-repo': 'not_a_list'
+                       }}
+    config_badkey = {'rh_subscription':
+                     {'activation_key': 'abcdef1234',
+                      'org': '123',
+                      }}
+
     def setUp(self):
-        super(BadTests, self).setUp()
-        self.name = "cc_rh_subscription"
-        self.cloud_init = None
-        self.log = logging.getLogger("bad_tests")
-        self.orig = self.log
-        self.args = []
+        super(TestBadInput, self).setUp()
         self.handle = cc_rh_subscription.handle
-        self.SM = cc_rh_subscription.SubscriptionManager
-        self.reg = "The system has been registered with ID:" \
-                   " 12345678-abde-abcde-1234-1234567890abc"
-
-        self.config_no_password = {'rh_subscription':
-                                   {'username': 'scooby@do.com'
-                                    }}
-
-        self.config_no_key = {'rh_subscription':
-                              {'activation-key': '1234abcde',
-                               }}
-
-        self.config_service = {'rh_subscription':
-                               {'username': 'scooby@do.com',
-                                'password': 'scooby-snacks',
-                                'service-level': 'self-support'
-                                }}
-
-        self.config_badpool = {'rh_subscription':
-                               {'username': 'scooby@do.com',
-                                'password': 'scooby-snacks',
-                                'add-pool': 'not_a_list'
-                                }}
-        self.config_badrepo = {'rh_subscription':
-                               {'username': 'scooby@do.com',
-                                'password': 'scooby-snacks',
-                                'enable-repo': 'not_a_list'
-                                }}
-        self.config_badkey = {'rh_subscription':
-                              {'activation_key': 'abcdef1234',
-                               'org': '123',
-                               }}
 
     def test_no_password(self):
         '''
         Attempt to register without the password key/value
         '''
-        self.missing_info(self.config_no_password)
+        self.input_is_missing_data(self.config_no_password)
 
     def test_no_org(self):
         '''
         Attempt to register without the org key/value
         '''
-        self.missing_info(self.config_no_key)
+        self.input_is_missing_data(self.config_no_key)
 
     def test_service_level_without_auto(self):
         '''
         Attempt to register using service-level without the auto-attach key
         '''
-        good_message = 'The service-level key must be used in conjunction'\
-                       ' with the auto-attach key.  Please re-run with '\
-                       'auto-attach: True'
-
-        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM.log_warn = mock.MagicMock()
         self.SM._sub_man_cli = mock.MagicMock(
             side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
         self.handle(self.name, self.config_service, self.cloud_init,
                     self.log, self.args)
-        self.log.warn.assert_any_call(good_message)
-        self.assertRaises(cc_rh_subscription.SubscriptionError)
         self.assertEqual(self.SM._sub_man_cli.call_count, 1)
+        self.assertEqual(self.SM.log_warn.call_count, 2)
 
     def test_pool_not_a_list(self):
         '''
         Register with pools that are not in the format of a list
         '''
-        good_message = "Pools must in the format of a list"
-        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM.log_warn = mock.MagicMock()
         self.SM._sub_man_cli = mock.MagicMock(
             side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
         self.handle(self.name, self.config_badpool, self.cloud_init,
                     self.log, self.args)
-        self.log.warn.assert_any_call(good_message)
-        self.assertRaises(cc_rh_subscription.SubscriptionError)
         self.assertEqual(self.SM._sub_man_cli.call_count, 2)
+        self.assertEqual(self.SM.log_warn.call_count, 2)
 
     def test_repo_not_a_list(self):
         '''
         Register with repos that are not in the format of a list
         '''
-        good_message = "Repo IDs must in the format of a list."
-        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM.log_warn = mock.MagicMock()
         self.SM._sub_man_cli = mock.MagicMock(
             side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
         self.handle(self.name, self.config_badrepo, self.cloud_init,
                     self.log, self.args)
-        self.log.warn.assert_any_call(good_message)
-        self.assertRaises(cc_rh_subscription.SubscriptionError)
+        self.assertEqual(self.SM.log_warn.call_count, 3)
         self.assertEqual(self.SM._sub_man_cli.call_count, 2)
 
     def test_bad_key_value(self):
         '''
         Attempt to register with a key that we don't know
         '''
-        good_message = 'activation_key is not a valid key for rh_subscription.'\
-                       ' Valid keys are: org, activation-key, username, '\
-                       'password, disable-repo, enable-repo, add-pool,'\
-                       ' rhsm-baseurl, server-hostname, auto-attach, '\
-                       'service-level'
-        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM.log_warn = mock.MagicMock()
         self.SM._sub_man_cli = mock.MagicMock(
             side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
         self.handle(self.name, self.config_badkey, self.cloud_init,
                     self.log, self.args)
-        self.assertRaises(cc_rh_subscription.SubscriptionError)
-        self.log.warn.assert_any_call(good_message)
+        self.assertEqual(self.SM.log_warn.call_count, 2)
         self.assertEqual(self.SM._sub_man_cli.call_count, 1)
 
-    def missing_info(self, config):
+    def input_is_missing_data(self, config):
         '''
         Helper def for tests that having missing information
         '''
-        good_message = "Unable to register system due to incomplete "\
-                       "information."
-        self.log.warn = mock.MagicMock(wraps=self.log.warn)
+        self.SM.log_warn = mock.MagicMock()
         self.SM._sub_man_cli = mock.MagicMock(
             side_effect=[util.ProcessExecutionError])
         self.handle(self.name, config, self.cloud_init,
                     self.log, self.args)
-        self.SM._sub_man_cli.assert_called_with(['subscription-manager',
-                                                 'identity'])
-        self.log.warn.assert_any_call(good_message)
+        self.SM._sub_man_cli.assert_called_with(['identity'])
+        self.assertEqual(self.SM.log_warn.call_count, 4)
         self.assertEqual(self.SM._sub_man_cli.call_count, 1)
-- 
cgit v1.2.3


From 3aa0fcc5983416d743fac6af1d40ca791feb23af Mon Sep 17 00:00:00 2001
From: Brent Baude <bbaude@redhat.com>
Date: Thu, 28 May 2015 09:02:11 -0500
Subject: Tightening up an error message and isinstance usage based on feedback
 from Dan

---
 cloudinit/config/cc_rh_subscription.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
index db3d5525..e57e8a07 100644
--- a/cloudinit/config/cc_rh_subscription.py
+++ b/cloudinit/config/cc_rh_subscription.py
@@ -50,7 +50,7 @@ def handle(_name, cfg, _cloud, log, _args):
                     sm.log.debug("Completed auto-attach")
 
             if sm.pools is not None:
-                if not isinstance(sm.pools, (list)):
+                if not isinstance(sm.pools, list):
                     pool_fail = "Pools must in the format of a list"
                     raise SubscriptionError(pool_fail)
 
@@ -122,8 +122,8 @@ class SubscriptionManager(object):
         if (self.auto_attach is not None) and \
                 not (util.is_true(self.auto_attach) or
                      util.is_false(self.auto_attach)):
-            not_bool = "The key auto-attach must be a value of "\
-                       "either True or False"
+            not_bool = "The key auto-attach must be a boolean value "\
+                       "(True/False "
             return False, not_bool
 
         if (self.servicelevel is not None) and \
@@ -337,11 +337,11 @@ class SubscriptionManager(object):
         executes the action to disable or enable
         '''
 
-        if (erepos is not None) and (not isinstance(erepos, (list))):
+        if (erepos is not None) and (not isinstance(erepos, list)):
             self.log_warn("Repo IDs must in the format of a list.")
             return False
 
-        if (drepos is not None) and (not isinstance(drepos, (list))):
+        if (drepos is not None) and (not isinstance(drepos, list)):
             self.log_warn("Repo IDs must in the format of a list.")
             return False
 
-- 
cgit v1.2.3


From 3c01b8e48400697362f190984ab9c96dee27a369 Mon Sep 17 00:00:00 2001
From: Brent Baude <bbaude@redhat.com>
Date: Fri, 29 May 2015 09:18:49 -0500
Subject: Corrected spelling error on variable name

---
 cloudinit/config/cc_rh_subscription.py  |  6 +++---
 tests/unittests/test_rh_subscription.py | 12 ++++++------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
index e57e8a07..cabebca4 100644
--- a/cloudinit/config/cc_rh_subscription.py
+++ b/cloudinit/config/cc_rh_subscription.py
@@ -62,12 +62,12 @@ def handle(_name, cfg, _cloud, log, _args):
                 return_stat = sm.update_repos(sm.enable_repo, sm.disable_repo)
                 if not return_stat:
                     raise SubscriptionError("Unable to add or remove repos")
-            sm.log_sucess("rh_subscription plugin completed successfully")
+            sm.log_success("rh_subscription plugin completed successfully")
         except SubscriptionError as e:
             sm.log_warn(str(e))
             sm.log_warn("rh_subscription plugin did not complete successfully")
     else:
-        sm.log_sucess("System is already registered")
+        sm.log_success("System is already registered")
 
 
 class SubscriptionError(Exception):
@@ -97,7 +97,7 @@ class SubscriptionManager(object):
         self.subman = ['subscription-manager']
         self.is_registered = self._is_registered()
 
-    def log_sucess(self, msg):
+    def log_success(self, msg):
         '''Simple wrapper for logging info messages. Useful for unittests'''
         self.log.info(msg)
 
diff --git a/tests/unittests/test_rh_subscription.py b/tests/unittests/test_rh_subscription.py
index 2f813f41..38d5763a 100644
--- a/tests/unittests/test_rh_subscription.py
+++ b/tests/unittests/test_rh_subscription.py
@@ -36,17 +36,17 @@ class GoodTests(unittest.TestCase):
         '''
         with mock.patch.object(cc_rh_subscription.SubscriptionManager,
                                '_sub_man_cli') as mockobj:
-            self.SM.log_sucess = mock.MagicMock()
+            self.SM.log_success = mock.MagicMock()
             self.handle(self.name, self.config, self.cloud_init,
                         self.log, self.args)
-            self.assertEqual(self.SM.log_sucess.call_count, 1)
+            self.assertEqual(self.SM.log_success.call_count, 1)
             self.assertEqual(mockobj.call_count, 1)
 
     def test_simple_registration(self):
         '''
         Simple registration with username and password
         '''
-        self.SM.log_sucess = mock.MagicMock()
+        self.SM.log_success = mock.MagicMock()
         reg = "The system has been registered with ID:" \
               " 12345678-abde-abcde-1234-1234567890abc"
         self.SM._sub_man_cli = mock.MagicMock(
@@ -60,7 +60,7 @@ class GoodTests(unittest.TestCase):
                                 logstring_val=True),
                       self.SM._sub_man_cli.call_args_list)
 
-        self.assertEqual(self.SM.log_sucess.call_count, 1)
+        self.assertEqual(self.SM.log_success.call_count, 1)
         self.assertEqual(self.SM._sub_man_cli.call_count, 2)
 
     def test_full_registration(self):
@@ -73,7 +73,7 @@ class GoodTests(unittest.TestCase):
         call_lists.append(['repos', '--enable=repo2', '--enable=repo3',
                            '--disable=repo5'])
         call_lists.append(['attach', '--auto', '--servicelevel=self-support'])
-        self.SM.log_sucess = mock.MagicMock()
+        self.SM.log_success = mock.MagicMock()
         reg = "The system has been registered with ID:" \
               " 12345678-abde-abcde-1234-1234567890abc"
         self.SM._sub_man_cli = mock.MagicMock(
@@ -88,7 +88,7 @@ class GoodTests(unittest.TestCase):
                     self.log, self.args)
         for call in call_lists:
             self.assertIn(mock.call(call), self.SM._sub_man_cli.call_args_list)
-        self.assertEqual(self.SM.log_sucess.call_count, 1)
+        self.assertEqual(self.SM.log_success.call_count, 1)
         self.assertEqual(self.SM._sub_man_cli.call_count, 9)
 
 
-- 
cgit v1.2.3