obsah/__init__.py (2)

260-265: Persist-path should respect per-user state locations

Hard-coding /var/lib/obsah/parameters.yaml limits usability for non-root users (e.g. CI pipelines, local development).
Consider falling back to XDG_STATE_HOME (or $HOME/.local/state) when OBSAH_PERSIST_PATH is unset.

-        return os.environ.get('OBSAH_PERSIST_PATH', '/var/lib/obsah/parameters.yaml')
+        default_dir = (
+            os.environ.get('XDG_STATE_HOME')
+            or os.path.join(os.path.expanduser('~'), '.local', 'state')
+        )
+        return os.environ.get(
+            'OBSAH_PERSIST_PATH',
+            os.path.join(default_dir, 'obsah', 'parameters.yaml'),
+        )

---

366-373: Positional parameters are silently excluded from persistence

When variable.parameter is positional (no leading --), the code adds the
variable name to obsah_dont_persist, effectively making positional
arguments never persistable. This is intentional for target, but it may
surprise users for other positionals.

If the design goal is never to persist positionals, add a short comment to
make this explicit; otherwise consider introducing an opt-in flag in metadata
(e.g. persist: true) to allow persistence where desired.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

obsah/__init__.py (1)

408-427: No mechanism to handle mutually exclusive parameters.

As mentioned in the PR objectives, there's no mechanism to automatically unset mutually exclusive parameters when one is set. While the reset rules in metadata help address this for some cases, it's still a limitation that should be documented.

Consider adding documentation for users explaining this limitation and how to properly manage parameters that could conflict with each other. Alternatively, enhancing the constraints system to explicitly model mutually exclusive parameters could be a future improvement.

Also applies to: 461-470

🧰 Tools

🪛 Ruff (0.11.9)

420-423: Use contextlib.suppress(AttributeError) instead of try-except-pass

Replace with contextlib.suppress(AttributeError)

(SIM105)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

obsah/__init__.py (1)

344-351: 🛠️ Refactor suggestion

Persisted parameters bypass argparse type validation

When loading parameters from the YAML file, the code uses set_defaults() which bypasses argparse's type validation. This means invalid values could be loaded without proper conversion or validation.

Consider converting the persisted parameters to CLI-style arguments and re-parsing them:

        if application_config.persist_params():
            with contextlib.suppress(FileNotFoundError):
                with open(application_config.persist_path()) as persist_file:
                    persist_params = yaml.safe_load(persist_file)
                if persist_params:
-                    subparser.set_defaults(**persist_params)
+                    # Convert persisted params to CLI args to ensure type validation
+                    cli_args = []
+                    for key, value in persist_params.items():
+                        if value is not None:  # Skip None values
+                            cli_args.extend([f"--{key.replace('_', '-')}", str(value)])
+                    
+                    # Parse the CLI args to validate and convert types
+                    parsed_defaults, _ = subparser.parse_known_args(cli_args)
+                    subparser.set_defaults(**vars(parsed_defaults))

obsah/__init__.py (3)

458-459: Consider more secure directory permissions

The current permissions (0o770) allow group members to write to the persistence directory. For sensitive parameters, a more restrictive mode like 0o700 might be more appropriate.

        if not os.path.exists(persist_dir):
-            os.makedirs(persist_dir, mode=0o770, exist_ok=True)
+            os.makedirs(persist_dir, mode=0o700, exist_ok=True)

---

372-378: Add a mechanism to reset all persisted parameters

The code provides a way to reset individual parameters but lacks a mechanism to reset all persisted parameters at once, which would be useful for troubleshooting.

Add a global reset parameter:

        if application_config.persist_params() and variable.parameter.startswith('--'):
            reset_param = variable.parameter.replace('--', '--reset-')
            subparser.add_argument(reset_param, help=f'Reset {variable.name}', action='append_const', dest='obsah_reset', const=variable.name)
        elif application_config.persist_params():
            # Don't persist positional arguments
            parser.obsah_dont_persist.add(variable.name)

+    # Add a global reset parameter to reset all persisted parameters
+    if application_config.persist_params():
+        subparser.add_argument('--reset-all', help='Reset all persisted parameters', action='store_true')

Then update the reset_args function to handle this case:

    if application_config.persist_params():
        args = reset_args(application_config, args.playbook.metadata, args)
+        # Handle --reset-all flag
+        if getattr(args, 'reset_all', False):
+            # Create an empty file to reset all parameters
+            with open(application_config.persist_path(), 'w') as persist_file:
+                yaml.safe_dump({}, persist_file)
+            # Reset the args object to remove persisted values
+            for attr in dir(args):
+                if not attr.startswith('_') and attr not in parser.obsah_dont_persist:
+                    with contextlib.suppress(AttributeError):
+                        delattr(args, attr)

---

456-465: Consider storing parameters per playbook

The current implementation stores all parameters in a single file, which could lead to parameter conflicts when different playbooks have parameters with the same name but different semantics.

Consider storing parameters per playbook:

    if application_config.persist_params():
        persist_dir = os.path.dirname(application_config.persist_path())
        if not os.path.exists(persist_dir):
            os.makedirs(persist_dir, mode=0o770, exist_ok=True)
-        with open(application_config.persist_path(), 'w') as persist_file:
+        # Use playbook-specific file
+        playbook_persist_path = os.path.join(persist_dir, f"{args.playbook.name}_parameters.yaml")
+        with open(playbook_persist_path, 'w') as persist_file:
            persist_params = dict(vars(args))
            for item in parser.obsah_dont_persist:
                persist_params.pop(item, None)
            yaml.safe_dump(persist_params, persist_file)

And update the loading code as well:

    @classmethod
    def persist_path(cls):
        """
        Where to persist parameters to
        """
-        return os.path.join(cls.state_path(), 'parameters.yaml')
+        return os.path.join(cls.state_path(), 'parameters.yaml')  # Default path

        if application_config.persist_params():
+            # Use playbook-specific file if it exists
+            playbook_persist_path = os.path.join(os.path.dirname(application_config.persist_path()), 
+                                               f"{playbook.name}_parameters.yaml")
+            persist_file_path = playbook_persist_path if os.path.exists(playbook_persist_path) else application_config.persist_path()
            with contextlib.suppress(FileNotFoundError):
-                with open(application_config.persist_path()) as persist_file:
+                with open(persist_file_path) as persist_file:
                    persist_params = yaml.safe_load(persist_file)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro