Merge ~powersj/cloud-init:fix-centos-unittests into cloud-init:master

[Joshua Powers]

unittests: fix unittests run on centos

Apt related tests were broken when running on centos becasue apt
is not available. This fixes the unit test, with a small re-work of apt_configure.

Also in 'tox -e centos6' only run nose on tests/unittests as tests/
also contain integration tests that should not be run.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:9ed5081b4654a04f9a9c01b752b84e26c97789ea
https://jenkins.ubuntu.com/server/job/cloud-init-ci/290/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/290

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/290/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks for taking this.

1) let's use @mock.patch.object decorators on the helper function for the common items (write, isfile, etc)

2) instead of testing all possible paths in the helper function and relying on the config_on_empty list, pull out the asserts to the test_* callers; they're setting up the scenario (distro=debian, cfg=XXX); they're in the best position to assert a specific path through the code.

3) Let's add a distro=ubuntu system_is_snappy=True case

4) Let's add distro=centos which should fall down the 'no apt binary' path

review: Needs Fixing

[Joshua Powers (powersj)]

If I use decorators on the helper function, but try to move asserts to the callers how is that going to work? From looking at this more it looks like every caller function would start to look like test_apt_v3_srcl_custom and have its own mocked objects and not even use the helper function.

[Scott Moser (smoser)]

You could have the helper function resturn the mocks and the result of the call. There is a helper wrap_and_call which does something similar, but at the moment does not return the mocks.

One option is to modify that to return the mocks also.

On April 28, 2017 2:30:40 PM EDT, Joshua Powers <email address hidden> wrote:
>If I use decorators on the helper function, but try to move asserts to
>the callers how is that going to work? From looking at this more it
>looks like every caller function would start to look like
>test_apt_v3_srcl_custom and have its own mocked objects and not even
>use the helper function.
>--
>https://code.launchpad.net/~powersj/cloud-init/+git/cloud-init/+merge/323351
>You are subscribed to branch cloud-init:master.

c7a3952... by Joshua Powers on 2017-05-01

Moving assert logic, using mock as context manager since using default arguments

1d23c8c... by Joshua Powers on 2017-05-01

fix

8e899e0... by Joshua Powers on 2017-05-01

revert fix

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:c7a39525da0960c2865632c019e06c6290c7f81d
https://jenkins.ubuntu.com/server/job/cloud-init-ci/294/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/294

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/294/rebuild

review: Approve (continuous-integration)

ed61b61... by Joshua Powers on 2017-05-01

slight change

[Joshua Powers (powersj)]

Update:

* I used mock context managers over the use of the decorators. I was unable to even move a single one up without getting some odd errors. I can show you want I had, but we now have two default arguments, so having non-default arguments follow default arguments won't work in any case.

* Like Scott suggested I returned the mocks (never done that before, but like how it worked) and now the callers are running the asserts.

* Added a centos/rhel test

* Added a is_snappy variable and test case for ubuntu_snappy. Not sure on the unit test on this one other than to verify it wasn't called, similar to centos/rhel.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:ed61b61094ed46726bcf69a6e2b4815eee54f9cf
https://jenkins.ubuntu.com/server/job/cloud-init-ci/295/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/295
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/295
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/295
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/295
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/295

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/295/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

I like this a lot better, thanks for reworking this.

Couple of items to address.

review: Needs Fixing

d5c58ed... by Joshua Powers on 2017-05-02

Revamping is_snappy test and variable names

[Joshua Powers (powersj)]

Because we already mock _should_configure_on_empty_apt I decided to pull the snappy check into its own separate function that is simple. If you disagree let me know.

Also updated all variable names to something more verbose.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:d5c58edce65cb46a5447f4955e07590f9a336dfa
https://jenkins.ubuntu.com/server/job/cloud-init-ci/296/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/296
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/296
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/296
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/296/console
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/296

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/296/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:d5c58edce65cb46a5447f4955e07590f9a336dfa
https://jenkins.ubuntu.com/server/job/cloud-init-ci/297/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/297

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/297/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Generally looks good, couple questions below.

[Joshua Powers (powersj)]

I'll push changes shortly. Reply below.

da2f7ad... by Joshua Powers on 2017-05-03

ryans changes

[Joshua Powers (powersj)]

I forgot comments get removed when I push, so:

1) I changed the line breaks from using / to use ()
2) I updated to using assert_called_with as suggested
3) +commands = nosetests {posargs:tests/unittests} is required as in the tests folder we have integration tests that contain unit tests that should not be run during unit testing.

