remove PREFERRED capabilities

Way back when I added support for core boot classic installs, we did not
have a way to indicate whether the installed model preferred to be
installed encrypted or unencrypted. Roll time on 9 months or so and our
API now supports a list of capabilites and we can define that we can
indicate which is preferred by putting it first in the list.

One part of the  implementation is a bit delicate really -- relying on
list.sort() being stable -- so maybe we should clean that up. But I
think the other changes make everything nicer.

subiquity/common/types.py

@@ -350,13 +350,12 @@ class GuidedCapability(enum.Enum):
 
     CORE_BOOT_ENCRYPTED = enum.auto()
     CORE_BOOT_UNENCRYPTED = enum.auto()
-    # These two are not valid as GuidedChoiceV2.capability:
-    CORE_BOOT_PREFER_ENCRYPTED = enum.auto()
-    CORE_BOOT_PREFER_UNENCRYPTED = enum.auto()
 
     DD = enum.auto()
 
     def __lt__(self, other) -> bool:
+        if self.is_core_boot() and other.is_core_boot():
+            return False
         return self.value < other.value
 
     def is_lvm(self) -> bool:
@@ -366,8 +365,6 @@ def is_core_boot(self) -> bool:
         return self in [
             GuidedCapability.CORE_BOOT_ENCRYPTED,
             GuidedCapability.CORE_BOOT_UNENCRYPTED,
-            GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED,
-            GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED,
         ]
 
     def supports_manual_customization(self) -> bool:

subiquity/server/controllers/filesystem.py

@@ -394,11 +394,13 @@ def disallowed_encryption(msg):
                 info.capability_info.allowed = [GuidedCapability.CORE_BOOT_ENCRYPTED]
             elif se.storage_safety == StorageSafety.PREFER_ENCRYPTED:
                 info.capability_info.allowed = [
-                    GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED
+                    GuidedCapability.CORE_BOOT_ENCRYPTED,
+                    GuidedCapability.CORE_BOOT_UNENCRYPTED,
                 ]
             elif se.storage_safety == StorageSafety.PREFER_UNENCRYPTED:
                 info.capability_info.allowed = [
-                    GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED
+                    GuidedCapability.CORE_BOOT_UNENCRYPTED,
+                    GuidedCapability.CORE_BOOT_ENCRYPTED,
                 ]
 
         return info
