Add selective settings menu for OCI modifications

oci/src/proxmenux_oci/management.py

@@ -423,25 +423,8 @@ def manage_instance(project, ui, row, action=None, lifecycle_args=()):
         proposal = None
         if action == 'modify':
             from .recreation import edit_recreation
-            from .cli import _deployment_summary_text
-            from .ui import BacktrackUI, RestartWizard
-            wizard = BacktrackUI(ui)
-            try:
-                while True:
-                    try:
-                        proposal = edit_recreation(record, wizard)
-                        approved = wizard.review(_deployment_summary_text(proposal['candidate']['template'],
-                                                 proposal['candidate']['deployment']), translate('Modify OCI'),
-                                                 question=translate('Apply these changes?'), default=True)
-                        break
-                    except RestartWizard:
-                        wizard.restart()
-                    except ValueError as error:
-                        if not wizard.retry_last(error):
-                            raise
-            finally:
-                wizard.close()
-            if not approved:
+            proposal = edit_recreation(record, ui)
+            if proposal is None:
                 return False
         elif not ui.review(translate('If the image channel has a new version, the CT is stopped, backed up and verified, and replaced with the new image to update the container. If the update fails after the container was changed, the backup is restored. An update that stops halfway is recovered from the OCI management menu.'),
                            translate('Update OCI'), question=translate('Update now?'), default=True):

oci/src/proxmenux_oci/recreation.py

@@ -4,7 +4,7 @@ import re
 from pathlib import Path, PurePosixPath
 
 from .i18n import translate
-from .ui import UserCancelled
+from .ui import BacktrackUI, RestartWizard, UserCancelled
 
 
 def positive_integer(ui, prompt, value, minimum=1):
@@ -30,13 +30,13 @@ def edit_network(deployment, ui):
     network.update(bridge=bridge, ipv4=ipv4, gateway=gateway)
 
 
-def edit_acceleration(candidate, ui):
+def edit_acceleration(candidate, ui, confirm=True):
     from .installer import configure_acceleration
     deployment = candidate['deployment']
     template = candidate.get('template', {})
     installer = template.get('proxmox', {}).get('installer_profile', {})
     hardware = installer.get('hardware_acceleration')
-    if not hardware or not ui.confirm(translate('Change GPU acceleration?'), False):
+    if not hardware or (confirm and not ui.confirm(translate('Change GPU acceleration?'), False)):
         return
     profiles = hardware.get('profiles', [])
     reference = template.get('container_contract', {}).get('image', {}).get('reference', '')
@@ -104,44 +104,142 @@ def edit_environment(deployment, ui):
             environment.append(item)
 
 
-def edit_recreation(record, ui):
-    candidate = copy.deepcopy(record)
-    refresh_template(candidate, ui)
+def modification_options(candidate):
+    """Only offer supported settings; never put environment values in the menu."""
     deployment = candidate['deployment']
     resources = deployment['resources']
-    resources['cores'] = positive_integer(ui, translate('Cores'), resources['cores'])
-    resources['memory_mb'] = positive_integer(ui, translate('RAM in MiB'), resources['memory_mb'], 128)
+    profile = candidate.get('template', {}).get('proxmox', {}).get('installer_profile', {})
+    network = deployment.get('network', {})
+    address = network.get('ipv4', 'dhcp')
+
+    def row(label, value):
+        return f"{label:<26} {value}"
+
+    options = [
+        ('cores', row(translate('CPU cores'), resources['cores'])),
+        ('memory', row(translate('RAM in MiB'), resources['memory_mb'])),
+        ('network', row(translate('Network'),
+                        f"{network.get('bridge', '-')} · {'DHCP' if address == 'dhcp' else address}")),
+        ('paths', row(translate('Extra paths'), len(deployment.get('mounts', [])))),
+    ]
+    if profile.get('shared_directories'):
+        options.append(('shares', translate('Shared directories')))
+    if profile.get('hardware_acceleration'):
+        hardware = profile['hardware_acceleration']
+        selected = next((p for p in hardware.get('profiles', [])
+                         if p['id'] == deployment.get('hardware_profile')), {})
+        options.append(('acceleration', row(translate('Acceleration'),
+                         translate(selected.get('label') or deployment.get('hardware_profile') or 'CPU'))))
+    if profile.get('object_detector'):
+        options.append(('detector', translate('Object detector')))
+    options += [
+        ('devices', row(translate('Devices'), len(deployment.get('devices', [])))),
+        ('environment', row(translate('Variables'), len(deployment.get('environment', [])))),
+        ('catalog', translate('Catalog options')),
+        ('apply', translate('Review and apply')),
+        ('discard', translate('Discard changes')),
+    ]
+    return options
+
+
+def edit_setting(candidate, ui, setting):
+    deployment = candidate['deployment']
     template = candidate.get('template', {})
