From 958936e7757ea3a328946fc529c8f539354e4665 Mon Sep 17 00:00:00 2001 From: Rabi Mishra Date: Wed, 5 Aug 2020 11:22:41 +0530 Subject: [PATCH] Don't build image upload tasks when dry_run=True Don't unnecessarily call ImageUploader when dry_run=True, as we only expect to update the stack environment. Depends-On: https://review.opendev.org/744836 Change-Id: I37698da0ce39ed1a8f80e4c3e8c3c3d865d952ee --- tripleo_common/image/image_uploader.py | 14 ++----- tripleo_common/image/kolla_builder.py | 3 +- .../tests/image/test_image_uploader.py | 42 +------------------ .../tests/image/test_kolla_builder.py | 18 +------- 4 files changed, 7 insertions(+), 70 deletions(-) diff --git a/tripleo_common/image/image_uploader.py b/tripleo_common/image/image_uploader.py index 96097225c..620f6cba7 100644 --- a/tripleo_common/image/image_uploader.py +++ b/tripleo_common/image/image_uploader.py @@ -379,7 +379,7 @@ class ImageUploadManager(BaseImageManager): """ def __init__(self, config_files=None, - dry_run=False, cleanup=CLEANUP_FULL, + cleanup=CLEANUP_FULL, mirrors=None, registry_credentials=None, multi_arch=False, lock=None): if config_files is None: @@ -390,7 +390,6 @@ class ImageUploadManager(BaseImageManager): 'python': PythonImageUploader() } self.uploaders['python'].init_global_state(lock) - self.dry_run = dry_run self.cleanup = cleanup if mirrors: for uploader in self.uploaders.values(): @@ -472,7 +471,7 @@ class ImageUploadManager(BaseImageManager): uploader = self.uploader(uploader) tasks.append(UploadTask( image_name, pull_source, push_destination, - append_tag, modify_role, modify_vars, self.dry_run, + append_tag, modify_role, modify_vars, self.cleanup, multi_arch)) # NOTE(mwhahaha): We want to randomize the upload process because of @@ -1177,9 +1176,6 @@ class SkopeoImageUploader(BaseImageUploader): target_image_local_url = parse.urlparse('containers-storage:%s' % t.target_image) - if t.dry_run: - return [] - target_username, target_password = self.credentials_for_registry( t.target_image_url.netloc) target_session = self.authenticate( @@ -1486,9 +1482,6 @@ class PythonImageUploader(BaseImageUploader): source_local = t.source_image.startswith('containers-storage:') target_image_local_url = parse.urlparse('containers-storage:%s' % t.target_image) - if t.dry_run: - return [] - target_username, target_password = self.credentials_for_registry( t.target_image_url.netloc) try: @@ -2562,7 +2555,7 @@ class PythonImageUploader(BaseImageUploader): class UploadTask(object): def __init__(self, image_name, pull_source, push_destination, - append_tag, modify_role, modify_vars, dry_run, cleanup, + append_tag, modify_role, modify_vars, cleanup, multi_arch): self.image_name = image_name self.pull_source = pull_source @@ -2570,7 +2563,6 @@ class UploadTask(object): self.append_tag = append_tag or '' self.modify_role = modify_role self.modify_vars = modify_vars - self.dry_run = dry_run self.cleanup = cleanup self.multi_arch = multi_arch diff --git a/tripleo_common/image/kolla_builder.py b/tripleo_common/image/kolla_builder.py index c5efe39ee..3dc286e8b 100644 --- a/tripleo_common/image/kolla_builder.py +++ b/tripleo_common/image/kolla_builder.py @@ -222,14 +222,13 @@ def container_images_prepare_multi(environment, roles_data, dry_run=False, ) env_params.update(prepare_data['image_params']) - if push_destination or pull_source or modify_role: + if not dry_run and (push_destination or pull_source or modify_role): with tempfile.NamedTemporaryFile(mode='w') as f: yaml.safe_dump({ 'container_images': prepare_data['upload_data'] }, f) uploader = image_uploader.ImageUploadManager( [f.name], - dry_run=dry_run, cleanup=cleanup, mirrors=mirrors, registry_credentials=creds, diff --git a/tripleo_common/tests/image/test_image_uploader.py b/tripleo_common/tests/image/test_image_uploader.py index 76f23ceb3..5a3403c7f 100644 --- a/tripleo_common/tests/image/test_image_uploader.py +++ b/tripleo_common/tests/image/test_image_uploader.py @@ -430,7 +430,6 @@ class TestUploadTask(base.TestCase): append_tag='baz', modify_role=None, modify_vars=None, - dry_run=False, cleanup=False, multi_arch=False) self.assertEqual(obj.repo, 'docker.io/namespace/foo') @@ -449,7 +448,6 @@ class TestUploadTask(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup=False, multi_arch=False) self.assertEqual(obj.repo, 'docker.io/namespace/foo') @@ -462,7 +460,6 @@ class TestUploadTask(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup=False, multi_arch=False) self.assertEqual(obj.target_image_no_tag, @@ -1201,7 +1198,6 @@ class TestSkopeoImageUploader(base.TestCase): None, None, None, - False, 'full', False) ) @@ -1267,7 +1263,6 @@ class TestSkopeoImageUploader(base.TestCase): append_tag, 'add-foo-plugin', {'foo_version': '1.0.1'}, - False, 'partial', False) ) @@ -1331,7 +1326,7 @@ class TestSkopeoImageUploader(base.TestCase): self.uploader.upload_image, image_uploader.UploadTask( image + ':' + tag, None, push_destination, append_tag, 'add-foo-plugin', {'foo_version': '1.0.1'}, - False, 'full', False) + 'full', False) ) mock_copy.assert_called_once_with( @@ -1339,34 +1334,6 @@ class TestSkopeoImageUploader(base.TestCase): urlparse('containers-storage:docker.io/t/nova-api:latest') ) - @mock.patch('subprocess.Popen') - @mock.patch('tripleo_common.actions.' - 'ansible.AnsiblePlaybookAction', autospec=True) - def test_modify_upload_image_dry_run(self, mock_ansible, mock_popen): - mock_process = mock.Mock() - mock_popen.return_value = mock_process - - image = 'docker.io/t/nova-api' - tag = 'latest' - append_tag = 'modify-123' - push_destination = 'localhost:8787' - - result = self.uploader.upload_image(image_uploader.UploadTask( - image + ':' + tag, - None, - push_destination, - append_tag, - 'add-foo-plugin', - {'foo_version': '1.0.1'}, - True, - 'full', - False) - ) - - mock_ansible.assert_not_called() - mock_process.communicate.assert_not_called() - self.assertEqual([], result) - @mock.patch('tripleo_common.image.image_uploader.' 'BaseImageUploader.authenticate') @mock.patch('tripleo_common.image.image_uploader.' @@ -1389,7 +1356,6 @@ class TestSkopeoImageUploader(base.TestCase): append_tag, 'add-foo-plugin', {'foo_version': '1.0.1'}, - False, 'full', False) ) @@ -1503,7 +1469,6 @@ class TestPythonImageUploader(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup='full', multi_arch=False ) @@ -1600,7 +1565,6 @@ class TestPythonImageUploader(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup='full', multi_arch=False ) @@ -1669,7 +1633,6 @@ class TestPythonImageUploader(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup='full', multi_arch=False ) @@ -1737,7 +1700,6 @@ class TestPythonImageUploader(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup='full', multi_arch=False ) @@ -1844,7 +1806,6 @@ class TestPythonImageUploader(base.TestCase): append_tag=append_tag, modify_role='add-foo-plugin', modify_vars={'foo_version': '1.0.1'}, - dry_run=False, cleanup='full', multi_arch=False ) @@ -1962,7 +1923,6 @@ class TestPythonImageUploader(base.TestCase): append_tag=None, modify_role=None, modify_vars=None, - dry_run=False, cleanup='full', multi_arch=False ) diff --git a/tripleo_common/tests/image/test_kolla_builder.py b/tripleo_common/tests/image/test_kolla_builder.py index 13ee579fe..3959a0d95 100644 --- a/tripleo_common/tests/image/test_kolla_builder.py +++ b/tripleo_common/tests/image/test_kolla_builder.py @@ -955,9 +955,7 @@ class TestPrepare(base.TestCase): ) @mock.patch('tripleo_common.image.kolla_builder.container_images_prepare') - @mock.patch('tripleo_common.image.image_uploader.ImageUploadManager', - autospec=True) - def test_container_images_prepare_multi_dry_run(self, mock_im, mock_cip): + def test_container_images_prepare_multi_dry_run(self, mock_cip): mock_lock = mock.MagicMock() mapping_args = { 'namespace': 't', @@ -1049,11 +1047,6 @@ class TestPrepare(base.TestCase): lock=mock_lock ) ]) - - mock_im.assert_called_once_with(mock.ANY, dry_run=True, cleanup='full', - mirrors={}, registry_credentials=None, - multi_arch=False, lock=mock_lock) - self.assertEqual( { 'BarImage': 't/bar:1.0', @@ -1065,10 +1058,7 @@ class TestPrepare(base.TestCase): ) @mock.patch('tripleo_common.image.kolla_builder.container_images_prepare') - @mock.patch('tripleo_common.image.image_uploader.ImageUploadManager', - autospec=True) - def test_container_images_prepare_multi_tag_from_label(self, mock_im, - mock_cip): + def test_container_images_prepare_multi_tag_from_label(self, mock_cip): mock_lock = mock.MagicMock() mapping_args = { 'namespace': 't', @@ -1167,10 +1157,6 @@ class TestPrepare(base.TestCase): ) ]) - mock_im.assert_called_once_with(mock.ANY, dry_run=True, cleanup='full', - mirrors={}, registry_credentials=None, - multi_arch=False, lock=mock_lock) - self.assertEqual( { 'BarImage': 't/bar:1.0',