From 85bf5535bcc4ffdbd67fc7d060ec4ab5c4041eb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Fri, 2 Nov 2018 12:43:56 +0100 Subject: [PATCH] init: Keep parent groups in addon comps environments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The environment in comps for a variant can refer to groups in parent variant (either for addons, or because of other configuration). We should not remove the groups in this case. This requires changes in two places: * teaching `comps_filter` about groups that should not be removed * fixing writing comps so that it does not actually change the data as well JIRA: COMPOSE-2612 Signed-off-by: Lubomír Sedlář --- bin/comps_filter | 9 +++- pungi/phases/init.py | 29 +++++++++++-- pungi/wrappers/comps.py | 13 +++--- tests/data/dummy-comps.xml | 11 +++++ tests/data/dummy-variants.xml | 3 ++ tests/fixtures/comps-formatted.xml | 9 ++++ tests/fixtures/comps-group.xml | 9 ++++ tests/test_initphase.py | 69 ++++++++++++++++++++++++++++++ 8 files changed, 142 insertions(+), 10 deletions(-) diff --git a/bin/comps_filter b/bin/comps_filter index 4b6c5ccf..f2e6f1e7 100755 --- a/bin/comps_filter +++ b/bin/comps_filter @@ -29,6 +29,13 @@ def main(): help="remove all environment sections") parser.add_argument("--keep-empty-group", default=[], action="append", metavar="GROUPID", help="keep groups even if they are empty") + parser.add_argument( + "--lookaside-group", + default=[], + action="append", + metavar="GROUPID", + help="keep this group in environments even if they are not defined in the comps", + ) parser.add_argument("--no-cleanup", default=False, action="store_true", help="don't remove empty groups and categories") parser.add_argument("--no-reindent", default=False, action="store_true", @@ -46,7 +53,7 @@ def main(): f.filter_environments(opts.arch, opts.variant, opts.arch_only_environments) if not opts.no_cleanup: - f.cleanup(opts.keep_empty_group) + f.cleanup(opts.keep_empty_group, opts.lookaside_group) if opts.remove_categories: f.remove_categories() diff --git a/pungi/phases/init.py b/pungi/phases/init.py index 00cdc281..35e2ebb9 100644 --- a/pungi/phases/init.py +++ b/pungi/phases/init.py @@ -107,6 +107,19 @@ def write_arch_comps(compose, arch): UNMATCHED_GROUP_MSG = 'Variant %s.%s requires comps group %s which does not match anything in input comps file' +def get_lookaside_groups(compose, variant): + """Find all groups listed in parent variant.""" + groups = set() + if variant.parent: + groups.update(g["name"] for g in variant.parent.groups) + + for var, lookaside in compose.conf.get("variant_as_lookaside", []): + if var == variant.uid: + lookaside_variant = compose.all_variants[lookaside] + groups.update(g["name"] for g in lookaside_variant.groups) + return groups + + def write_variant_comps(compose, arch, variant): comps_file = compose.paths.work.comps(arch=arch, variant=variant) msg = "Writing comps file (arch: %s, variant: %s): %s" % (arch, variant, comps_file) @@ -123,10 +136,18 @@ def write_variant_comps(compose, arch, variant): return compose.log_debug(msg) - run(["comps_filter", "--arch=%s" % arch, "--keep-empty-group=conflicts", - "--keep-empty-group=conflicts-%s" % variant.uid.lower(), - "--variant=%s" % variant.uid, - "--output=%s" % comps_file, compose.paths.work.comps(arch="global")]) + cmd = [ + "comps_filter", + "--arch=%s" % arch, + "--keep-empty-group=conflicts", + "--keep-empty-group=conflicts-%s" % variant.uid.lower(), + "--variant=%s" % variant.uid, + "--output=%s" % comps_file, + compose.paths.work.comps(arch="global") + ] + for group in get_lookaside_groups(compose, variant): + cmd.append("--lookaside-group=%s" % group) + run(cmd) comps = CompsWrapper(comps_file) if variant.groups or variant.modules is not None or variant.type != 'variant': diff --git a/pungi/wrappers/comps.py b/pungi/wrappers/comps.py index 2ace7b21..3c26a566 100644 --- a/pungi/wrappers/comps.py +++ b/pungi/wrappers/comps.py @@ -169,11 +169,11 @@ class CompsFilter(object): for i in self.tree.xpath("//*[@xml:lang]"): i.getparent().remove(i) - def filter_environment_groups(self): + def filter_environment_groups(self, lookaside_groups=[]): """ Remove undefined groups from environments. """ - all_groups = self.tree.xpath("/comps/group/id/text()") + all_groups = self.tree.xpath("/comps/group/id/text()") + lookaside_groups for environment in self.tree.xpath("/comps/environment"): for group in environment.xpath("grouplist/groupid"): if group.text not in all_groups: @@ -199,15 +199,18 @@ class CompsFilter(object): self.tree.write(file_obj, pretty_print=self.reindent, xml_declaration=True, encoding=self.encoding) file_obj.write(b"\n") - def cleanup(self, keep_groups=[]): + def cleanup(self, keep_groups=[], lookaside_groups=[]): """ Remove empty groups, categories and environment from the comps file. Groups given in ``keep_groups`` will be preserved even if empty. + Lookaside groups are groups that are available in parent variant and + can be referenced in environments even if they are not directly defined + in the same comps file. """ self.remove_empty_groups(keep_groups) self.filter_category_groups() self.remove_empty_categories() - self.filter_environment_groups() + self.filter_environment_groups(lookaside_groups) self.remove_empty_environments() @@ -319,7 +322,7 @@ class CompsWrapper(object): append_grouplist(doc, cat_node, groups) for environment in sorted(self.comps.environments, key=attrgetter('id')): - groups = set(x.name for x in environment.group_ids) & set(self.get_comps_groups()) + groups = set(x.name for x in environment.group_ids) if not groups: continue env_node = doc.createElement("environment") diff --git a/tests/data/dummy-comps.xml b/tests/data/dummy-comps.xml index 92e99086..72ed7738 100644 --- a/tests/data/dummy-comps.xml +++ b/tests/data/dummy-comps.xml @@ -135,6 +135,17 @@ + + foobar + Foo Bar + Referencing a group from parent variant + 10 + + resilient-storage + text-internet + + + diff --git a/tests/data/dummy-variants.xml b/tests/data/dummy-variants.xml index ad485647..f8427369 100644 --- a/tests/data/dummy-variants.xml +++ b/tests/data/dummy-variants.xml @@ -9,6 +9,9 @@ resilient-storage + + foobar + diff --git a/tests/fixtures/comps-formatted.xml b/tests/fixtures/comps-formatted.xml index 58c60483..2ad7e255 100644 --- a/tests/fixtures/comps-formatted.xml +++ b/tests/fixtures/comps-formatted.xml @@ -99,6 +99,15 @@ text-internet + + empty + Empty + Should not appear in the repos. + 10 + + does-not-exist + + minimal Minimal install diff --git a/tests/fixtures/comps-group.xml b/tests/fixtures/comps-group.xml index f2985afa..d3fa8e85 100644 --- a/tests/fixtures/comps-group.xml +++ b/tests/fixtures/comps-group.xml @@ -65,6 +65,15 @@ text-internet + + empty + Empty + Should not appear in the repos. + 10 + + does-not-exist + + minimal Minimal install diff --git a/tests/test_initphase.py b/tests/test_initphase.py index 8712af3f..250c3ed3 100644 --- a/tests/test_initphase.py +++ b/tests/test_initphase.py @@ -293,6 +293,48 @@ class TestWriteVariantComps(PungiTestCase): [mock.call(variant.environments)]) self.assertEqual(comps.write_comps.mock_calls, [mock.call()]) + @mock.patch("pungi.phases.init.get_lookaside_groups") + @mock.patch("pungi.phases.init.run") + @mock.patch("pungi.phases.init.CompsWrapper") + def test_run_with_lookaside_groups(self, CompsWrapper, run, glg): + compose = DummyCompose(self.topdir, {}) + variant = compose.variants["Server"] + comps = CompsWrapper.return_value + comps.filter_groups.return_value = [] + glg.return_value = ["foo", "bar"] + + init.write_variant_comps(compose, "x86_64", variant) + + self.assertEqual( + run.mock_calls, + [ + mock.call( + [ + "comps_filter", + "--arch=x86_64", + "--keep-empty-group=conflicts", + "--keep-empty-group=conflicts-server", + "--variant=Server", + "--output=%s/work/x86_64/comps/comps-Server.x86_64.xml" % self.topdir, + self.topdir + "/work/global/comps/comps-global.xml", + "--lookaside-group=foo", + "--lookaside-group=bar", + ] + ), + ], + ) + self.assertEqual( + CompsWrapper.call_args_list, + [mock.call(self.topdir + "/work/x86_64/comps/comps-Server.x86_64.xml")], + ) + self.assertEqual( + comps.filter_groups.call_args_list, [mock.call(variant.groups)] + ) + self.assertEqual( + comps.filter_environments.mock_calls, [mock.call(variant.environments)] + ) + self.assertEqual(comps.write_comps.mock_calls, [mock.call()]) + @mock.patch('pungi.phases.init.run') @mock.patch('pungi.phases.init.CompsWrapper') def test_run_no_filter_without_groups(self, CompsWrapper, run): @@ -389,6 +431,33 @@ class TestWriteVariantComps(PungiTestCase): self.assertEqual(comps.write_comps.mock_calls, []) +class TestGetLookasideGroups(PungiTestCase): + def test_toplevel_variant(self): + compose = DummyCompose(self.topdir, {}) + self.assertItemsEqual( + init.get_lookaside_groups(compose, compose.variants["Server"]), [] + ) + + def test_classic_addon(self): + compose = DummyCompose(self.topdir, {}) + compose.setup_addon() + compose.variants["Server"].groups = [{"name": "foo"}] + self.assertItemsEqual( + init.get_lookaside_groups(compose, compose.all_variants["Server-HA"]), + ["foo"], + ) + + def test_variant_as_lookaside(self): + compose = DummyCompose( + self.topdir, {"variant_as_lookaside": [("Server", "Client")]} + ) + compose.variants["Client"].groups = [{"name": "foo"}] + self.assertItemsEqual( + init.get_lookaside_groups(compose, compose.variants["Server"]), + ["foo"], + ) + + @mock.patch("shutil.copytree") @mock.patch("pungi.phases.init.get_dir_from_scm") class TestWriteModuleDefaults(PungiTestCase):