Pass transient operands for persisted removals

docs/source/development.rst

@@ -305,6 +305,31 @@ The ``dest`` field (`argparse dest`_) allows multiple parameters to modify the s
 
 This allows both ``--add-option`` and ``--remove-option`` to modify the same ``options`` variable.
 
+When a ``remove`` variable has a distinct ``dest``, Obsah also exposes the
+operands supplied to that parameter under the source variable name. This is
+useful when the playbook needs to validate the requested operations while the
+destination contains the final value:
+
+.. code-block:: yaml
+
+    variables:
+      options:
+        parameter: --add-option
+        action: append_unique
+      remove_options:
+        parameter: --remove-option
+        action: remove
+        dest: options
+        persist: false
+
+The playbook receives both ``options`` (the final list) and
+``remove_options`` (the unique values explicitly supplied on the command
+line). ``persist: false`` applies to the source operation list; the ``options``
+destination is still persisted. A remove variable may also set ``effective_dest``
+to expose only operands that actually removed an existing item. This lets a
+playbook distinguish an idempotent removal no-op from a feature that was
+removed during this invocation.
+
 Parameter Persistence
 """""""""""""""""""""
 
@@ -320,7 +345,9 @@ When ``OBSAH_PERSIST_PARAMS`` is enabled, parameter values are saved between run
         help: One-time flag
         persist: false  # Value does not persist
 
-Persisted parameters are marked with ``(persisted)`` in the help output and can be reset using ``--reset-<parameter-name>``.
+Persisted parameters are marked with ``(persisted)`` in the help output and can be reset using ``--reset-<parameter-name>``. Non-persistent variables are not loaded from an existing state file, so a one-shot operation cannot be replayed accidentally on a later command.
+
+Parameters are written only after a successful playbook run.
 
 Constraints
 ^^^^^^^^^^^

obsah/__init__.py

@@ -35,7 +35,7 @@
 display = None  # pylint: disable=C0103
 
 
-Variable = namedtuple('Variable', ['name', 'parameter', 'help_text', 'action', 'type', 'choices', 'dest', 'persist'])
+Variable = namedtuple('Variable', ['name', 'parameter', 'help_text', 'action', 'type', 'choices', 'dest', 'persist', 'effective_dest'])
 
 
 class AppendUniqueAction(argparse.Action):
@@ -64,10 +64,16 @@ class RemoveAction(argparse.Action):
     """
     Custom argparse action that removes values from a list.
     Useful when you have a default list and want to selectively remove items.
+
+    ``record_dest`` records all requested operands under the source variable
+    name. ``effective_dest`` records only operands that removed an existing
+    item, so playbooks can distinguish real removals from idempotent no-ops.
     """
-    def __init__(self, option_strings, dest, default=None, **kwargs):
+    def __init__(self, option_strings, dest, default=None, record_dest=None, effective_dest=None, **kwargs):
         if default is None:
             default = []
+        self.record_dest = record_dest
+        self.effective_dest = effective_dest
         super().__init__(option_strings, dest, default=default, **kwargs)
 
     def __call__(self, parser, namespace, values, option_string=None):
@@ -77,11 +83,23 @@ def __call__(self, parser, namespace, values, option_string=None):
         else:
             # Create a new list to avoid modifying the default
             items = list(items)
-            # Remove the value if it exists in the list
-            if values in items:
-                items.remove(values)
+        was_present = values in items
+        if was_present:
+            items.remove(values)
         setattr(namespace, self.dest, items)
 
+        if self.record_dest is not None:
+            recorded = list(getattr(namespace, self.record_dest, None) or [])
+            if values not in recorded:
+                recorded.append(values)
+            setattr(namespace, self.record_dest, recorded)
+
+        if self.effective_dest is not None:
+            removed = list(getattr(namespace, self.effective_dest, None) or [])
+            if was_present and values not in removed:
+                removed.append(values)
+            setattr(namespace, self.effective_dest, removed)
+
 
 def _merge_constraints(base, other):
     merged = dict(base)
@@ -225,7 +243,11 @@ def _parse_parameters(self, variables: Mapping[str, Mapping[str, str]]) -> Itera
             except KeyError:
                 parameter = '--{}'.format(name.removeprefix(namespace).replace('_', '-'))
 
-            yield Variable(name, parameter, options.get('help'), options.get('action'), options.get('type'), options.get('choices'), options.get('dest'), options.get('persist', True))
+            yield Variable(
+                name, parameter, options.get('help'), options.get('action'), options.get('type'),
+                options.get('choices'), options.get('dest'), options.get('persist', True),
+                options.get('effective_dest'),
+            )
 
     @property
     def __doc__(self):
@@ -387,6 +409,14 @@ def format_usage(self):
         return super().format_help()
 
 
+def _load_persisted_params(path):
+    """Load persisted parameters, returning an empty mapping when absent."""
+    with contextlib.suppress(FileNotFoundError):
+        with open(path) as persist_file:
+            return yaml.safe_load(persist_file) or {}
+    return {}
+
+
 def find_targets(inventory_path) -> Optional[Iterable]:
     """
     Find all targets in the given inventory
