cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #05200
[Merge] ~smoser/cloud-init:fix/add-rhel-to-get-linux-distro into cloud-init:master
Scott Moser has proposed merging ~smoser/cloud-init:fix/add-rhel-to-get-linux-distro into cloud-init:master.
Commit message:
pylint: Fix pylint warnings reported in pylint 2.0.0.
Pylint 2.0.0 was recently released and complains more about
logging-not-lazy than it used to. I've fixed those warnings, here.
The changes in rh_subscription are more extensive. pylint may be
complaining incorrectly there, but the tests were not correctly un-doing
all of their mock/patching. This cleans those up and makes pylint happy.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/350376
see commit message
--
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/add-rhel-to-get-linux-distro into cloud-init:master.
diff --git a/cloudinit/config/cc_lxd.py b/cloudinit/config/cc_lxd.py
index ac72ac4..a604825 100644
--- a/cloudinit/config/cc_lxd.py
+++ b/cloudinit/config/cc_lxd.py
@@ -276,27 +276,27 @@ def maybe_cleanup_default(net_name, did_init, create, attach,
if net_name != _DEFAULT_NETWORK_NAME or not did_init:
return
- fail_assume_enoent = " failed. Assuming it did not exist."
- succeeded = " succeeded."
+ fail_assume_enoent = "failed. Assuming it did not exist."
+ succeeded = "succeeded."
if create:
- msg = "Deletion of lxd network '%s'" % net_name
+ msg = "Deletion of lxd network '%s' %s"
try:
_lxc(["network", "delete", net_name])
- LOG.debug(msg + succeeded)
+ LOG.debug(msg, net_name, succeeded)
except util.ProcessExecutionError as e:
if e.exit_code != 1:
raise e
- LOG.debug(msg + fail_assume_enoent)
+ LOG.debug(msg, net_name, fail_assume_enoent)
if attach:
- msg = "Removal of device '%s' from profile '%s'" % (nic_name, profile)
+ msg = "Removal of device '%s' from profile '%s' %s"
try:
_lxc(["profile", "device", "remove", profile, nic_name])
- LOG.debug(msg + succeeded)
+ LOG.debug(msg, nic_name, profile, succeeded)
except util.ProcessExecutionError as e:
if e.exit_code != 1:
raise e
- LOG.debug(msg + fail_assume_enoent)
+ LOG.debug(msg, nic_name, profile, fail_assume_enoent)
# vi: ts=4 expandtab
diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
index 1c67943..edee01e 100644
--- a/cloudinit/config/cc_rh_subscription.py
+++ b/cloudinit/config/cc_rh_subscription.py
@@ -126,7 +126,6 @@ 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 = ['subscription-manager']
def log_success(self, msg):
'''Simple wrapper for logging info messages. Useful for unittests'''
@@ -173,21 +172,12 @@ class SubscriptionManager(object):
cmd = ['identity']
try:
- self._sub_man_cli(cmd)
+ _sub_man_cli(cmd)
except util.ProcessExecutionError:
return False
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
- '''
- cmd = self.subman + cmd
- return util.subp(cmd, logstring=logstring_val)
-
def rhn_register(self):
'''
Registers the system by userid and password or activation key
@@ -209,7 +199,7 @@ class SubscriptionManager(object):
cmd.append("--serverurl={0}".format(self.server_hostname))
try:
- return_out = self._sub_man_cli(cmd, logstring_val=True)[0]
+ return_out = _sub_man_cli(cmd, logstring_val=True)[0]
except util.ProcessExecutionError as e:
if e.stdout == "":
self.log_warn("Registration failed due "
@@ -232,7 +222,7 @@ class SubscriptionManager(object):
# Attempting to register the system only
try:
- return_out = self._sub_man_cli(cmd, logstring_val=True)[0]
+ return_out = _sub_man_cli(cmd, logstring_val=True)[0]
except util.ProcessExecutionError as e:
if e.stdout == "":
self.log_warn("Registration failed due "
@@ -255,7 +245,7 @@ class SubscriptionManager(object):
.format(self.servicelevel)]
try:
- return_out = self._sub_man_cli(cmd)[0]
+ return_out = _sub_man_cli(cmd)[0]
except util.ProcessExecutionError as e:
if e.stdout.rstrip() != '':
for line in e.stdout.split("\n"):
@@ -273,7 +263,7 @@ class SubscriptionManager(object):
def _set_auto_attach(self):
cmd = ['attach', '--auto']
try:
- return_out = self._sub_man_cli(cmd)[0]
+ return_out = _sub_man_cli(cmd)[0]
except util.ProcessExecutionError as e:
self.log_warn("Auto-attach failed with: {0}".format(e))
return False
@@ -292,12 +282,12 @@ class SubscriptionManager(object):
# Get all available pools
cmd = ['list', '--available', '--pool-only']
- results = self._sub_man_cli(cmd)[0]
+ results = _sub_man_cli(cmd)[0]
available = (results.rstrip()).split("\n")
# Get all consumed pools
cmd = ['list', '--consumed', '--pool-only']
- results = self._sub_man_cli(cmd)[0]
+ results = _sub_man_cli(cmd)[0]
consumed = (results.rstrip()).split("\n")
return available, consumed
@@ -309,14 +299,14 @@ class SubscriptionManager(object):
'''
cmd = ['repos', '--list-enabled']
- return_out = self._sub_man_cli(cmd)[0]
+ return_out = _sub_man_cli(cmd)[0]
active_repos = []
for repo in return_out.split("\n"):
if "Repo ID:" in repo:
active_repos.append((repo.split(':')[1]).strip())
cmd = ['repos', '--list-disabled']
- return_out = self._sub_man_cli(cmd)[0]
+ return_out = _sub_man_cli(cmd)[0]
inactive_repos = []
for repo in return_out.split("\n"):
@@ -346,7 +336,7 @@ class SubscriptionManager(object):
if len(pool_list) > 0:
cmd.extend(pool_list)
try:
- self._sub_man_cli(cmd)
+ _sub_man_cli(cmd)
self.log.debug("Attached the following pools to your "
"system: %s", (", ".join(pool_list))
.replace('--pool=', ''))
@@ -423,7 +413,7 @@ class SubscriptionManager(object):
cmd.extend(enable_list)
try:
- self._sub_man_cli(cmd)
+ _sub_man_cli(cmd)
except util.ProcessExecutionError as e:
self.log_warn("Unable to alter repos due to {0}".format(e))
return False
@@ -439,4 +429,15 @@ class SubscriptionManager(object):
def is_configured(self):
return bool((self.userid and self.password) or self.activation_key)
+
+def _sub_man_cli(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 util.subp(['subscription-manager'] + cmd,
+ logstring=logstring_val)
+
+
# vi: ts=4 expandtab
diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
index f92e8b5..ad8cfb9 100644
--- a/cloudinit/sources/DataSourceSmartOS.py
+++ b/cloudinit/sources/DataSourceSmartOS.py
@@ -564,7 +564,7 @@ class JoyentMetadataSerialClient(JoyentMetadataClient):
continue
LOG.warning('Unexpected response "%s" during flush', response)
except JoyentMetadataTimeoutException:
- LOG.warning('Timeout while initializing metadata client. ' +
+ LOG.warning('Timeout while initializing metadata client. '
'Is the host metadata service running?')
LOG.debug('Got "invalid command". Flush complete.')
self.fp.timeout = timeout
diff --git a/cloudinit/warnings.py b/cloudinit/warnings.py
index f9f7a63..1da90c4 100644
--- a/cloudinit/warnings.py
+++ b/cloudinit/warnings.py
@@ -130,7 +130,7 @@ def show_warning(name, cfg=None, sleep=None, mode=True, **kwargs):
os.path.join(_get_warn_dir(cfg), name),
topline + "\n".join(fmtlines) + "\n" + topline)
- LOG.warning(topline + "\n".join(fmtlines) + "\n" + closeline)
+ LOG.warning("%s%s\n%s", topline, "\n".join(fmtlines), closeline)
if sleep:
LOG.debug("sleeping %d seconds for warning '%s'", sleep, name)
diff --git a/tests/unittests/test_rh_subscription.py b/tests/unittests/test_rh_subscription.py
index 2271810..4cd27ee 100644
--- a/tests/unittests/test_rh_subscription.py
+++ b/tests/unittests/test_rh_subscription.py
@@ -8,10 +8,16 @@ import logging
from cloudinit.config import cc_rh_subscription
from cloudinit import util
-from cloudinit.tests.helpers import TestCase, mock
+from cloudinit.tests.helpers import CiTestCase, mock
+SUBMGR = cc_rh_subscription.SubscriptionManager
+SUB_MAN_CLI = 'cloudinit.config.cc_rh_subscription._sub_man_cli'
+
+
+@mock.patch(SUB_MAN_CLI)
+class GoodTests(CiTestCase):
+ with_logs = True
-class GoodTests(TestCase):
def setUp(self):
super(GoodTests, self).setUp()
self.name = "cc_rh_subscription"
@@ -19,7 +25,6 @@ class GoodTests(TestCase):
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@xxxxxx',
@@ -35,55 +40,47 @@ class GoodTests(TestCase):
'disable-repo': ['repo4', 'repo5']
}}
- def test_already_registered(self):
+ def test_already_registered(self, m_sman_cli):
'''
Emulates a system that is already registered. Ensure it gets
a non-ProcessExecution error from is_registered()
'''
- with mock.patch.object(cc_rh_subscription.SubscriptionManager,
- '_sub_man_cli') as mockobj:
- self.SM.log_success = mock.MagicMock()
- self.handle(self.name, self.config, self.cloud_init,
- self.log, self.args)
- self.assertEqual(self.SM.log_success.call_count, 1)
- self.assertEqual(mockobj.call_count, 1)
-
- def test_simple_registration(self):
+ self.handle(self.name, self.config, self.cloud_init,
+ self.log, self.args)
+ self.assertEqual(m_sman_cli.call_count, 1)
+ self.assertIn('System is already registered', self.logs.getvalue())
+
+ def test_simple_registration(self, m_sman_cli):
'''
Simple registration with username and password
'''
- 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(
- side_effect=[util.ProcessExecutionError, (reg, 'bar')])
+ m_sman_cli.side_effect = [util.ProcessExecutionError, (reg, 'bar')]
self.handle(self.name, self.config, self.cloud_init,
self.log, self.args)
- self.assertIn(mock.call(['identity']),
- self.SM._sub_man_cli.call_args_list)
+ self.assertIn(mock.call(['identity']), m_sman_cli.call_args_list)
self.assertIn(mock.call(['register', '--username=scooby@xxxxxx',
'--password=scooby-snacks'],
logstring_val=True),
- self.SM._sub_man_cli.call_args_list)
-
- self.assertEqual(self.SM.log_success.call_count, 1)
- self.assertEqual(self.SM._sub_man_cli.call_count, 2)
+ m_sman_cli.call_args_list)
+ self.assertIn('rh_subscription plugin completed successfully',
+ self.logs.getvalue())
+ self.assertEqual(m_sman_cli.call_count, 2)
@mock.patch.object(cc_rh_subscription.SubscriptionManager, "_getRepos")
- @mock.patch.object(cc_rh_subscription.SubscriptionManager, "_sub_man_cli")
- def test_update_repos_disable_with_none(self, m_sub_man_cli, m_get_repos):
+ def test_update_repos_disable_with_none(self, m_get_repos, m_sman_cli):
cfg = copy.deepcopy(self.config)
m_get_repos.return_value = ([], ['repo1'])
- m_sub_man_cli.return_value = (b'', b'')
cfg['rh_subscription'].update(
{'enable-repo': ['repo1'], 'disable-repo': None})
mysm = cc_rh_subscription.SubscriptionManager(cfg)
self.assertEqual(True, mysm.update_repos())
m_get_repos.assert_called_with()
- self.assertEqual(m_sub_man_cli.call_args_list,
+ self.assertEqual(m_sman_cli.call_args_list,
[mock.call(['repos', '--enable=repo1'])])
- def test_full_registration(self):
+ def test_full_registration(self, m_sman_cli):
'''
Registration with auto-attach, service-level, adding pools,
and enabling and disabling yum repos
@@ -93,26 +90,28 @@ class GoodTests(TestCase):
call_lists.append(['repos', '--disable=repo5', '--enable=repo2',
'--enable=repo3'])
call_lists.append(['attach', '--auto', '--servicelevel=self-support'])
- 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(
- 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', ''),
- ('', '')])
+ m_sman_cli.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.assertEqual(m_sman_cli.call_count, 9)
for call in call_lists:
- self.assertIn(mock.call(call), self.SM._sub_man_cli.call_args_list)
- self.assertEqual(self.SM.log_success.call_count, 1)
- self.assertEqual(self.SM._sub_man_cli.call_count, 9)
+ self.assertIn(mock.call(call), m_sman_cli.call_args_list)
+ self.assertIn("rh_subscription plugin completed successfully",
+ self.logs.getvalue())
-class TestBadInput(TestCase):
+@mock.patch(SUB_MAN_CLI)
+class TestBadInput(CiTestCase):
+ with_logs = True
name = "cc_rh_subscription"
cloud_init = None
log = logging.getLogger("bad_tests")
@@ -155,81 +154,81 @@ class TestBadInput(TestCase):
super(TestBadInput, self).setUp()
self.handle = cc_rh_subscription.handle
- def test_no_password(self):
- '''
- Attempt to register without the password key/value
- '''
- self.SM._sub_man_cli = mock.MagicMock(
- side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+ def assert_logged_warnings(self, warnings):
+ logs = self.logs.getvalue()
+ missing = [w for w in warnings if "WARNING: " + w not in logs]
+ self.assertEqual([], missing, "Missing expected warnings.")
+
+ def test_no_password(self, m_sman_cli):
+ '''Attempt to register without the password key/value.'''
+ m_sman_cli.side_effect = [util.ProcessExecutionError,
+ (self.reg, 'bar')]
self.handle(self.name, self.config_no_password, self.cloud_init,
self.log, self.args)
- self.assertEqual(self.SM._sub_man_cli.call_count, 0)
+ self.assertEqual(m_sman_cli.call_count, 0)
- def test_no_org(self):
- '''
- Attempt to register without the org key/value
- '''
- 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
- '''
- self.SM.log_warn = mock.MagicMock()
- self.SM._sub_man_cli = mock.MagicMock(
- side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+ def test_no_org(self, m_sman_cli):
+ '''Attempt to register without the org key/value.'''
+ m_sman_cli.side_effect = [util.ProcessExecutionError]
+ self.handle(self.name, self.config_no_key, self.cloud_init,
+ self.log, self.args)
+ m_sman_cli.assert_called_with(['identity'])
+ self.assertEqual(m_sman_cli.call_count, 1)
+ self.assert_logged_warnings((
+ 'Unable to register system due to incomplete information.',
+ 'Use either activationkey and org *or* userid and password',
+ 'Registration failed or did not run completely',
+ 'rh_subscription plugin did not complete successfully'))
+
+ def test_service_level_without_auto(self, m_sman_cli):
+ '''Attempt to register using service-level without auto-attach key.'''
+ m_sman_cli.side_effect = [util.ProcessExecutionError,
+ (self.reg, 'bar')]
self.handle(self.name, self.config_service, self.cloud_init,
self.log, self.args)
- self.assertEqual(self.SM._sub_man_cli.call_count, 1)
- self.assertEqual(self.SM.log_warn.call_count, 2)
+ self.assertEqual(m_sman_cli.call_count, 1)
+ self.assert_logged_warnings((
+ 'The service-level key must be used in conjunction with ',
+ 'rh_subscription plugin did not complete successfully'))
- def test_pool_not_a_list(self):
+ def test_pool_not_a_list(self, m_sman_cli):
'''
Register with pools that are not in the format of a list
'''
- self.SM.log_warn = mock.MagicMock()
- self.SM._sub_man_cli = mock.MagicMock(
- side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+ m_sman_cli.side_effect = [util.ProcessExecutionError,
+ (self.reg, 'bar')]
self.handle(self.name, self.config_badpool, self.cloud_init,
self.log, self.args)
- self.assertEqual(self.SM._sub_man_cli.call_count, 2)
- self.assertEqual(self.SM.log_warn.call_count, 2)
+ self.assertEqual(m_sman_cli.call_count, 2)
+ self.assert_logged_warnings((
+ 'Pools must in the format of a list',
+ 'rh_subscription plugin did not complete successfully'))
- def test_repo_not_a_list(self):
+ def test_repo_not_a_list(self, m_sman_cli):
'''
Register with repos that are not in the format of a list
'''
- self.SM.log_warn = mock.MagicMock()
- self.SM._sub_man_cli = mock.MagicMock(
- side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+ m_sman_cli.side_effect = [util.ProcessExecutionError,
+ (self.reg, 'bar')]
self.handle(self.name, self.config_badrepo, self.cloud_init,
self.log, self.args)
- self.assertEqual(self.SM.log_warn.call_count, 3)
- self.assertEqual(self.SM._sub_man_cli.call_count, 2)
+ self.assertEqual(m_sman_cli.call_count, 2)
+ self.assert_logged_warnings((
+ 'Repo IDs must in the format of a list.',
+ 'Unable to add or remove repos',
+ 'rh_subscription plugin did not complete successfully'))
- def test_bad_key_value(self):
+ def test_bad_key_value(self, m_sman_cli):
'''
Attempt to register with a key that we don't know
'''
- self.SM.log_warn = mock.MagicMock()
- self.SM._sub_man_cli = mock.MagicMock(
- side_effect=[util.ProcessExecutionError, (self.reg, 'bar')])
+ m_sman_cli.side_effect = [util.ProcessExecutionError,
+ (self.reg, 'bar')]
self.handle(self.name, self.config_badkey, self.cloud_init,
self.log, self.args)
- self.assertEqual(self.SM.log_warn.call_count, 2)
- self.assertEqual(self.SM._sub_man_cli.call_count, 1)
-
- def input_is_missing_data(self, config):
- '''
- Helper def for tests that having missing information
- '''
- 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(['identity'])
- self.assertEqual(self.SM.log_warn.call_count, 4)
- self.assertEqual(self.SM._sub_man_cli.call_count, 1)
+ self.assertEqual(m_sman_cli.call_count, 1)
+ self.assert_logged_warnings((
+ 'fookey is not a valid key for rh_subscription. Valid keys are:',
+ 'rh_subscription plugin did not complete successfully'))
# vi: ts=4 expandtab
Follow ups