-    if template.get('proxmox', {}).get('installer_profile', {}).get('shared_directories'):
+    profile = template.get('proxmox', {}).get('installer_profile', {})
+    if setting == 'cores':
+        resources = deployment['resources']
+        resources['cores'] = positive_integer(ui, translate('Cores'), resources['cores'])
+    elif setting == 'memory':
+        resources = deployment['resources']
+        resources['memory_mb'] = positive_integer(ui, translate('RAM in MiB'), resources['memory_mb'], 128)
+    elif setting == 'network':
+        edit_network(deployment, ui)
+    elif setting == 'shares':
         from .installer import ask_shared_directories
         deployment['mounts'] += ask_shared_directories(
             ui, template, deployment['mounts'], deployment['rootfs']['storage'], True, optional=True,
             unprivileged=deployment.get('security', {}).get('unprivileged', True))
-    from .custom_mounts import ask_custom_mounts
-    deployment['mounts'] = ask_custom_mounts(
-        ui, deployment['mounts'], deployment['rootfs']['storage'])
-    if ui.confirm(translate('Change the access network?'), False):
-        edit_network(deployment, ui)
-    edit_acceleration(candidate, ui)
-    from .installer import configure_detector
-    installer_profile = candidate.get('template', {}).get('proxmox', {}).get('installer_profile', {})
-    deployment['devices'], notes = configure_detector(installer_profile, deployment.get('devices', []), ui)
-    if notes:
-        deployment['completion_notes'] = notes
-    from .extra_devices import ask_extra_devices
-    reference = candidate.get('template', {}).get('container_contract', {}).get('image', {}).get('reference', '')
-    repository = reference.split('@')[0].rsplit(':', 1)[0]
-    deployment['devices'] = ask_extra_devices(
-        ui, deployment.get('devices', []), deployment.get('security', {}).get('unprivileged', True),
-        allow_coral=repository in ('ghcr.io/blakeblackshear/frigate', 'codeproject/ai-server', 'docker.io/codeproject/ai-server'))
-    from .extra_devices import device_permissions
-    deployment['device_permissions'] = device_permissions(
-        reference, deployment['devices'], deployment.get('device_permissions'))
-    edit_environment(deployment, ui)
-    proposal = {'operation': 'recreate', 'candidate': candidate}
-    if record.get('observed', {}).get('config_sha256'):
-        proposal['base_config_sha256'] = record['observed']['config_sha256']
-    return proposal
+    elif setting == 'paths':
+        from .custom_mounts import ask_custom_mounts
+        deployment['mounts'] = ask_custom_mounts(ui, deployment['mounts'], deployment['rootfs']['storage'])
+    elif setting == 'acceleration':
+        edit_acceleration(candidate, ui, confirm=False)
+    elif setting == 'detector':
+        from .installer import configure_detector
+        deployment['devices'], notes = configure_detector(profile, deployment.get('devices', []), ui)
+        if notes:
+            deployment['completion_notes'] = notes
+    elif setting == 'devices':
+        from .extra_devices import ask_extra_devices
+        reference = template.get('container_contract', {}).get('image', {}).get('reference', '')
+        repository = reference.split('@')[0].rsplit(':', 1)[0]
+        deployment['devices'] = ask_extra_devices(
+            ui, deployment.get('devices', []), deployment.get('security', {}).get('unprivileged', True),
+            allow_coral=repository in ('ghcr.io/blakeblackshear/frigate', 'codeproject/ai-server', 'docker.io/codeproject/ai-server'))
+    elif setting == 'environment':
+        edit_environment(deployment, ui)
+    elif setting == 'catalog':
+        refresh_template(candidate, ui)
+    else:
+        raise ValueError(setting)
+    if setting in ('acceleration', 'detector', 'devices'):
+        from .extra_devices import device_permissions
+        reference = candidate.get('template', {}).get('container_contract', {}).get('image', {}).get('reference', '')
+        permissions = device_permissions(reference, deployment.get('devices', []), deployment.get('device_permissions'))
+        if permissions is not None or 'device_permissions' in deployment:
+            deployment['device_permissions'] = permissions
+
+
+def edit_section(candidate, ui, setting):
+    """A cancelled or invalid section cannot leave a half-edited draft."""
+    wizard = BacktrackUI(ui)
+    try:
+        while True:
+            edited = copy.deepcopy(candidate)
+            try:
+                edit_setting(edited, wizard, setting)
+                return edited
+            except RestartWizard:
+                wizard.restart()
+            except UserCancelled:
+                return candidate
+            except ValueError as error:
+                if not wizard.retry_last(error):
+                    ui.message(str(error), translate('Modify OCI'))
+                    return candidate
+    finally:
+        wizard.close()
+
+
+def edit_recreation(record, ui):
+    """Keep a draft across independently editable settings; apply it once."""
+    from .cli import _deployment_summary_text
+    candidate = copy.deepcopy(record)
+    selected = 'cores'
+    while True:
+        pending = candidate != record
+        status = translate('Unapplied changes') if pending else translate('No pending changes')
+        text = (f"CT {record.get('vmid', '-')}: {candidate['deployment'].get('hostname', '')}\n"
+                f"{status}\n\n"
+                + translate('Choose a setting. Changes are applied only after the final review.'))
+        selected = ui.choose(text, modification_options(candidate), selected,
+                             title=translate('Modify OCI'), show_tags=False)
+        if selected is None or selected == 'discard':
+            if not pending or ui.confirm(translate('Discard unapplied changes?'), False):
+                return None
+            selected = 'apply'
+        elif selected == 'apply':
+            if not pending:
+                ui.message(translate('Nothing was changed.'), translate('Modify OCI'))
+                return None
+            if ui.review(_deployment_summary_text(candidate['template'], candidate['deployment']),
+                         translate('Modify OCI'), question=translate('Apply these changes?'), default=True):
+                proposal = {'operation': 'recreate', 'candidate': candidate}
+                if record.get('observed', {}).get('config_sha256'):
+                    proposal['base_config_sha256'] = record['observed']['config_sha256']
+                return proposal
+        else:
+            candidate = edit_section(candidate, ui, selected)
 
 
 def refresh_template(candidate, ui):

oci/tests/test_modify_menu.py

@@ -0,0 +1,201 @@
+"""Selective OCI edits stay in a draft until reviewed, then use one transaction."""
+import copy
+import json
+from pathlib import Path
+import stat
+import sys
+import unittest
+from unittest.mock import Mock, patch
+
+ROOT = Path(__file__).resolve().parents[1]
+sys.path.insert(0, str(ROOT / 'src'))
+sys.path.insert(0, str(ROOT / 'remote'))
+
+from proxmenux_oci import management, recreation
+from proxmenux_oci.ui import BackRequested, UserCancelled
+
+
+def record():
+    return {'vmid': 115, 'installation_id': 'test-installation', 'status': 'installed',
+            'observed': {'config_sha256': 'native-config-fingerprint'},
+            'template': {'id': 'example-app', 'catalog_ui': {'title': 'Example'},
+                         'container_contract': {'image': {'reference': 'example/app:latest'}},
+                         'proxmox': {'installer_profile': {}}},
+            'deployment': {'vmid': 115, 'hostname': 'example', 'resources': {'cores': 2, 'memory_mb': 512},
+                           'network': {'bridge': 'vmbr0', 'ipv4': 'dhcp', 'gateway': None},
+                           'rootfs': {'storage': 'local-zfs', 'size_gb': 8}, 'mounts': [
+                               {'container_path': '/data', 'source': 'local-zfs',
+                                'type': 'managed-volume', 'size_gb': 8, 'backup': True}],
+                           'devices': [], 'security': {'unprivileged': True},
+                           'environment': [{'name': 'PASSWORD', 'value': 'private-test-secret',
+                                            'sensitive': True}]}}
+
+
+class MenuUI:
+    def __init__(self, choices, answers=(), reviews=(True,), confirms=()):
+        self.choices, self.answers = iter(choices), iter(answers)
+        self.reviews, self.confirms = iter(reviews), iter(confirms)
+        self.events = []
+        self.back_enabled = False
+
+    def choose(self, text, options, default=None, **kwargs):
+        self.events.append(('menu', text, dict(options)))
+        value = next(self.choices)
+        if value is not None:
+            assert value in dict(options), value
+        return value
+
+    def ask(self, text, default=None, **kwargs):
+        self.events.append(('ask', text, default))
+        value = next(self.answers)
+        if isinstance(value, Exception):
+            raise value
+        return value
+
+    def confirm(self, text, default=False):
+        self.events.append(('confirm', text))
+        return next(self.confirms)
+
+    def review(self, text, *args, **kwargs):
+        self.events.append(('review', text))
+        return next(self.reviews)
+
+    def message(self, text, *args):
+        self.events.append(('message', text))
+
+
+@patch('proxmenux_oci.i18n.language', return_value='en')
+class ModifyMenuTests(unittest.TestCase):
+    def test_ram_only_does_not_ask_about_cpu_network_paths_devices_or_catalog(self, _language):
+        original = record()
+        saved = copy.deepcopy(original)
+        ui = MenuUI(['memory', 'apply'], ['768'])
+        with patch.object(recreation, 'refresh_template') as refresh:
+            result = recreation.edit_recreation(original, ui)
+        self.assertEqual(result['candidate']['deployment']['resources'], {'cores': 2, 'memory_mb': 768})
+        self.assertEqual([e[1] for e in ui.events if e[0] == 'ask'], ['RAM in MiB'])
+        self.assertEqual(result['base_config_sha256'], 'native-config-fingerprint')
+        self.assertEqual(original, saved)
+        refresh.assert_not_called()
+        self.assertFalse(ui.back_enabled)
+
+    def test_several_sections_and_revisits_keep_the_current_draft(self, _language):
+        ui = MenuUI(['memory', 'cores', 'memory', 'apply'], ['768', '3', '1024'])
+        result = recreation.edit_recreation(record(), ui)
+        self.assertEqual(result['candidate']['deployment']['resources'], {'cores': 3, 'memory_mb': 1024})
+        self.assertEqual([e[2] for e in ui.events if e[0] == 'ask'], ['512', '2', '768'])
+        self.assertEqual(sum(e[0] == 'review' for e in ui.events), 1)
+
+    def test_declining_review_returns_to_menu_without_losing_edits(self, _language):
+        ui = MenuUI(['cores', 'apply', 'memory', 'apply'], ['3', '768'], reviews=[False, True])
+        result = recreation.edit_recreation(record(), ui)
+        self.assertEqual(result['candidate']['deployment']['resources'], {'cores': 3, 'memory_mb': 768})
+        self.assertEqual(sum(e[0] == 'review' for e in ui.events), 2)
+
+    def test_unchanged_apply_does_not_propose_recreating_the_guest(self, _language):
+        ui = MenuUI(['memory', 'apply'], ['512'])
+        self.assertIsNone(recreation.edit_recreation(record(), ui))
+        self.assertFalse(any(e[0] == 'review' for e in ui.events))
+        self.assertIn(('message', 'Nothing was changed.'), ui.events)
+
+    def test_discard_leaves_the_saved_record_untouched(self, _language):
+        original = record()
+        saved = copy.deepcopy(original)
+        ui = MenuUI(['memory', 'discard'], ['768'], confirms=[True])
+        self.assertIsNone(recreation.edit_recreation(original, ui))
+        self.assertEqual(original, saved)
+        self.assertFalse(any(e[0] == 'review' for e in ui.events))
+
+    def test_declining_discard_preserves_the_draft(self, _language):
+        ui = MenuUI(['memory', None, 'apply'], ['768'], confirms=[False])
+        result = recreation.edit_recreation(record(), ui)
+        self.assertEqual(result['candidate']['deployment']['resources']['memory_mb'], 768)
+
+    def test_cancel_without_changes_asks_nothing_else(self, _language):
+        ui = MenuUI([None])
+        self.assertIsNone(recreation.edit_recreation(record(), ui))
+        self.assertEqual(len(ui.events), 1)
+
+    def test_invalid_ram_is_retried_without_restarting_other_sections(self, _language):
+        ui = MenuUI(['cores', 'memory', 'apply'], ['3', 'invalid', '64', '768'])
+        result = recreation.edit_recreation(record(), ui)
+        self.assertEqual(result['candidate']['deployment']['resources'], {'cores': 3, 'memory_mb': 768})
+        self.assertEqual(sum(e[0] == 'message' for e in ui.events), 2)
+        self.assertEqual(sum(e[:2] == ('ask', 'Cores') for e in ui.events), 1)
+
+    def test_back_from_first_field_returns_to_menu_and_keeps_other_edits(self, _language):
+        ui = MenuUI(['cores', 'memory', 'apply'], ['3', BackRequested()])
+        result = recreation.edit_recreation(record(), ui)
+        self.assertEqual(result['candidate']['deployment']['resources'], {'cores': 3, 'memory_mb': 512})
+        self.assertFalse(ui.back_enabled)
+
+    def test_cancelling_a_partly_edited_section_rolls_back_only_that_section(self, _language):
+        real_edit = recreation.edit_setting
+        def edit(candidate, ui, setting):
+            if setting == 'environment':
+                candidate['deployment']['environment'][0]['value'] = 'partial-value'
+                raise UserCancelled()
+            real_edit(candidate, ui, setting)
+        ui = MenuUI(['cores', 'environment', 'apply'], ['3'])
+        with patch.object(recreation, 'edit_setting', side_effect=edit):
+            result = recreation.edit_recreation(record(), ui)
+        self.assertEqual(result['candidate']['deployment']['resources']['cores'], 3)
+        self.assertEqual(result['candidate']['deployment']['environment'], record()['deployment']['environment'])
+
+    def test_menu_and_review_do_not_expose_a_saved_secret(self, _language):
+        ui = MenuUI(['cores', 'apply'], ['3'])
+        recreation.edit_recreation(record(), ui)
+        self.assertNotIn('private-test-secret', json.dumps(ui.events))
+
+    def test_optional_categories_follow_the_application_profile(self, _language):
+        candidate = record()
+        options = dict(recreation.modification_options(candidate))
+        self.assertTrue({'cores', 'memory', 'network', 'paths', 'devices', 'environment', 'catalog', 'apply'} <= options.keys())
+        self.assertFalse({'shares', 'acceleration', 'detector'} & options.keys())
+        candidate['template']['proxmox']['installer_profile'] = {
+            'shared_directories': {'base_path': '/shares'}, 'object_detector': {'devices': []},
+            'hardware_acceleration': {'profiles': [{'id': 'intel', 'label': 'Intel GPU'}]}}
+        candidate['deployment']['hardware_profile'] = 'intel'
+        options = dict(recreation.modification_options(candidate))
+        self.assertTrue({'shares', 'acceleration', 'detector'} <= options.keys())
+        self.assertIn('Intel GPU', options['acceleration'])
+
+
+class ModifyDispatchTests(unittest.TestCase):
+    def call_manager(self, proposal, lifecycle):
+        import oci_instances
+        import oci_instance_reconcile
+        saved = record()
+        row = {'vmid': 115, 'reason': 'matched', 'stack': False, 'pending': False, 'status': 'installed'}
+        with patch.object(management, 'carry_records', return_value={}), \
+                patch.object(management, 'check_selected', return_value=row), \
+                patch.object(management, '_work_backup_ready', return_value=True), \
+                patch.object(oci_instances, 'read', return_value=saved), \
+                patch.object(oci_instances, 'command', return_value='hostname: example'), \
+                patch.object(oci_instance_reconcile, 'propose', return_value=None), \
+                patch.object(recreation, 'edit_recreation', return_value=proposal), \
+                patch.object(management, '_run_lifecycle', side_effect=lifecycle):
+            return management.manage_instance(ROOT, Mock(), row, action='modify')
+
+    def test_cancel_does_not_start_a_lifecycle_transaction(self):
+        lifecycle = Mock()
+        self.assertFalse(self.call_manager(None, lifecycle))
+        lifecycle.assert_not_called()
+
+    def test_apply_sends_one_private_proposal_to_the_existing_backend(self):
+        proposal = {'operation': 'recreate', 'candidate': record(), 'base_config_sha256': 'fingerprint'}
+        seen = []
+        def lifecycle(command, title):
+            path = Path(command[command.index('--proposal') + 1])
+            seen.append(json.loads(path.read_text()))
+            self.assertEqual(stat.S_IMODE(path.stat().st_mode), 0o600)
+            self.assertNotIn('private-test-secret', ' '.join(command))
+            self.assertIn('oci_update_current.py', command[1])
+            self.assertIn('--acknowledge-external-data', command)
+            return True
+        self.assertTrue(self.call_manager(proposal, lifecycle))
+        self.assertEqual(seen, [proposal])
+
+
+if __name__ == '__main__':
+    unittest.main()

oci/tests/test_shared_directories.py

@@ -43,15 +43,18 @@ class ScriptedUI:
     def password(self, text, required=True):
         return self._answer(text, "")
 
-    def choose(self, text, options, default=None):
+    def choose(self, text, options, default=None, **_kwargs):
         return self._answer(text, default)
 
     def confirm(self, text, default=False):
         return self._answer(text, default)
 
-    def message(self, text):
+    def message(self, text, *_args):
         pass
 
+    def review(self, *_args, **_kwargs):
+        return True
+
 
 @patch("proxmenux_oci.i18n.language", return_value="en")
 @patch("proxmenux_oci.installer.host.storages", side_effect=storages)
@@ -223,14 +226,15 @@ class SharedDirectoriesTests(unittest.TestCase):
              patch("proxmenux_oci.recreation.edit_acceleration"), \
              patch("proxmenux_oci.recreation.edit_environment"):
             ui = ScriptedUI({"Add a directory to share": [True], NAME: ["photos"],
+                             "Choose a setting. Changes are applied only after the final review.": ["shares", "apply"],
                              "Where to store /shares/photos": ["host-bind"],
                              "Host path for /shares/photos": ["/tank/photos"]})
             proposal = edit_recreation(record, ui)
             mounts = [m["container_path"] for m in proposal["candidate"]["deployment"]["mounts"]]
             self.assertEqual(mounts, ["/config", "/shares/media", "/shares/photos"])
-            untouched = edit_recreation(record, ScriptedUI())
-            self.assertEqual([m["container_path"] for m in untouched["candidate"]["deployment"]["mounts"]],
-                             ["/config", "/shares/media"])
+            untouched = edit_recreation(record, ScriptedUI({
+                "Choose a setting. Changes are applied only after the final review.": ["discard"]}))
+            self.assertIsNone(untouched)
         self.assertEqual([m["container_path"] for m in plan["mounts"]], ["/config", "/shares/media"])