Lasted push includes these changes/fixes.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:da2f7adc71a2d3b8fc7a7c70868474e26732a986
https://jenkins.ubuntu.com/server/job/cloud-init-ci/299/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/299
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/299
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/299
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/299

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/299/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

review: Approve

[Joshua Powers (powersj)]

Confirmed on fresh centos 6 & 7 LXD as working. This one is ready to go.

diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py
index f6885d7..177cbcf 100644
--- a/cloudinit/config/cc_apt_configure.py
+++ b/cloudinit/config/cc_apt_configure.py
@@ -282,16 +282,21 @@ def handle(name, ocfg, cloud, log, _):
     apply_apt(cfg, cloud, target)
 
 
+def _should_configure_on_empty_apt():
+    # if no config was provided, should apt configuration be done?
+    if util.system_is_snappy():
+        return False, "system is snappy."
+    if not (util.which('apt-get') or util.which('apt')):
+        return False, "no apt commands."
+    return True, "Apt is available."
+
+
 def apply_apt(cfg, cloud, target):
     # cfg is the 'apt' top level dictionary already in 'v3' format.
     if not cfg:
-        # no config was provided.  If apt configuration does not seem
-        # necessary on this system, then return.
-        if util.system_is_snappy():
-            LOG.debug("Nothing to do: No apt config and running on snappy")
-            return
-        if not (util.which('apt-get') or util.which('apt')):
-            LOG.debug("Nothing to do: No apt config and no apt commands")
+        should_config, msg = _should_configure_on_empty_apt()
+        if not should_config:
+            LOG.debug("Nothing to do: No apt config and %s", msg)
             return
 
     LOG.debug("handling apt config: %s", cfg)
diff --git a/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v3.py b/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v3.py
index 24e4523..1ca915b 100644
--- a/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v3.py
+++ b/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v3.py
@@ -121,39 +121,82 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
             myds.metadata.update(metadata)
         return cloud.Cloud(myds, paths, {}, mydist, None)
 
-    def _apt_source_list(self, cfg, expected, distro):
-        "_apt_source_list - Test rendering from template (generic)"
-
+    def _apt_source_list(self, distro, cfg, cfg_on_empty=False):
+        """_apt_source_list - Test rendering from template (generic)"""
         # entry at top level now, wrap in 'apt' key
         cfg = {'apt': cfg}
         mycloud = self._get_cloud(distro)
-        with mock.patch.object(util, 'write_file') as mockwf:
+
+        with mock.patch.object(util, 'write_file') as mock_writefile:
             with mock.patch.object(util, 'load_file',
-                                   return_value=MOCKED_APT_SRC_LIST) as mocklf:
+                                   return_value=MOCKED_APT_SRC_LIST
+                                   ) as mock_loadfile:
                 with mock.patch.object(os.path, 'isfile',
-                                       return_value=True) as mockisfile:
-                    with mock.patch.object(util, 'rename'):
-                        cc_apt_configure.handle("test", cfg, mycloud,
-                                                LOG, None)
-
-        # check if it would have loaded the distro template
-        mockisfile.assert_any_call(
-            ('/etc/cloud/templates/sources.list.%s.tmpl' % distro))
-        mocklf.assert_any_call(
-            ('/etc/cloud/templates/sources.list.%s.tmpl' % distro))
-        # check expected content in result
-        mockwf.assert_called_once_with('/etc/apt/sources.list', expected,
-                                       mode=0o644)
+                                       return_value=True) as mock_isfile:
+                    cfg_func = ('cloudinit.config.cc_apt_configure.' +
+                                '_should_configure_on_empty_apt')
+                    with mock.patch(cfg_func,
+                                    return_value=(cfg_on_empty, "test")
+                                    ) as mock_shouldcfg:
+                        cc_apt_configure.handle("test", cfg, mycloud, LOG,
+                                                None)
+
+        return mock_writefile, mock_loadfile, mock_isfile, mock_shouldcfg
 
     def test_apt_v3_source_list_debian(self):
         """test_apt_v3_source_list_debian - without custom sources or parms"""
         cfg = {}
-        self._apt_source_list(cfg, EXPECTED_BASE_CONTENT, 'debian')
+        distro = 'debian'
+        expected = EXPECTED_BASE_CONTENT
+
+        mock_writefile, mock_load_file, mock_isfile, mock_shouldcfg = (
+            self._apt_source_list(distro, cfg, cfg_on_empty=True))
+
+        template = '/etc/cloud/templates/sources.list.%s.tmpl' % distro
+        mock_writefile.assert_called_once_with('/etc/apt/sources.list',
+                                               expected, mode=0o644)
+        mock_load_file.assert_called_with(template)
+        mock_isfile.assert_any_call(template)
+        self.assertEqual(1, mock_shouldcfg.call_count)
 
     def test_apt_v3_source_list_ubuntu(self):
         """test_apt_v3_source_list_ubuntu - without custom sources or parms"""
         cfg = {}
-        self._apt_source_list(cfg, EXPECTED_BASE_CONTENT, 'ubuntu')
+        distro = 'ubuntu'
+        expected = EXPECTED_BASE_CONTENT
+
+        mock_writefile, mock_load_file, mock_isfile, mock_shouldcfg = (
+            self._apt_source_list(distro, cfg, cfg_on_empty=True))
+
+        template = '/etc/cloud/templates/sources.list.%s.tmpl' % distro
+        mock_writefile.assert_called_once_with('/etc/apt/sources.list',
+                                               expected, mode=0o644)
+        mock_load_file.assert_called_with(template)
+        mock_isfile.assert_any_call(template)
+        self.assertEqual(1, mock_shouldcfg.call_count)
+
+    def test_apt_v3_source_list_ubuntu_snappy(self):
+        """test_apt_v3_source_list_ubuntu_snappy - without custom sources or
+        parms"""
+        cfg = {'apt': {}}
+        mycloud = self._get_cloud('ubuntu')
+
+        with mock.patch.object(util, 'write_file') as mock_writefile:
+            with mock.patch.object(util, 'system_is_snappy',
+                                   return_value=True) as mock_issnappy:
+                cc_apt_configure.handle("test", cfg, mycloud, LOG, None)
+
+        self.assertEqual(0, mock_writefile.call_count)
+        self.assertEqual(1, mock_issnappy.call_count)
+
+    def test_apt_v3_source_list_centos(self):
+        """test_apt_v3_source_list_centos - without custom sources or parms"""
+        cfg = {}
+        distro = 'rhel'
+
+        mock_writefile, _, _, _ = self._apt_source_list(distro, cfg)
+
+        self.assertEqual(0, mock_writefile.call_count)
 
     def test_apt_v3_source_list_psm(self):
         """test_apt_v3_source_list_psm - Test specifying prim+sec mirrors"""
@@ -164,8 +207,17 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
                             'uri': pm}],
                'security': [{'arches': ["default"],
                              'uri': sm}]}
+        distro = 'ubuntu'
+        expected = EXPECTED_PRIMSEC_CONTENT
+
+        mock_writefile, mock_load_file, mock_isfile, _ = (
+            self._apt_source_list(distro, cfg, cfg_on_empty=True))
 
-        self._apt_source_list(cfg, EXPECTED_PRIMSEC_CONTENT, 'ubuntu')
+        template = '/etc/cloud/templates/sources.list.%s.tmpl' % distro
+        mock_writefile.assert_called_once_with('/etc/apt/sources.list',
+                                               expected, mode=0o644)
+        mock_load_file.assert_called_with(template)
+        mock_isfile.assert_any_call(template)
 
     def test_apt_v3_srcl_custom(self):
         """test_apt_v3_srcl_custom - Test rendering a custom source template"""
diff --git a/tests/unittests/test_handler/test_handler_yum_add_repo.py b/tests/unittests/test_handler/test_handler_yum_add_repo.py
index 4815bdb..c4396df 100644
--- a/tests/unittests/test_handler/test_handler_yum_add_repo.py
+++ b/tests/unittests/test_handler/test_handler_yum_add_repo.py
@@ -72,7 +72,7 @@ class TestConfig(helpers.FilesystemMockingTestCase):
         }
         for section in expected:
             self.assertTrue(parser.has_section(section),
-                            "Contains section {}".format(section))
+                            "Contains section {0}".format(section))
             for k, v in expected[section].items():
                 self.assertEqual(parser.get(section, k), v)
 
@@ -109,7 +109,7 @@ class TestConfig(helpers.FilesystemMockingTestCase):
         }
         for section in expected:
             self.assertTrue(parser.has_section(section),
-                            "Contains section {}".format(section))
+                            "Contains section {0}".format(section))
             for k, v in expected[section].items():
                 self.assertEqual(parser.get(section, k), v)
 
diff --git a/tox.ini b/tox.ini
index bf9046a..826f554 100644
--- a/tox.ini
+++ b/tox.ini
@@ -70,7 +70,7 @@ deps =
 
 [testenv:centos6]
 basepython = python2.6
-commands = nosetests {posargs:tests}
+commands = nosetests {posargs:tests/unittests}
 deps =
     # requirements
     argparse==1.2.1