Write global metadata about extra files

This is basically collecting all individual extra_files.json and putting
their content into a single location in
compose/metadata/extra_files.json. The file format is part of productmd
1.23.

JIRA: COMPOSE-3831
Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

pungi/metadata.py

@@ -17,7 +17,6 @@
 import copy
 import os
 import time
-import json
 
 import productmd.composeinfo
 import productmd.treeinfo
@@ -346,43 +345,34 @@ def write_tree_info(compose, arch, variant, timestamp=None, bi=None):
     ti.dump(path)
 
 
-def write_extra_files(tree_path, files, checksum_type='sha256', logger=None):
+def populate_extra_files_metadata(
+    metadata, variant, arch, topdir, files, checksum_types, relative_root=None
+):
     """
-    Write the metadata for all extra files added to the compose.
+    :param metadata: an instance of productmd.extra_files.ExtraFiles to
-
+                     populate with the current files
-    :param tree_path:
+    :param Variant variant: under which variant should the files be listed
-        Root of the tree to write the ``extra_files.json`` metadata file for.
+    :param str arch: under which arch should the files be listed
-
+    :param topdir: directory where files are located
-    :param files:
+    :param files: list of file paths relative to topdir
-        A list of files that should be included in the metadata file. These
+    :param checksum_types: list of checksums to compute
-        should be paths that are relative to ``tree_path``.
+    :param relative_root: ancestor directory of topdir, this will be removed
-
+                          from paths written to local metadata file
-    :return:
-        Path to the metadata file written.
     """
-    metadata_path = os.path.join(tree_path, 'extra_files.json')
+    for copied_file in files:
-    if logger:
+        full_path = os.path.join(topdir, copied_file)
-        logger.info('Calculating content of {metadata}'.format(metadata=metadata_path))
+        size = os.path.getsize(full_path)
-    metadata = {'header': {'version': '1.0'}, 'data': []}
-    for f in files:
-        if logger:
-            logger.debug('Processing {file}'.format(file=f))
-        path = os.path.join(tree_path, f)
         try:
-            checksum = compute_file_checksums(path, checksum_type)
+            checksums = compute_file_checksums(full_path, checksum_types)
         except IOError as exc:
-            file = os.path.relpath(exc.filename, '/'.join(tree_path.split('/')[:-3]))
+            raise RuntimeError(
-            raise RuntimeError('Failed to calculate checksum for %s: %s' % (file, exc.strerror))
+                "Failed to calculate checksum for %s: %s" % (full_path, exc)
-        entry = {
+            )
-            'file': f,
-            'checksums': checksum,
-            'size': os.path.getsize(path),
-        }
-        metadata['data'].append(entry)
 
-    if logger:
+        if relative_root:
-        logger.info('Writing {metadata}'.format(metadata=metadata_path))
+            copied_file = os.path.relpath(full_path, relative_root)
+        metadata.add(variant.uid, arch, copied_file, size, checksums)
 
-    with open(metadata_path, 'w') as fd:
+    strip_prefix = (os.path.relpath(topdir, relative_root) + "/") if relative_root else ""
-        json.dump(metadata, fd, sort_keys=True, indent=4, separators=(',', ': '))
+    with open(os.path.join(topdir, "extra_files.json"), "w") as f:
-    return metadata_path
+        metadata.dump_for_tree(f, variant.uid, arch, strip_prefix)

pungi/phases/extra_files.py

@@ -18,11 +18,13 @@ import os
 import copy
 import fnmatch
 
+from productmd.extra_files import ExtraFiles
+
 from pungi.util import get_arch_variant_data, pkg_is_rpm, copy_all
 from pungi.arch import split_name_arch
-from pungi import metadata
 from pungi.wrappers.scm import get_file_from_scm, get_dir_from_scm
 from pungi.phases.base import ConfigGuardedPhase
+from pungi import metadata
 
 
 class ExtraFilesPhase(ConfigGuardedPhase):
@@ -33,6 +35,12 @@ class ExtraFilesPhase(ConfigGuardedPhase):
         super(ExtraFilesPhase, self).__init__(compose)
         # pkgset_phase provides package_sets
         self.pkgset_phase = pkgset_phase
+        # Prepare metadata
+        self.metadata = ExtraFiles()
+        self.metadata.compose.id = self.compose.compose_id
+        self.metadata.compose.type = self.compose.compose_type
+        self.metadata.compose.date = self.compose.compose_date
+        self.metadata.compose.respin = self.compose.compose_respin
 
     def run(self):
         for variant in self.compose.get_variants():
@@ -41,13 +49,26 @@ class ExtraFilesPhase(ConfigGuardedPhase):
             for arch in variant.arches + ["src"]:
                 cfg = get_arch_variant_data(self.compose.conf, self.name, arch, variant)
                 if cfg:
-                    copy_extra_files(self.compose, cfg, arch, variant, self.pkgset_phase.package_sets)
+                    copy_extra_files(
+                        self.compose,
+                        cfg,
+                        arch,
+                        variant,
+                        self.pkgset_phase.package_sets,
+                        self.metadata,
+                    )
                 else:
                     self.compose.log_info('[SKIP ] No extra files (arch: %s, variant: %s)'
                                           % (arch, variant.uid))
 
+        metadata_path = self.compose.paths.compose.metadata("extra_files.json")
+        self.compose.log_info("Writing global extra files metadata: %s" % metadata_path)
+        self.metadata.dump(metadata_path)
 
-def copy_extra_files(compose, cfg, arch, variant, package_sets, checksum_type=None):
+
+def copy_extra_files(
+    compose, cfg, arch, variant, package_sets, extra_metadata, checksum_type=None
+):
     checksum_type = checksum_type or compose.conf['media_checksums']
     var_dict = {
         "arch": arch,
@@ -85,8 +106,15 @@ def copy_extra_files(compose, cfg, arch, variant, package_sets, checksum_type=No
         getter(scm_dict, target_path, compose=compose)
 
     if os.listdir(extra_files_dir):
-        files_copied = copy_all(extra_files_dir, os_tree)
+        metadata.populate_extra_files_metadata(
-        metadata.write_extra_files(os_tree, files_copied, checksum_type, compose._logger)
+            extra_metadata,
+            variant,
+            arch,
+            os_tree,
+            copy_all(extra_files_dir, os_tree),
+            compose.conf["media_checksums"],
+            relative_root=compose.paths.compose.topdir(),
+        )
 
     compose.log_info("[DONE ] %s" % msg)
 

pungi/phases/extra_isos.py

@@ -18,6 +18,7 @@ import os
 from kobo.shortcuts import force_list
 from kobo.threads import ThreadPool, WorkerThread
 import productmd.treeinfo
+from productmd.extra_files import ExtraFiles
 
 from pungi import createiso
 from pungi import metadata
@@ -155,11 +156,13 @@ def get_extra_files(compose, variant, arch, extra_files):
         )
 
     if filelist:
-        metadata.write_extra_files(
+        metadata.populate_extra_files_metadata(
+            ExtraFiles(),
+            variant,
+            arch,
             extra_files_dir,
             filelist,
             compose.conf["media_checksums"],
-            logger=compose._logger,
         )
 
 

tests/test_extra_files_phase.py

@@ -4,6 +4,8 @@ import mock
 import os
 import sys
 
+from productmd.extra_files import ExtraFiles
+
 import six
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
@@ -38,23 +40,44 @@ class TestExtraFilePhase(helpers.PungiTestCase):
         six.assertCountEqual(
             self,
             copy_extra_files.call_args_list,
-            [mock.call(compose, [cfg], 'x86_64', compose.variants['Server'],
+            [
-                       pkgset_phase.package_sets),
+                mock.call(
-             mock.call(compose, [cfg], 'x86_64', compose.variants['Everything'],
+                    compose,
-                       pkgset_phase.package_sets)]
+                    [cfg],
+                    "x86_64",
+                    compose.variants["Server"],
+                    pkgset_phase.package_sets,
+                    phase.metadata,
+                ),
+                mock.call(
+                    compose,
+                    [cfg],
+                    "x86_64",
+                    compose.variants["Everything"],
+                    pkgset_phase.package_sets,
+                    phase.metadata,
+                ),
+            ],
         )
+        self.assertTrue(isinstance(phase.metadata, ExtraFiles))
 
 
 class TestCopyFiles(helpers.PungiTestCase):
 
+    def setUp(self):
+        super(TestCopyFiles, self).setUp()
+        self.metadata = ExtraFiles()
+        self.compose = helpers.DummyCompose(self.topdir, {})
+        self.variant = self.compose.variants["Server"]
+
     def test_copy_local_file(self):
         tgt = os.path.join(self.topdir, 'file')
         helpers.touch(tgt)
-        compose = helpers.DummyCompose(self.topdir, {})
         cfg = {'scm': 'file', 'file': tgt, 'repo': None}
 
-        extra_files.copy_extra_files(compose, [cfg], 'x86_64',
+        extra_files.copy_extra_files(
-                                     compose.variants['Server'], mock.Mock())
+            self.compose, [cfg], "x86_64", self.variant, mock.Mock(), self.metadata
+        )
 
         self.assertTrue(os.path.isfile(os.path.join(
             self.topdir, 'compose', 'Server', 'x86_64', 'os', 'file')))
@@ -64,12 +87,17 @@ class TestCopyFiles(helpers.PungiTestCase):
         tgt2 = os.path.join(self.topdir, 'gpl')
         helpers.touch(tgt1)
         helpers.touch(tgt2)
-        compose = helpers.DummyCompose(self.topdir, {})
         cfg1 = {'scm': 'file', 'file': tgt1, 'repo': None}
         cfg2 = {'scm': 'file', 'file': tgt2, 'repo': None, 'target': 'license'}
 
-        extra_files.copy_extra_files(compose, [cfg1, cfg2], 'x86_64',
+        extra_files.copy_extra_files(
-                                     compose.variants['Server'], mock.Mock())
+            self.compose,
+            [cfg1, cfg2],
+            "x86_64",
+            self.variant,
+            mock.Mock(),
+            self.metadata
+        )
 
         self.assertTrue(os.path.isfile(os.path.join(
             self.topdir, 'compose', 'Server', 'x86_64', 'os', 'file')))
@@ -79,11 +107,11 @@ class TestCopyFiles(helpers.PungiTestCase):
     def test_copy_local_dir(self):
         helpers.touch(os.path.join(self.topdir, 'src', 'file'))
         helpers.touch(os.path.join(self.topdir, 'src', 'another'))
-        compose = helpers.DummyCompose(self.topdir, {})
         cfg = {'scm': 'file', 'dir': os.path.join(self.topdir, 'src'),
                'repo': None, 'target': 'subdir'}
-        extra_files.copy_extra_files(compose, [cfg], 'x86_64',
+        extra_files.copy_extra_files(
-                                     compose.variants['Server'], mock.Mock())
+            self.compose, [cfg], "x86_64", self.variant, mock.Mock(), self.metadata
+        )
 
         self.assertTrue(os.path.isfile(os.path.join(
             self.topdir, 'compose', 'Server', 'x86_64', 'os', 'subdir', 'file')))
@@ -93,13 +121,13 @@ class TestCopyFiles(helpers.PungiTestCase):
     @mock.patch('pungi.phases.extra_files.get_file_from_scm')
     @mock.patch('pungi.phases.extra_files.get_dir_from_scm')
     def test_copy_from_external_rpm(self, get_dir_from_scm, get_file_from_scm):
-        compose = helpers.DummyCompose(self.topdir, {})
         cfg = {'scm': 'rpm', 'file': 'file.txt', 'repo': 'http://example.com/package.rpm'}
 
         get_file_from_scm.side_effect = self.fake_get_file
 
-        extra_files.copy_extra_files(compose, [cfg], 'x86_64',
+        extra_files.copy_extra_files(
-                                     compose.variants['Server'], mock.Mock())
+            self.compose, [cfg], "x86_64", self.variant, mock.Mock(), self.metadata
+        )
 
         self.assertEqual(len(get_file_from_scm.call_args_list), 1)
         self.assertEqual(get_dir_from_scm.call_args_list, [])
@@ -112,7 +140,6 @@ class TestCopyFiles(helpers.PungiTestCase):
     @mock.patch('pungi.phases.extra_files.get_file_from_scm')
     @mock.patch('pungi.phases.extra_files.get_dir_from_scm')
     def test_copy_from_rpm_in_compose(self, get_dir_from_scm, get_file_from_scm):
-        compose = helpers.DummyCompose(self.topdir, {})
         cfg = {'scm': 'rpm', 'file': 'file.txt', 'repo': '%(variant_uid)s-data*'}
         server_po, client_po, src_po = mock.Mock(), mock.Mock(), mock.Mock()
         server_po.configure_mock(name='Server-data-1.1-1.fc24.x86_64.rpm',
@@ -136,8 +163,9 @@ class TestCopyFiles(helpers.PungiTestCase):
 
         get_file_from_scm.side_effect = self.fake_get_file
 
-        extra_files.copy_extra_files(compose, [cfg], 'x86_64',
+        extra_files.copy_extra_files(
-                                     compose.variants['Server'], package_sets)
+            self.compose, [cfg], "x86_64", self.variant, package_sets, self.metadata
+        )
 
         self.assertEqual(len(get_file_from_scm.call_args_list), 1)
         self.assertEqual(get_dir_from_scm.call_args_list, [])
@@ -156,13 +184,13 @@ class TestCopyFiles(helpers.PungiTestCase):
     @mock.patch('pungi.phases.extra_files.get_file_from_scm')
     @mock.patch('pungi.phases.extra_files.get_dir_from_scm')
     def test_copy_from_non_existing_rpm_in_compose(self, get_dir_from_scm, get_file_from_scm):
-        compose = helpers.DummyCompose(self.topdir, {})
         cfg = {'scm': 'rpm', 'file': 'file.txt', 'repo': 'bad-%(variant_uid_lower)s*'}
         package_sets = [{"x86_64": {}}]
 
         with self.assertRaises(RuntimeError) as ctx:
             extra_files.copy_extra_files(
-                compose, [cfg], 'x86_64', compose.variants['Server'], package_sets)
+                self.compose, [cfg], "x86_64", self.variant, package_sets, self.metadata
+            )
 
         self.assertRegexpMatches(
             str(ctx.exception), r'No.*package.*matching bad-server\*.*'

tests/test_extra_isos_phase.py

@@ -424,7 +424,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase):
         self.assertEqual(aitm.call_args_list, [])
 
 
-@mock.patch("pungi.metadata.write_extra_files")
+@mock.patch("pungi.metadata.populate_extra_files_metadata")
 @mock.patch('pungi.phases.extra_isos.get_file_from_scm')
 @mock.patch('pungi.phases.extra_isos.get_dir_from_scm')
 class GetExtraFilesTest(helpers.PungiTestCase):
@@ -438,14 +438,14 @@ class GetExtraFilesTest(helpers.PungiTestCase):
             self.topdir, "work", self.arch, self.variant.uid, "extra-iso-extra-files"
         )
 
-    def test_no_config(self, get_dir, get_file, write_extra):
+    def test_no_config(self, get_dir, get_file, populate_md):
         extra_isos.get_extra_files(self.compose, self.variant, self.arch, [])
 
         self.assertEqual(get_dir.call_args_list, [])
         self.assertEqual(get_file.call_args_list, [])
-        self.assertEqual(write_extra.call_args_list, [])
+        self.assertEqual(populate_md.call_args_list, [])
 
-    def test_get_file(self, get_dir, get_file, write_extra):
+    def test_get_file(self, get_dir, get_file, populate_md):
         get_file.return_value = ["GPL"]
         cfg = {
             'scm': 'git',
@@ -465,18 +465,20 @@ class GetExtraFilesTest(helpers.PungiTestCase):
             ],
         )
         self.assertEqual(
-            write_extra.call_args_list,
+            populate_md.call_args_list,
             [
                 mock.call(
+                    mock.ANY,
+                    self.variant,
+                    self.arch,
                     self.dir,
                     ["legalese/GPL"],
                     self.compose.conf["media_checksums"],
-                    logger=self.compose._logger,
                 )
             ],
         )
 
-    def test_get_dir(self, get_dir, get_file, write_extra):
+    def test_get_dir(self, get_dir, get_file, populate_md):
         get_dir.return_value = ["a", "b"]
         cfg = {
             'scm': 'git',
@@ -489,25 +491,23 @@ class GetExtraFilesTest(helpers.PungiTestCase):
         self.assertEqual(get_file.call_args_list, [])
         self.assertEqual(
             get_dir.call_args_list,
-            [
+            [mock.call(cfg, os.path.join(self.dir, "foo"), compose=self.compose)],
-                mock.call(
-                    cfg, os.path.join(self.dir, "foo"), compose=self.compose
-                )
-            ],
         )
         self.assertEqual(
-            write_extra.call_args_list,
+            populate_md.call_args_list,
             [
                 mock.call(
+                    mock.ANY,
+                    self.variant,
+                    self.arch,
                     self.dir,
                     ["foo/a", "foo/b"],
                     self.compose.conf["media_checksums"],
-                    logger=self.compose._logger,
+                ),
-                )
             ],
         )
 
-    def test_get_multiple_files(self, get_dir, get_file, write_extra):
+    def test_get_multiple_files(self, get_dir, get_file, populate_md):
         get_file.side_effect = [["GPL"], ["setup.py"]]
         cfg1 = {
             'scm': 'git',
@@ -531,14 +531,16 @@ class GetExtraFilesTest(helpers.PungiTestCase):
             ],
         )
         self.assertEqual(
-            write_extra.call_args_list,
+            populate_md.call_args_list,
             [
                 mock.call(
+                    mock.ANY,
+                    self.variant,
+                    self.arch,
                     self.dir,
                     ["legalese/GPL", "setup.py"],
                     self.compose.conf["media_checksums"],
-                    logger=self.compose._logger,
+                ),
-                )
             ],
         )
 

tests/test_metadata.py

@@ -1,4 +1,3 @@
-import json
 import mock
 import os
 import sys
@@ -153,101 +152,67 @@ class MediaRepoTestCase(helpers.PungiTestCase):
         self.assertFalse(os.path.isfile(self.path))
 
 
-class TestWriteExtraFiles(helpers.PungiTestCase):
+FOO_MD5 = {"md5": "acbd18db4cc2f85cedef654fccc4a4d8"}
+BAR_MD5 = {"md5": "37b51d194a7513e45b56f6524f2d51f2"}
+
+
+class TestPopulateExtraFiles(helpers.PungiTestCase):
 
     def setUp(self):
-        super(TestWriteExtraFiles, self).setUp()
+        super(TestPopulateExtraFiles, self).setUp()
-        self.compose = helpers.DummyCompose(self.topdir, {})
+        self.variant = mock.Mock(uid="Server")
+        self.metadata = mock.Mock()
 
-    def test_write_extra_files(self):
+    def test_with_relative_root(self):
-        """Assert metadata is written to the proper location with valid data"""
+        helpers.touch(
-        mock_logger = mock.Mock()
+            os.path.join(self.topdir, "compose/Server/x86_64/os/foo"), content="foo"
-        files = ['file1', 'file2', 'subdir/file3']
+        )
-        expected_metadata = {
+        helpers.touch(
-            u'header': {u'version': u'1.0'},
+            os.path.join(self.topdir, "compose/Server/x86_64/os/bar"), content="bar"
-            u'data': [
+        )
-                {
-                    u'file': u'file1',
-                    u'checksums': {u'sha256': u'ecdc5536f73bdae8816f0ea40726ef5e9b810d914493075903bb90623d97b1d8'},
-                    u'size': 6,
-                },
-                {
-                    u'file': u'file2',
-                    u'checksums': {u'sha256': u'67ee5478eaadb034ba59944eb977797b49ca6aa8d3574587f36ebcbeeb65f70e'},
-                    u'size': 6,
-                },
-                {
-                    u'file': u'subdir/file3',
-                    u'checksums': {u'sha256': u'52f9f0e467e33da811330cad085fdb4eaa7abcb9ebfe6001e0f5910da678be51'},
-                    u'size': 13,
-                },
-            ]
-        }
-        tree_dir = os.path.join(self.topdir, 'compose', 'Server', 'x86_64', 'os')
-        for f in files:
-            helpers.touch(os.path.join(tree_dir, f), f + '\n')
 