@@ -461,11 +491,26 @@ def obsah_argument_parser(application_config=ApplicationConfig, playbooks=None,
         subparser.set_defaults(playbook=playbook)
         if application_config.persist_params():
             subparser.epilog = "Parameters marked as (persisted) can be reset by --reset-<parameter-name>."
-            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)
+            persist_params = _load_persisted_params(application_config.persist_path())
+            non_persistent = {variable.name for variable in playbook.playbook_variables if not variable.persist}
+            non_persistent.update(
+                variable.effective_dest
+                for variable in playbook.playbook_variables
+                if variable.effective_dest
+            )
+            stale_non_persistent = sorted(non_persistent.intersection(persist_params))
+            if stale_non_persistent:
+                print(
+                    "Warning: ignoring non-persistent parameter(s) loaded from state: {}; "
+                    "they will be removed after a successful command.".format(
+                        ', '.join(stale_non_persistent)
+                    ),
+                    file=sys.stderr,
+                )
+            for variable_name in non_persistent:
+                persist_params.pop(variable_name, None)
+            if persist_params:
+                subparser.set_defaults(**persist_params)
 
         if playbook.takes_target_parameter:
             subparser.add_argument('target',
@@ -484,13 +529,19 @@ def obsah_argument_parser(application_config=ApplicationConfig, playbooks=None,
                 argument_args['choices'] = variable.choices
             if variable.parameter.startswith('--'):
                 argument_args['dest'] = variable.dest or variable.name
+                if variable.action == 'remove' and variable.dest and variable.dest != variable.name:
+                    argument_args['record_dest'] = variable.name
+                    argument_args['effective_dest'] = variable.effective_dest
 
             if application_config.persist_params() and variable.parameter.startswith('--') and variable.persist:
                 base_help = argument_args.get('help') or ''
                 argument_args['help'] = f"{base_help} (persisted)".strip()
 
             subparser.add_argument(variable.parameter, **argument_args)
             parser.obsah_arguments.append(variable.name)
+            if variable.effective_dest:
+                parser.obsah_arguments.append(variable.effective_dest)
+                parser.obsah_dont_persist.add(variable.effective_dest)
 
             if application_config.persist_params():
                 if variable.parameter.startswith('--') and variable.persist:
@@ -540,33 +591,27 @@ def generate_ansible_args(inventory_path, args, obsah_arguments):
 
 
 def reset_args(application_config: ApplicationConfig, metadata: dict, args: argparse.Namespace):
-    with contextlib.suppress(FileNotFoundError):
-        with open(application_config.persist_path()) as persist_file:
-            persist_params = yaml.safe_load(persist_file)
-        if persist_params:
-            for (reset_key, reset_values) in metadata['reset']:
-                if reset_key in persist_params and persist_params.get(reset_key) != getattr(args, reset_key):
-                    for arg in reset_values:
-                        if arg in persist_params and persist_params[arg] == getattr(args, arg):
-                            delattr(args, arg)
-            for reset_arg in (getattr(args, 'obsah_reset', None) or []):
-                with contextlib.suppress(AttributeError):
-                    delattr(args, reset_arg)
+    persist_params = _load_persisted_params(application_config.persist_path())
+    if persist_params:
+        for (reset_key, reset_values) in metadata['reset']:
+            if reset_key in persist_params and persist_params.get(reset_key) != getattr(args, reset_key):
+                for arg in reset_values:
+                    if arg in persist_params and persist_params[arg] == getattr(args, arg):
+                        delattr(args, arg)
+        for reset_arg in (getattr(args, 'obsah_reset', None) or []):
+            with contextlib.suppress(AttributeError):
+                delattr(args, reset_arg)
     return args
 
 
 def persist_args(application_config, args, dont_persist):
-    persist_dir = os.path.dirname(application_config.persist_path())
+    persist_path = application_config.persist_path()
+    persist_dir = os.path.dirname(persist_path)
     if not os.path.exists(persist_dir):
         os.makedirs(persist_dir, mode=0o770, exist_ok=True)
 
-    try:
-        with open(application_config.persist_path()) as persist_file:
-            persist_params = yaml.safe_load(persist_file)
-    except FileNotFoundError:
-        persist_params = {}
-
-    with open(application_config.persist_path(), 'w') as persist_file:
+    persist_params = _load_persisted_params(persist_path)
+    with open(persist_path, 'w') as persist_file:
         persist_params.update(dict(vars(args)))
         for item in dont_persist:
             persist_params.pop(item, None)

setup.py

@@ -34,7 +34,7 @@ def find_package_data(package, data_dir):
 
 setup(
     name='obsah',
-    version='1.11.0',
+    version='1.12.0',
     license='GPL-2.0-only',
     description='packaging wrapper using ansible',
     long_description=get_long_description(),

tests/fixtures/playbooks/dummy/metadata.obsah.yaml

@@ -25,3 +25,14 @@ variables:
     parameter: --my-list
     action: append
     help: Repeatable action
+  features:
+    parameter: --add-feature
+    action: append_unique
+    help: Add a feature
+  remove_features:
+    parameter: --remove-feature
+    action: remove
+    dest: features
+    persist: false
+    effective_dest: removed_features
+    help: Remove a feature

tests/test_argparse.py

@@ -58,6 +58,50 @@ def test_remove_nonexistent_item(self):
 
         assert args.exclude == ['foo', 'bar']
 
+    def test_remove_records_raw_and_effective_operands(self):
+        parser = argparse.ArgumentParser()
+        parser.add_argument(
+            '--remove-option',
+            action=RemoveAction,
+            dest='options',
+            record_dest='remove_options',
+            effective_dest='removed_options',
+            default=['foo', 'bar'],
+        )
+
+        args = parser.parse_args([
+            '--remove-option', 'bar',
+            '--remove-option', 'missing',
+            '--remove-option', 'bar',
+            '--remove-option', 'foo',
+        ])
+
+        assert args.options == []
+        assert args.remove_options == ['bar', 'missing', 'foo']
+        assert args.removed_options == ['bar', 'foo']
+
+    def test_remove_and_append_apply_in_cli_order(self):
+        parser = ObsahArgumentParser()
+        parser.add_argument('--add-option', action='append_unique', dest='options', default=[])
+        parser.add_argument(
+            '--remove-option',
+            action='remove',
+            dest='options',
+            record_dest='remove_options',
+            effective_dest='removed_options',
+            default=[],
+        )
+
+        args = parser.parse_args([
+            '--add-option', 'foo',
+            '--remove-option', 'foo',
+            '--add-option', 'foo',
+        ])
+
+        assert args.options == ['foo']
+        assert args.remove_options == ['foo']
+        assert args.removed_options == ['foo']
+
 
 class TestActionStringNames:
     """Test that custom actions can be used via string names"""

tests/test_obsah.py

@@ -121,6 +121,38 @@ def test_generate_ansible_args(playbooks_path, parser, cliargs, expected):
     assert ansible_args == base_expected + expected
 
 
+def test_remove_destination_exposes_raw_and_effective_operands(playbooks_path, parser):
+    args = parser.parse_args([
+        'dummy', 'testpackage',
+        '--add-feature', 'foo',
+        '--remove-feature', 'foo',
+        '--remove-feature', 'bar',
+        '--add-feature', 'bar',
+    ])
+
+    assert args.features == ['bar']
+    assert args.remove_features == ['foo', 'bar']
+    assert args.removed_features == ['foo']
+
+    ansible_args = obsah.generate_ansible_args('inventory.yml', args, parser.obsah_arguments)
+    assert ansible_args[-2:] == [
+        '-e',
+        '{"features": ["bar"], "obsah_state_path": "/var/lib/obsah", "remove_features": ["foo", "bar"], "removed_features": ["foo"]}',
+    ]
+
+
+def test_remove_absent_item_exposes_empty_effective_removals(parser):
+    args = parser.parse_args([
+        'dummy', 'testpackage', '--remove-feature', 'not-present',
+    ])
+
+    assert args.remove_features == ['not-present']
+    assert args.removed_features == []
+
+    ansible_args = obsah.generate_ansible_args('inventory.yml', args, parser.obsah_arguments)
+    assert '"removed_features": []' in ansible_args[-1]
+
+
 @pytest.mark.parametrize("base,other,expected", [
     ({}, {}, {}),
     ({'required_if': [['a', 1, ['b']]]}, {}, {'required_if': [['a', 1, ['b']]]}),

tests/test_parameter_persistence.py

@@ -118,6 +118,37 @@ def test_params_persisted(self, persist_file, persist_application_config):
         assert params['automatic'] == 'foo'
         assert params['mapped'] == 'bar'
 
+    def test_remove_destination_is_persisted_but_raw_operands_are_not(
+        self, write_persist, persist_file, persist_application_config
+    ):
+        write_persist({
+            'features': ['foo', 'bar'],
+            'remove_features': ['stale'],
+            'removed_features': ['stale'],
+            'automatic': 'preserved',
+        })
+
+        parser = obsah.obsah_argument_parser(persist_application_config, targets=['testpackage'])
+        args = parser.parse_args(['dummy', 'testpackage', '--remove-feature', 'foo'])
+
+        assert args.features == ['bar']
+        assert args.remove_features == ['foo']
+        assert args.removed_features == ['foo']
+
+        obsah.persist_args(persist_application_config, args, parser.obsah_dont_persist)
+        params = yaml.safe_load(persist_file.read_text())
+        assert params == {'automatic': 'preserved', 'features': ['bar']}
+
+    def test_remove_from_missing_state_creates_only_destination(
+        self, persist_file, persist_application_config
+    ):
+        parser = obsah.obsah_argument_parser(persist_application_config, targets=['testpackage'])
+        args = parser.parse_args(['dummy', 'testpackage', '--remove-feature', 'foo'])
+
+        obsah.persist_args(persist_application_config, args, parser.obsah_dont_persist)
+        params = yaml.safe_load(persist_file.read_text())
+        assert params == {'features': []}
+
     def test_dont_persist_filtered(self, persist_file, persist_application_config):
         args = argparse.Namespace(automatic='foo', mapped='bar')
         obsah.persist_args(persist_application_config, args, {'mapped'})