Merge pull request 'Fix: clan machines delete persistance logic' (#2871) from hsjobeki/clan-core:hsjobeki-main into main

Reviewed-on: https://git.clan.lol/clan/clan-core/pulls/2871

pkgs/clan-cli/clan_cli/inventory/__init__.py

@@ -418,7 +418,7 @@ def load_inventory_json(flake_dir: str | Path) -> Inventory:
     return inventory
 
 
-def delete(d: dict[str, Any], path: str) -> Any:
+def delete_by_path(d: dict[str, Any], path: str) -> Any:
     """
     Deletes the nested entry specified by a dot-separated path from the dictionary using pop().
 
@@ -547,7 +547,7 @@ def set_inventory(inventory: Inventory, flake_dir: str | Path, message: str) ->
         patch(persisted, patch_path, data)
 
     for delete_path in delete_set:
-        delete(persisted, delete_path)
+        delete_by_path(persisted, delete_path)
 
     inventory_file = get_inventory_path(flake_dir)
     with inventory_file.open("w") as f:
@@ -556,6 +556,29 @@ def set_inventory(inventory: Inventory, flake_dir: str | Path, message: str) ->
     commit_file(inventory_file, Path(flake_dir), commit_message=message)
 
 
+@API.register
+def delete(directory: str | Path, delete_set: set[str]) -> None:
+    """
+    Delete keys from the inventory
+    """
+    write_info = get_inventory_with_writeable_keys(directory)
+
+    data_disk = dict(write_info.data_disk)
+
+    for delete_path in delete_set:
+        delete_by_path(data_disk, delete_path)
+
+    inventory_file = get_inventory_path(directory)
+    with inventory_file.open("w") as f:
+        json.dump(data_disk, f, indent=2)
+
+    commit_file(
+        inventory_file,
+        Path(directory),
+        commit_message=f"Delete inventory keys {delete_set}",
+    )
+
+
 def init_inventory(directory: str, init: Inventory | None = None) -> None:
     inventory = None
     # Try reading the current flake

pkgs/clan-cli/clan_cli/machines/delete.py

@@ -6,27 +6,16 @@ from clan_cli.api import API
 from clan_cli.clan_uri import Flake
 from clan_cli.completions import add_dynamic_completer, complete_machines
 from clan_cli.dirs import specific_machine_dir
-from clan_cli.errors import ClanError
-from clan_cli.inventory import get_inventory, set_inventory
+from clan_cli.inventory import delete
 
 log = logging.getLogger(__name__)
 
 
 @API.register
 def delete_machine(flake: Flake, name: str) -> None:
-    inventory = get_inventory(flake.path)
-
-    if "machines" not in inventory:
-        msg = "No machines in inventory"
-        raise ClanError(msg)
-
-    machine = inventory["machines"].pop(name, None)
-    if machine is None:
-        msg = f"Machine {name} does not exist"
-        raise ClanError(msg)
-
-    set_inventory(inventory, flake.path, f"Delete machine {name}")
+    delete(str(flake.path), {f"machines.{name}"})
 
+    # Remove the machine directory
     folder = specific_machine_dir(flake.path, name)
     if folder.exists():
         shutil.rmtree(folder)

pkgs/clan-cli/tests/test_machines_cli.py

@@ -1,4 +1,5 @@
 import pytest
+from clan_cli.inventory import load_inventory_json
 from fixtures_flakes import FlakeForTest
 from helpers import cli
 from stdout import CaptureOutput
@@ -21,6 +22,10 @@ def test_machine_subcommands(
         ]
     )
 
+    inventory: dict = dict(load_inventory_json(str(test_flake_with_core.path)))
+    assert "machine1" in inventory["machines"]
+    assert "service" not in inventory
+
     with capture_output as output:
         cli.run(["machines", "list", "--flake", str(test_flake_with_core.path)])
 
@@ -33,6 +38,10 @@ def test_machine_subcommands(
         ["machines", "delete", "--flake", str(test_flake_with_core.path), "machine1"]
     )
 
+    inventory_2: dict = dict(load_inventory_json(str(test_flake_with_core.path)))
+    assert "machine1" not in inventory_2["machines"]
+    assert "service" not in inventory_2
+
     with capture_output as output:
         cli.run(["machines", "list", "--flake", str(test_flake_with_core.path)])
     assert "machine1" not in output.out

pkgs/clan-cli/tests/test_patch_inventory.py

@@ -5,7 +5,7 @@ import pytest
 from clan_cli.errors import ClanError
 from clan_cli.inventory import (
     calc_patches,
-    delete,
+    delete_by_path,
     determine_writeability,
     patch,
     unmerge_lists,
@@ -384,6 +384,32 @@ def test_dont_persist_defaults() -> None:
     assert delete_set == set()
 
 
+def test_machine_delete() -> None:
+    prios = {
+        "machines": {"__prio": 100},
+    }
+    data_eval = {
+        "machines": {
+            "foo": {"name": "foo"},
+            "bar": {"name": "bar"},
+            "naz": {"name": "naz"},
+        },
+    }
+    data_disk = data_eval
+
+    writeables = determine_writeability(prios, data_eval, data_disk)
+    assert writeables == {"writeable": {"machines"}, "non_writeable": set()}
+
+    # Delete machine "bar"  from the inventory
+    update = {"machines": {"foo": {"name": "foo"}, "naz": {"name": "naz"}}}
+    patchset, delete_set = calc_patches(
+        data_disk, update, all_values=data_eval, writeables=writeables
+    )
+
+    assert patchset == {}
+    assert delete_set == {"machines.bar"}
+
+
 def test_update_mismatching_update_type() -> None:
     prios = {
         "foo": {
@@ -504,7 +530,7 @@ def test_delete_atom() -> None:
     data = {"foo": {"bar": 1}}
     # Removes the key "foo.bar"
     # Returns the deleted key-value pair { "bar": 1 }
-    entry = delete(data, "foo.bar")
+    entry = delete_by_path(data, "foo.bar")
 
     assert entry == {"bar": 1}
     assert data == {"foo": {}}
@@ -513,7 +539,7 @@ def test_delete_atom() -> None:
 def test_delete_intermediate() -> None:
     data = {"a": {"b": {"c": {"d": 42}}}}
     # Removes "a.b.c.d"
-    entry = delete(data, "a.b.c")
+    entry = delete_by_path(data, "a.b.c")
 
     assert entry == {"c": {"d": 42}}
     # Check all intermediate dictionaries remain intact
@@ -523,7 +549,7 @@ def test_delete_intermediate() -> None:
 def test_delete_top_level() -> None:
     data = {"x": 100, "y": 200}
     # Deletes top-level key
-    entry = delete(data, "x")
+    entry = delete_by_path(data, "x")
     assert entry == {"x": 100}
     assert data == {"y": 200}
 
@@ -532,7 +558,7 @@ def test_delete_key_not_found() -> None:
     data = {"foo": {"bar": 1}}
     # Trying to delete a non-existing key "foo.baz"
     with pytest.raises(ClanError) as excinfo:
-        delete(data, "foo.baz")
+        delete_by_path(data, "foo.baz")
     assert "Cannot delete. Path 'foo.baz'" in str(excinfo.value)
     # Data should remain unchanged
     assert data == {"foo": {"bar": 1}}
@@ -542,7 +568,7 @@ def test_delete_intermediate_not_dict() -> None:
     data = {"foo": "not a dict"}
     # Trying to go deeper into a non-dict value
     with pytest.raises(ClanError) as excinfo:
-        delete(data, "foo.bar")
+        delete_by_path(data, "foo.bar")
     assert "not found or not a dictionary" in str(excinfo.value)
     # Data should remain unchanged
     assert data == {"foo": "not a dict"}
@@ -552,7 +578,7 @@ def test_delete_empty_path() -> None:
     data = {"foo": {"bar": 1}}
     # Attempting to delete with an empty path
     with pytest.raises(ClanError) as excinfo:
-        delete(data, "")
+        delete_by_path(data, "")
     # Depending on how you handle empty paths, you might raise an error or handle it differently.
     # If you do raise an error, check the message.
     assert "Cannot delete. Path is empty" in str(excinfo.value)
@@ -563,7 +589,7 @@ def test_delete_non_existent_path_deep() -> None:
     data = {"foo": {"bar": {"baz": 123}}}
     # non-existent deep path
     with pytest.raises(ClanError) as excinfo:
-        delete(data, "foo.bar.qux")
+        delete_by_path(data, "foo.bar.qux")
     assert "not found" in str(excinfo.value)
     # Data remains unchanged
     assert data == {"foo": {"bar": {"baz": 123}}}