Răsfoiți Sursa

merge(release): merge v0.0.7 improvements into v0.1.0

Includes comprehensive variable dependency and prompt improvements
xcad 4 luni în urmă
părinte
comite
0f39b1e68c

+ 47 - 6
cli/core/collection.py

@@ -65,14 +65,25 @@ class VariableCollection:
 
     def _create_section(self, key: str, data: dict[str, Any]) -> VariableSection:
         """Create a VariableSection from data."""
+        # Build section init data with only explicitly provided fields
+        # This prevents None values from overriding module spec values during merge
         section_init_data = {
             "key": key,
             "title": data.get("title", key.replace("_", " ").title()),
-            "description": data.get("description"),
-            "toggle": data.get("toggle"),
-            "required": data.get("required", key == "general"),
-            "needs": data.get("needs"),
         }
+        
+        # Only add optional fields if explicitly provided in the source data
+        if "description" in data:
+            section_init_data["description"] = data["description"]
+        if "toggle" in data:
+            section_init_data["toggle"] = data["toggle"]
+        if "required" in data:
+            section_init_data["required"] = data["required"]
+        elif key == "general":
+            section_init_data["required"] = True
+        if "needs" in data:
+            section_init_data["needs"] = data["needs"]
+            
         return VariableSection(section_init_data)
 
     def _initialize_variables(
@@ -277,9 +288,13 @@ class VariableCollection:
                         )
                 else:
                     # New format: validate variable exists
+                    # NOTE: We only warn here, not raise an error, because the variable might be
+                    # added later during merge with module spec. The actual runtime check in
+                    # _is_need_satisfied() will handle missing variables gracefully.
                     if var_or_section not in self._variable_map:
