init: Keep parent groups in addon comps environments

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ář <lsedlar@redhat.com>

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()

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",
+    cmd = [
-         "--keep-empty-group=conflicts-%s" % variant.uid.lower(),
+        "comps_filter",
-         "--variant=%s" % variant.uid,
+        "--arch=%s" % arch,
-         "--output=%s" % comps_file, compose.paths.work.comps(arch="global")])
+        "--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':

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")

tests/data/dummy-comps.xml

@@ -135,6 +135,17 @@
     </grouplist>
   </environment>
 
+  <environment>
+    <id>foobar</id>
+    <name>Foo Bar</name>
+    <description>Referencing a group from parent variant</description>
+    <display_order>10</display_order>
+    <grouplist>
+      <groupid>resilient-storage</groupid>
+      <groupid>text-internet</groupid>
+    </grouplist>
+  </environment>
+
   <!-- LANGPACKS -->
 
   <langpacks>

tests/data/dummy-variants.xml

@@ -9,6 +9,9 @@
     <groups>
       <group default="true">resilient-storage</group>
     </groups>
+    <environments>
+      <environment>foobar</environment>
+    </environments>
   </variant>
 
   <variant id="Live" name="Live" type="variant" is_empty="true">

tests/fixtures/comps-formatted.xml

@@ -99,6 +99,15 @@
       <groupid>text-internet</groupid>
     </optionlist>
   </environment>
+  <environment>
+    <id>empty</id>
+    <name>Empty</name>
+    <description>Should not appear in the repos.</description>
+    <display_order>10</display_order>
+    <grouplist>
+      <groupid>does-not-exist</groupid>
+    </grouplist>
+  </environment>
   <environment>
     <id>minimal</id>
     <name>Minimal install</name>

tests/fixtures/comps-group.xml

@@ -65,6 +65,15 @@
       <groupid>text-internet</groupid>
     </optionlist>
   </environment>
+  <environment>
+    <id>empty</id>
+    <name>Empty</name>
+    <description>Should not appear in the repos.</description>
+    <display_order>10</display_order>
+    <grouplist>
+      <groupid>does-not-exist</groupid>
+    </grouplist>
+  </environment>
   <environment>
     <id>minimal</id>
     <name>Minimal install</name>

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):