@@ -651,26 +653,8 @@ def start_guided_resize(
 
     def set_info_for_capability(self, capability: GuidedCapability):
         """Given a request for a capability, select the variation to use."""
-        if capability == GuidedCapability.CORE_BOOT_ENCRYPTED:
-            # If the request is for encryption, a variation with any
-            # of these capabilities is OK:
-            caps = {
-                GuidedCapability.CORE_BOOT_ENCRYPTED,
-                GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED,
-                GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED,
-            }
-        elif capability == GuidedCapability.CORE_BOOT_UNENCRYPTED:
-            # Similar if the request is for uncrypted
-            caps = {
-                GuidedCapability.CORE_BOOT_UNENCRYPTED,
-                GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED,
-                GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED,
-            }
-        else:
-            # Otherwise, just look for what we were asked for.
-            caps = {capability}
         for info in self._variation_info.values():
-            if caps & set(info.capability_info.allowed):
+            if capability in info.capability_info.allowed:
                 self._info = info
                 return
         raise Exception("could not find variation for {}".format(capability))
@@ -1313,12 +1297,12 @@ async def run_autoinstall_guided(self, layout):
         if name == "hybrid":
             # this check is conceptually unnecessary but results in a
             # much cleaner error message...
-            core_boot_caps = set()
+            core_boot_caps = []
             for variation in self._variation_info.values():
                 if not variation.is_valid():
                     continue
                 if variation.is_core_boot_classic():
-                    core_boot_caps.update(variation.capability_info.allowed)
+                    core_boot_caps.extend(variation.capability_info.allowed)
             if not core_boot_caps:
                 raise Exception(
                     "can only use name: hybrid when installing core boot classic"
@@ -1328,18 +1312,12 @@ async def run_autoinstall_guided(self, layout):
             encrypted = layout.get("encrypted", None)
             GC = GuidedCapability
             if encrypted is None:
-                if (
-                    GC.CORE_BOOT_ENCRYPTED in core_boot_caps
-                    or GC.CORE_BOOT_PREFER_ENCRYPTED in core_boot_caps
-                ):
-                    capability = GC.CORE_BOOT_ENCRYPTED
-                else:
-                    capability = GC.CORE_BOOT_UNENCRYPTED
+                capability = core_boot_caps[0]
             elif encrypted:
                 capability = GC.CORE_BOOT_ENCRYPTED
             else:
                 if (
-                    core_boot_caps == {GuidedCapability.CORE_BOOT_ENCRYPTED}
+                    core_boot_caps == [GuidedCapability.CORE_BOOT_ENCRYPTED]
                     and not encrypted
                 ):
                     raise Exception("cannot install this model unencrypted")

subiquity/server/controllers/tests/test_filesystem.py

@@ -696,19 +696,26 @@ async def test_bad_modes(self, mode):
 
 
 class TestGuidedV2(IsolatedAsyncioTestCase):
-    async def _setup(self, bootloader, ptable, fix_bios=True, **kw):
+    async def _setup(
+        self, bootloader, ptable, fix_bios=True, snapd_system_labels=(None,), **kw
+    ):
         self.app = make_app()
         self.app.opts.bootloader = bootloader.value
         self.fsc = FilesystemController(app=self.app)
         self.fsc.calculate_suggested_install_min = mock.Mock()
         self.fsc.calculate_suggested_install_min.return_value = 10 << 30
         self.fsc.model = self.model = make_model(bootloader)
         self.fsc._examine_systems_task.start_sync()
+        self.app.snapdapi = snapdapi.make_api_client(AsyncSnapd(get_fake_connection()))
         self.app.dr_cfg = DRConfig()
+        self.app.dr_cfg.systems_dir_exists = True
+        self.app.base_model.source.search_drivers = False
         self.app.base_model.source.current.type = "fsimage"
-        self.app.base_model.source.current.variations = {
-            "default": CatalogEntryVariation(path="", size=1),
-        }
+        variations = self.app.base_model.source.current.variations = {}
+        for label in snapd_system_labels:
+            variations["var" + str(label)] = CatalogEntryVariation(
+                path="", size=1, snapd_system_label=label
+            )
         self.app.controllers.Source.get_handler.return_value = TrivialSourceHandler("")
         await self.fsc._examine_systems_task.wait()
         self.disk = make_disk(self.model, ptable=ptable, **kw)
@@ -719,7 +726,7 @@ async def _setup(self, bootloader, ptable, fix_bios=True, **kw):
             "filesystem": self.fs_probe,
         }
         self.fsc._probe_task.task = mock.Mock()
-        self.fsc._examine_systems_task.task = mock.Mock()
+        # self.fsc._examine_systems_task.task = mock.Mock()
         if bootloader == Bootloader.BIOS and ptable != "msdos" and fix_bios:
             make_partition(
                 self.model,
@@ -1153,6 +1160,29 @@ async def test_in_use_reformat_and_gap(self, bootloader, ptable):
         self.assertEqual(expected, resp.targets)
         self.assertEqual(ProbeStatus.DONE, resp.status)
 
+    @parameterized.expand(
+        [
+            ("mandatory", ["ENCRYPTED"]),
+            ("prefer-encrypted", ["ENCRYPTED", "UNENCRYPTED"]),
+            ("prefer-unencrypted", ["UNENCRYPTED", "ENCRYPTED"]),
+            ("unavailable", ["UNENCRYPTED"]),
+        ]
+    )
+    async def test_core_boot_and_classic(self, label, core_boot_cap_names):
+        expected_core_boot_caps = [
+            getattr(GuidedCapability, f"CORE_BOOT_{cap_name}")
+            for cap_name in core_boot_cap_names
+        ]
+        await self._setup(
+            Bootloader.UEFI,
+            "gpt",
+            snapd_system_labels=[None, label],
+        )
+        resp = await self.fsc.v2_guided_GET()
+        [target, man] = resp.targets
+        got_core_boot_caps = [cap for cap in target.allowed if cap.is_core_boot()]
+        self.assertEqual(got_core_boot_caps, expected_core_boot_caps)
+
 
 class TestManualBoot(IsolatedAsyncioTestCase):
     def _setup(self, bootloader, ptable, **kw):

subiquity/tests/api/test_api.py

@@ -629,7 +629,7 @@ async def test_basic_core_boot(self):
             await inst.post("/source", source_id="ubuntu-desktop")
             resp = await inst.get("/storage/v2/guided", wait=True)
             [reformat, manual] = resp["targets"]
-            self.assertIn("CORE_BOOT_PREFER_ENCRYPTED", reformat["allowed"])
+            self.assertIn("CORE_BOOT_ENCRYPTED", reformat["allowed"])
             data = dict(target=reformat, capability="CORE_BOOT_ENCRYPTED")
             await inst.post("/storage/v2/guided", data)
             v2resp = await inst.get("/storage/v2")

subiquity/ui/views/filesystem/guided.py

@@ -134,39 +134,23 @@ class TPMChoice:
     help: str
 
 
-tpm_help_texts = {
-    "AVAILABLE_CAN_BE_DESELECTED": _(
-        "The entire disk will be encrypted and protected by the "
-        "TPM. If this option is deselected, the disk will be "
-        "unencrypted and without any protection."
-    ),
-    "AVAILABLE_CANNOT_BE_DESELECTED": _(
+tpm_help_texts_1 = {
+    GuidedCapability.CORE_BOOT_ENCRYPTED: _(
         "The entire disk will be encrypted and protected by the TPM."
     ),
-    "UNAVAILABLE":
+    GuidedCapability.CORE_BOOT_UNENCRYPTED:
     # for translators: 'reason' is the reason FDE is unavailable.
     _(
         "TPM backed full-disk encryption is not available "
         'on this device (the reason given was "{reason}").'
     ),
 }
 
-choices = {
-    GuidedCapability.CORE_BOOT_ENCRYPTED: TPMChoice(
-        enabled=False,
-        default=True,
-        help=tpm_help_texts["AVAILABLE_CANNOT_BE_DESELECTED"],
-    ),
-    GuidedCapability.CORE_BOOT_UNENCRYPTED: TPMChoice(
-        enabled=False, default=False, help=tpm_help_texts["UNAVAILABLE"]
-    ),
-    GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED: TPMChoice(
-        enabled=True, default=True, help=tpm_help_texts["AVAILABLE_CAN_BE_DESELECTED"]
-    ),
-    GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED: TPMChoice(
-        enabled=True, default=False, help=tpm_help_texts["AVAILABLE_CAN_BE_DESELECTED"]
-    ),
-}
+tpm_help_texts_can_be_deselected = _(
+    "The entire disk will be encrypted and protected by the "
+    "TPM. If this option is deselected, the disk will be "
+    "unencrypted and without any protection."
+)
 
 
 class GuidedChoiceForm(SubForm):
@@ -223,16 +207,28 @@ def _select_disk(self, sender, val):
         self.use_lvm.enabled = GuidedCapability.LVM in val.allowed
         core_boot_caps = [c for c in val.allowed if c.is_core_boot()]
         if core_boot_caps:
-            assert len(val.allowed) == 1
-            cap = core_boot_caps[0]
+            assert len(val.allowed) == len(core_boot_caps)
+
             reason = ""
             for disallowed in val.disallowed:
                 if disallowed.capability == GuidedCapability.CORE_BOOT_ENCRYPTED:
                     reason = disallowed.message
-            self.tpm_choice = choices[cap]
+
+            cap0 = core_boot_caps[0]
+            if len(core_boot_caps) == 1:
+                self.tpm_choice = TPMChoice(
+                    enabled=False,
+                    default=cap0 == GuidedCapability.CORE_BOOT_ENCRYPTED,
+                    help=tpm_help_texts_1[cap0],
+                )
+            else:
+                self.tpm_choice = TPMChoice(
+                    enabled=True,
+                    default=cap0 == GuidedCapability.CORE_BOOT_ENCRYPTED,
+                    help=tpm_help_texts_can_be_deselected,
+                )
             self.use_tpm.enabled = self.tpm_choice.enabled
             self.use_tpm.value = self.tpm_choice.default
-            self.use_tpm.help = self.tpm_choice.help
             self.use_tpm.help = self.tpm_choice.help.format(reason=reason)
         else:
             self.use_tpm.enabled = False