-                        raise ValueError(
-                            f"Section '{section_key}' has need '{dep}', but variable '{var_or_section}' does not exist"
+                        logger.debug(
+                            f"Section '{section_key}' has need '{dep}', but variable '{var_or_section}' "
+                            f"not found (might be added during merge)"
                         )
 
         # Check for missing dependencies in variables
@@ -418,6 +433,18 @@ class VariableCollection:
         # Rebuild _sections dict in new order
         self._sections = {key: section for _, (key, section) in sorted_items}
 
+        # NOTE: Sort variables within each section by their dependencies.
+        # This is critical for correct behavior in both display and prompts:
+        # 1. DISPLAY: Variables are shown in logical order (dependencies before dependents)
+        # 2. PROMPTS: Users are asked for dependency values BEFORE dependent values
+        #    Example: network_mode (bridge/host/macvlan) is prompted before
+        #             network_macvlan_ipv4_address (which needs network_mode=macvlan)
+        # 3. VALIDATION: Ensures config/CLI overrides can be checked in correct order
+        # Without this sorting, users would be prompted for irrelevant variables or see
+        # confusing variable order in the UI.
+        for section in self._sections.values():
+            section.sort_variables(self._is_need_satisfied)
+
     def _topological_sort(self) -> List[str]:
         """Perform topological sort on sections based on dependencies using Kahn's algorithm."""
         in_degree = {key: len(section.needs) for key, section in self._sections.items()}
@@ -558,6 +585,20 @@ class VariableCollection:
                         except Exception:
                             pass  # If conversion fails, let normal validation handle it
 
+                # Check if variable's needs are satisfied
+                # If not, warn that the override will have no effect
+                if not self.is_variable_satisfied(var_name):
+                    # Build a friendly message about which needs aren't satisfied
+                    unmet_needs = []
+                    for need in variable.needs:
+                        if not self._is_need_satisfied(need):
+                            unmet_needs.append(need)
+                    needs_str = ", ".join(unmet_needs) if unmet_needs else "unknown"
+                    logger.warning(
+                        f"Setting '{var_name}' via {origin} will have no effect - needs not satisfied: {needs_str}"
+                    )
+                    # Continue anyway to store the value (it might become relevant later)
+
                 # Store original value before overriding (for display purposes)
                 # Only store if this is the first time config is being applied
                 if origin == "config" and not hasattr(variable, "_original_stored"):

+ 10 - 7
cli/core/prompt.py

@@ -84,14 +84,10 @@ class PromptHandler:
             elif section.toggle:
                 toggle_var = section.variables.get(section.toggle)
                 if toggle_var:
-                    # Use description for prompt if available, otherwise use title
-                    prompt_text = (
-                        section.description
-                        if section.description
-                        else f"Enable {section.title}?"
-                    )
+                    # Prompt for toggle variable using standard variable prompting logic
+                    # This ensures consistent handling of description, extra text, validation hints, etc.
                     current_value = toggle_var.convert(toggle_var.value)
-                    new_value = self._prompt_bool(prompt_text, current_value)
+                    new_value = self._prompt_variable(toggle_var, required=section.required)
 
                     if new_value != current_value:
                         collected[toggle_var.name] = new_value
@@ -107,6 +103,13 @@ class PromptHandler:
                 if section.toggle and var_name == section.toggle:
                     continue
 
+                # Skip variables with unsatisfied needs (similar to display logic)
+                if not variables.is_variable_satisfied(var_name):
+                    logger.debug(
+                        f"Skipping variable '{var_name}' - needs not satisfied"
+                    )
+                    continue
+
                 # Skip non-required variables if section is disabled
                 if not section_will_be_enabled and not variable.required:
                     logger.debug(

+ 70 - 1
cli/core/section.py

@@ -34,10 +34,15 @@ class VariableSection:
         # Default "general" section to required=True, all others to required=False
         self.required: bool = data.get("required", data["key"] == "general")
         # Section dependencies - can be string or list of strings
+        # Supports semicolon-separated multiple conditions: "var1=value1;var2=value2,value3"
         needs_value = data.get("needs")
         if needs_value:
             if isinstance(needs_value, str):
-                self.needs: List[str] = [needs_value]
+                # Split by semicolon to support multiple AND conditions in a single string
+                # Example: "traefik_enabled=true;network_mode=bridge,macvlan"
+                self.needs: List[str] = [
+                    need.strip() for need in needs_value.split(";") if need.strip()
+                ]
             elif isinstance(needs_value, list):
                 self.needs: List[str] = needs_value
             else:
@@ -125,3 +130,67 @@ class VariableSection:
                 cloned.variables[var_name] = variable.clone()
 
         return cloned
+
+    def sort_variables(self, is_need_satisfied_func=None) -> None:
+        """Sort variables within section for optimal display and user interaction.
+
+        Current sorting strategy:
+        - Variables with no dependencies come first
+        - Variables that depend on others come after their dependencies (topological sort)
+        - Original order is preserved for variables at the same dependency level
+
+        Future sorting strategies can be added here (e.g., by type, required first, etc.)
+
+        Args:
+            is_need_satisfied_func: Optional function to check if a variable need is satisfied
+                                   (reserved for future use in conditional sorting)
+        """
+        if not self.variables:
+            return
+
+        # Build dependency graph
+        var_list = list(self.variables.keys())
+        var_set = set(var_list)
+
+        # For each variable, find which OTHER variables in THIS section it depends on
+        dependencies = {var_name: [] for var_name in var_list}
+        for var_name in var_list:
+            variable = self.variables[var_name]
+            if variable.needs:
+                for need in variable.needs:
+                    # Parse need format: "variable_name=value"
+                    dep_var = need.split("=")[0] if "=" in need else need
+                    # Only track dependencies within THIS section
+                    if dep_var in var_set and dep_var != var_name:
+                        dependencies[var_name].append(dep_var)
+
+        # Topological sort using Kahn's algorithm
+        in_degree = {var_name: len(deps) for var_name, deps in dependencies.items()}
+        queue = [var for var, degree in in_degree.items() if degree == 0]
+        # Preserve original order for variables with same dependency level
+        queue.sort(key=lambda v: var_list.index(v))
+        result = []
+
+        while queue:
+            current = queue.pop(0)
+            result.append(current)
+
+            # Update in-degree for dependent variables
+            for var_name, deps in dependencies.items():
+                if current in deps:
+                    in_degree[var_name] -= 1
+                    if in_degree[var_name] == 0:
+                        queue.append(var_name)
+                        queue.sort(key=lambda v: var_list.index(v))
+
+        # If not all variables were sorted (cycle), append remaining in original order
+        if len(result) != len(var_list):
+            for var_name in var_list:
+                if var_name not in result:
+                    result.append(var_name)
+
+        # Rebuild variables OrderedDict in new order
+        sorted_vars = OrderedDict()
+        for var_name in result:
+            sorted_vars[var_name] = self.variables[var_name]
+        self.variables = sorted_vars

+ 6 - 1
cli/core/variable.py

@@ -62,10 +62,15 @@ class Variable:
         # Original value before config override (used for display)
         self.original_value: Optional[Any] = data.get("original_value")
         # Variable dependencies - can be string or list of strings in format "var_name=value"
+        # Supports semicolon-separated multiple conditions: "var1=value1;var2=value2,value3"
         needs_value = data.get("needs")
         if needs_value:
             if isinstance(needs_value, str):
-                self.needs: List[str] = [needs_value]
+                # Split by semicolon to support multiple AND conditions in a single string
+                # Example: "traefik_enabled=true;network_mode=bridge,macvlan"
+                self.needs: List[str] = [
+                    need.strip() for need in needs_value.split(";") if need.strip()
+                ]
             elif isinstance(needs_value, list):
                 self.needs: List[str] = needs_value
             else:

+ 6 - 2
library/compose/pihole/template.yaml

@@ -67,12 +67,12 @@ spec:
         type: bool
         description: "Enable DNS server functionality"
         default: true
-        extra: "Requires port 53 to be exposed or macvlan network mode"
+        extra: "Exposes port 53 for DNS queries in bridge network mode"
       dhcp_enabled:
         type: bool
         description: "Enable DHCP server functionality"
         default: false
-        extra: "Requires macvlan network mode"
+        extra: "Exposes port 67 for DHCP in bridge network mode"
   traefik:
     vars:
       traefik_host:
@@ -82,8 +82,12 @@ spec:
     vars:
       network_name:
         default: "pihole_network"
+      network_external:
+        default: false
   ports:
     vars:
+      ports_enabled:
+        extra: "Only required in bridge network mode without Traefik"
       ports_http:
         type: int
         default: 8080