refactor: do not modify Collection logic

tdp/core/collection.py

@@ -14,7 +14,6 @@
     YML_EXTENSION,
 )
 from tdp.core.inventory_reader import InventoryReader
-from tdp.core.operation import Operation
 from tdp.core.types import PathLike
 
 MANDATORY_DIRECTORIES = [
@@ -66,7 +65,7 @@ def __init__(
 
         self._inventory_reader = inventory_reader or InventoryReader()
         self._dag_yamls: Optional[list[Path]] = None
-        self._init_playbooks()
+        self._playbooks: Optional[dict[str, Path]] = None
 
     @staticmethod
     def from_path(path: PathLike) -> "Collection":
@@ -117,21 +116,15 @@ def dag_yamls(self) -> list[Path]:
         return self._dag_yamls
 
     @property
-    def playbooks(self) -> dict[str, Operation]:
+    def playbooks(self) -> dict[str, Path]:
         """Dictionary of playbooks."""
+        if not self._playbooks:
+            self._playbooks = {
+                playbook.stem: playbook
+                for playbook in self.playbooks_directory.glob("*" + YML_EXTENSION)
+            }
         return self._playbooks
 
-    def _init_playbooks(self):
-        """Initialize the playbooks."""
-        self._playbooks = {
-            playbook.stem: Operation(
-                name=playbook.stem,
-                collection_name=self.name,
-                host_names=read_host_from_playbook(playbook, self._inventory_reader),
-            )
-            for playbook in self.playbooks_directory.glob("*" + YML_EXTENSION)
-        }
-
     def get_service_default_vars(self, service_name: str) -> list[tuple[str, Path]]:
         """Get the default variables for a service.
 
@@ -159,6 +152,24 @@ def get_service_schema(self, service_name: str) -> dict:
         with schema_path.open() as fd:
             return json.load(fd)
 
+    def get_hosts_from_playbook(self, playbook: str) -> set[str]:
+        """Get the set of hosts for a playbook.
+
+        Args:
+            playbook: Playbook name without extension.
+
+        Returns:
+            Set of hosts.
+        """
+        if playbook not in self.playbooks:
+            raise MissingPlaybookError(f"Playbook {playbook} not found.")
+        try:
+            return self._inventory_reader.get_hosts_from_playbook(
+                self.playbooks[playbook].open()
+            )
+        except Exception as e:
+            raise ValueError(f"Can't parse playbook {self.playbooks[playbook]}.") from e
+
     def _check_path(self):
         """Validate the collection path content."""
         if not self._path.exists():
@@ -171,28 +182,3 @@ def _check_path(self):
                 raise MissingMandatoryDirectoryError(
                     f"{self._path} does not contain the mandatory directory {mandatory_directory}.",
                 )
-
-
-def read_host_from_playbook(
-    playbook_path: Path, inventory_reader: Optional[InventoryReader] = None
-) -> set[str]:
-    """Read the hosts from a playbook.
-
-    Args:
-        playbook_path: A Path object pointing to the playbook file.
-        inventory_reader: An optional InventoryReader instance to read the inventory.
-          If None, a new instance will be created.
-
-    Returns:
-        A set of hosts.
-
-    Raises:
-        FileNotFoundError: If the playbook file is not found.
-        ValueError: If the playbook file cannot be parsed.
-    """
-    inventory_reader = inventory_reader or InventoryReader()
-    try:
-        with playbook_path.open() as fd:
-            return inventory_reader.get_hosts_from_playbook(fd)
-    except Exception as e:
-        raise ValueError(f"Error parsing playbook {playbook_path}: {str(e)}") from e

tdp/core/collections.py

@@ -149,18 +149,17 @@ def _init_operations(
                 for read_operation in read_tdp_lib_dag_file(dag_file):
                     existing_operation = dag_operations.get(read_operation.name)
 
-                    # The read_operation is a playbook operation defined in the current
-                    # collection
-                    if playbook_operation := collection.playbooks.get(
-                        read_operation.name
-                    ):
-                        # Create an Operation instance based on the playbook
+                    # The read_operation is associated with a playbook defined in the
+                    # current collection
+                    if _ := collection.playbooks.get(read_operation.name):
                         # TODO: would be nice to dissociate the Operation class from the playbook and store the playbook in the Operation
                         dag_operation_to_register = Operation(
                             name=read_operation.name,
-                            collection_name=playbook_operation.collection_name,
-                            host_names=playbook_operation.host_names,
-                            depends_on=read_operation.depends_on,
+                            collection_name=collection.name,
+                            host_names=collection.get_hosts_from_playbook(
+                                read_operation.name
+                            ),
+                            depends_on=read_operation.depends_on.copy(),
                         )
                         # If the operation is already listed, merge its dependencies
                         if existing_operation:
@@ -194,7 +193,7 @@ def _init_operations(
                     dag_operations[read_operation.name] = Operation(
                         name=read_operation.name,
                         collection_name=collection.name,
-                        depends_on=read_operation.depends_on,
+                        depends_on=read_operation.depends_on.copy(),
                         noop=True,
                         host_names=None,
                     )
@@ -212,7 +211,7 @@ def _init_operations(
                         other_operations[restart_operation_name] = Operation(
                             name=restart_operation_name,
                             collection_name="replace_restart_noop",
-                            depends_on=read_operation.depends_on,
+                            depends_on=read_operation.depends_on.copy(),
                             noop=True,
                             host_names=None,
                         )
@@ -223,7 +222,7 @@ def _init_operations(
                         other_operations[stop_operation_name] = Operation(
                             name=stop_operation_name,
                             collection_name="replace_stop_noop",
-                            depends_on=read_operation.depends_on,
+                            depends_on=read_operation.depends_on.copy(),
                             noop=True,
                             host_names=None,
                         )
@@ -234,16 +233,20 @@ def _init_operations(
         for collection in collections.values():
             # Load playbook operations to complete the operations list with the
             # operations that are not defined in the DAG files
-            for operation in collection.playbooks.values():
-                if operation.name in dag_operations:
+            for operation_name in collection.playbooks:
+                if operation_name in dag_operations:
                     continue
-                if operation.name in other_operations:
+                if operation_name in other_operations:
                     logger.debug(
-                        f"'{operation.name}' defined in "
-                        f"'{other_operations[operation.name].collection_name}' "
+                        f"'{operation_name}' defined in "
+                        f"'{other_operations[operation_name].collection_name}' "
                         f"is overridden by '{collection.name}'"
                     )
-                other_operations[operation.name] = operation
+                other_operations[operation_name] = Operation(
+                    name=operation_name,
+                    host_names=collection.get_hosts_from_playbook(operation_name),
+                    collection_name=collection.name,
+                )
 
         return dag_operations, other_operations
 

tdp/core/deployment/deployment_runner.py

@@ -66,11 +66,9 @@ def _run_operation(self, operation_rec: OperationModel) -> None:
             return
 
         # Execute the operation
-        playbook_file = self._collections[
-            operation.collection_name
-        ].playbooks_directory / (
-            operation.name + ".yml"
-        )  # TODO: would be better to store the playbook path in the operation object
+        playbook_file = self._collections[operation.collection_name].playbooks[
+            operation.name
+        ]
         state, logs = self._executor.execute(
             playbook=playbook_file,
             host=operation_rec.host,

tdp/core/test_collection.py

@@ -11,10 +11,8 @@
     MissingMandatoryDirectoryError,
     PathDoesNotExistsError,
     PathIsNotADirectoryError,
-    read_host_from_playbook,
 )
 from tdp.core.constants import DAG_DIRECTORY_NAME
-from tdp.core.models.test_deployment_log import MockInventoryReader
 
 
 def test_collection_from_path_does_not_exist():
@@ -52,25 +50,3 @@ def test_collection_from_path(tmp_path_factory: pytest.TempPathFactory):
     assert collection_path / DAG_DIRECTORY_NAME / "service.yml" in collection.dag_yamls
     assert "service_install" in collection.playbooks
     assert "service_config" in collection.playbooks
-
-
-def test_read_host_from_playbook(tmp_path: Path):
-    # Create a temporary playbook file
-    playbook_path = tmp_path / "playbook.yml"
-    playbook_path.write_text(
-        """---
-        - name: Play 1
-          hosts: host1, host2
-          tasks:
-            - name: Task 1
-              command: echo "Hello, World!"
-        """
-    )
-
-    # Call the function under test
-    hosts = read_host_from_playbook(
-        playbook_path, MockInventoryReader(["host1", "host2"])
-    )
-
-    # Assert the expected result
-    assert hosts == {"host1", "host2"}