-        metadata_file = metadata.write_extra_files(tree_dir, files, logger=mock_logger)
+        metadata.populate_extra_files_metadata(
-        with open(metadata_file) as metadata_fd:
+            self.metadata,
-            actual_metadata = json.load(metadata_fd)
+            self.variant,
+            "x86_64",
+            os.path.join(self.topdir, "compose/Server/x86_64/os"),
+            ["foo", "bar"],
+            ["md5"],
+            relative_root=os.path.join(self.topdir, "compose"),
+        )
 
-        self.assertEqual(expected_metadata['header'], actual_metadata['header'])
-        self.assertEqual(expected_metadata['data'], actual_metadata['data'])
-
-    def test_write_extra_files_multiple_checksums(self):
-        """Assert metadata is written to the proper location with valid data"""
         self.maxDiff = None
-        mock_logger = mock.Mock()
-        files = ['file1', 'file2', 'subdir/file3']
-        expected_metadata = {
-            u'header': {u'version': u'1.0'},
-            u'data': [
-                {
-                    u'file': u'file1',
-                    u'checksums': {
-                        u'md5': u'5149d403009a139c7e085405ef762e1a',
-                        u'sha256': u'ecdc5536f73bdae8816f0ea40726ef5e9b810d914493075903bb90623d97b1d8'
-                    },
-                    u'size': 6,
-                },
-                {
-                    u'file': u'file2',
-                    u'checksums': {
-                        u'md5': u'3d709e89c8ce201e3c928eb917989aef',
-                        u'sha256': u'67ee5478eaadb034ba59944eb977797b49ca6aa8d3574587f36ebcbeeb65f70e'
-                    },
-                    u'size': 6,
-                },
-                {
-                    u'file': u'subdir/file3',
-                    u'checksums': {
-                        u'md5': u'1ed02b5cf7fd8626f854e9ef3fee8694',
-                        u'sha256': u'52f9f0e467e33da811330cad085fdb4eaa7abcb9ebfe6001e0f5910da678be51'
-                    },
-                    u'size': 13,
-                },
-            ]
-        }
-        tree_dir = os.path.join(self.topdir, 'compose', 'Server', 'x86_64', 'os')
-        for f in files:
-            helpers.touch(os.path.join(tree_dir, f), f + '\n')
 
-        metadata_file = metadata.write_extra_files(tree_dir, files,
+        six.assertCountEqual(
-                                                   checksum_type=['md5', 'sha256'],
+            self,
-                                                   logger=mock_logger)
+            self.metadata.mock_calls,
-        with open(metadata_file) as metadata_fd:
+            [
-            actual_metadata = json.load(metadata_fd)
+                mock.call.add(
+                    "Server", "x86_64", "Server/x86_64/os/foo", 3, FOO_MD5
+                ),
+                mock.call.add(
+                    "Server", "x86_64", "Server/x86_64/os/bar", 3, BAR_MD5
+                ),
+                mock.call.dump_for_tree(
+                    mock.ANY, "Server", "x86_64", "Server/x86_64/os/"
+                ),
+            ],
+        )
 
-        self.assertEqual(expected_metadata['header'], actual_metadata['header'])
+    def test_without_relative_root(self):
-        self.assertEqual(expected_metadata['data'], actual_metadata['data'])
+        helpers.touch(os.path.join(self.topdir, "foo"), content="foo")
+        helpers.touch(os.path.join(self.topdir, "bar"), content="bar")
 
-    def test_write_extra_files_missing_file(self):
+        metadata.populate_extra_files_metadata(
-        """Assert metadata is written to the proper location with valid data"""
+            self.metadata, self.variant, "x86_64", self.topdir, ["foo", "bar"], ["md5"]
-        mock_logger = mock.Mock()
+        )
-        files = ['file1', 'file2', 'subdir/file3']
-        tree_dir = os.path.join(self.topdir, 'compose', 'Server', 'x86_64', 'os')
-        for f in files:
-            helpers.touch(os.path.join(tree_dir, f), f + '\n')
-        files.append('missing_file')
 
-        self.assertRaises(RuntimeError, metadata.write_extra_files, tree_dir, files, 'sha256', mock_logger)
+        six.assertCountEqual(
+            self,
+            self.metadata.mock_calls,
+            [
+                mock.call.add("Server", "x86_64", "foo", 3, FOO_MD5),
+                mock.call.add("Server", "x86_64", "bar", 3, BAR_MD5),
+                mock.call.dump_for_tree(mock.ANY, "Server", "x86_64", ""),
+            ],
+        )