Add feature removal validation and removability metadata

Co-Authored-By: Claude <noreply@anthropic.com>

Author: ehelms

src/features.yaml

@@ -107,6 +107,7 @@ container-gateway:
     - foreman
 bmc:
   description: Power management for bare metal hosts (IPMI, Redfish)
+  removable: true
   foreman_proxy:
     plugin_name: bmc
 webhooks:
@@ -116,10 +117,12 @@ webhooks:
   hammer: foreman_webhooks
 templates:
   description: Templates feature for foreman-proxy
+  removable: true
   foreman_proxy:
     plugin_name: templates
 registration:
   description: Host registration feature for foreman-proxy
+  removable: true
   foreman_proxy:
     plugin_name: registration
   dependencies:

src/filter_plugins/foremanctl.py

@@ -71,32 +71,47 @@ def available_foreman_plugins(_value):
     return compact_list(plugins)
 
 
-def list_all_features(enabled_features, only_enabled=False):
+def list_all_features(enabled_features, only_enabled=False, flavor_features=None):
     enabled_list = []
     available_list = []
     list_internal = os.environ.get('FOREMANCTL_FEATURES_LIST_INTERNAL', '') == 'true'
+    flavor_features_set = set(flavor_features or [])
+
     for name, meta in FEATURE_MAP.items():
         internal = meta.get('internal', False)
         if internal and not list_internal:
             continue
         description = meta.get('description', '')
+
+        if name in flavor_features_set:
+            removable = 'flavor'
+        elif meta.get('removable', False):
+            removable = 'yes'
+        else:
+            removable = 'no'
+
         if has_feature(enabled_features, name):
-            enabled_list.append((name, 'enabled', internal, description))
+            enabled_list.append((name, 'enabled', internal, removable, description))
         elif not only_enabled:
-            available_list.append((name, 'available', internal, description))
+            available_list.append((name, 'available', internal, removable, description))
 
     if not list_internal:
-        output = [f"{'FEATURE':<25} {'STATE':<12} DESCRIPTION"]
-        for name, state, _internal, description in enabled_list + available_list:
-            output.append(f"{name:<25} {state:<12} {description}")
+        output = [f"{'FEATURE':<25} {'STATE':<12} {'REMOVABLE':<13} DESCRIPTION"]
+        for name, state, _internal, removable, description in enabled_list + available_list:
+            output.append(f"{name:<25} {state:<12} {removable:<13} {description}")
     else:
-        output = [f"{'FEATURE':<25} {'STATE':<12} {'INTERNAL':<8} DESCRIPTION"]
-        for name, state, internal, description in enabled_list + available_list:
-            output.append(f"{name:<25} {state:<12} {internal:<8} {description}")
+        output = [f"{'FEATURE':<25} {'STATE':<12} {'INTERNAL':<8} {'REMOVABLE':<13} DESCRIPTION"]
+        for name, state, internal, removable, description in enabled_list + available_list:
+            output.append(f"{name:<25} {state:<12} {internal:<8} {removable:<13} {description}")
 
     return "\n".join(output)
 
 
+def is_feature_removable(feature_name):
+    """Check if a feature supports removal."""
+    return FEATURE_MAP.get(feature_name, {}).get('removable', False)
+
+
 def invalid_features(features):
     """Return a list of unknown features not defined in features.yaml."""
     return [feature for feature in features if feature not in FEATURE_MAP]
@@ -112,6 +127,59 @@ def conflicting_features(features):
     return [f"{pair[0]} conflicts with {pair[1]}" for pair in conflicts]
 
 
+def validate_feature_removals(remove_features, flavor_features):
+    """Validate that requested feature removals are allowed.
+
+    Returns a list of error message strings. Empty list means all valid.
+    """
+    errors = []
+
+    for feature in remove_features:
+        if feature in flavor_features:
+            errors.append(
+                f"Cannot remove '{feature}' — it is a core feature of the current flavor. "
+                f"Flavor features cannot be removed."
+            )
+        elif feature not in FEATURE_MAP:
+            errors.append(
+                f"Cannot remove unknown feature '{feature}'. "
+                f"Run 'foremanctl features' to see available features."
+            )
+        elif not is_feature_removable(feature):
+            errors.append(
+                f"Cannot remove feature '{feature}' — this feature does not support removal. "
+                f"Run 'foremanctl features' to see which features can be removed."
+            )
+
+    return errors
+
+
+def unsatisfied_dependencies(enabled_features, remove_features=None):
+    """Detect removed features that are still required by an enabled feature.
+
+    ``enabled_features`` is the effective feature list (flavor + user features
+    with removals already subtracted). ``remove_features`` are the features the
+    user asked to remove. Because dependencies are auto-included, a dependency
+    is only ever "unsatisfied" when the user explicitly removes something that
+    a still-enabled feature depends on.
+
+    Returns a list of error strings; an empty list means all removals are safe.
+    """
+    remove_set = set(remove_features or [])
+    if not remove_set:
+        return []
+
+    errors = []
+    for feature in enabled_features:
+        still_required = get_dependencies_for_feature(feature) & remove_set
+        for dependency in sorted(still_required):
+            errors.append(
+                f"Cannot remove '{dependency}' — it is required by enabled feature '{feature}'"
+            )
+
+    return errors
+
+
 def hammer_plugins(value):
     dependencies = list(get_dependencies(filter_features(value)))
     features = set(filter_features(value + dependencies))
@@ -164,6 +232,8 @@ def filters(self):
             'list_all_features': list_all_features,
             'invalid_features': invalid_features,
             'conflicting_features': conflicting_features,
+            'validate_feature_removals': validate_feature_removals,
+            'unsatisfied_dependencies': unsatisfied_dependencies,
             'has_feature': has_feature,
             'databases_for_features': databases_for_features,
             'to_postgresql_databases': to_postgresql_databases,

src/playbooks/_flavor_features/metadata.obsah.yaml

@@ -6,6 +6,6 @@ variables:
     action: append_unique
   remove_features:
     parameter: --remove-feature
-    help: Additional features to disable in this deployment.
-    action: remove
-    dest: features
+    help: Features to remove from this deployment.
+    action: append_unique
+    persist: false

src/roles/check_features/tasks/main.yaml

@@ -1,26 +1,69 @@
 ---
+- name: Validate feature removal requests
+  ansible.builtin.assert:
+    that:
+      - check_features_removal_errors | length == 0
+    fail_msg: |
+      ERROR: Invalid feature removal request:
+      {% for error in check_features_removal_errors %}
+        - {{ error }}
+      {% endfor %}
+  vars:
+    check_features_removal_errors: "{{ remove_features | validate_feature_removals(flavor_features) }}"
+  when: remove_features | length > 0
+
+- name: Validate feature dependencies
+  ansible.builtin.assert:
+    that:
+      - check_features_dependency_errors | length == 0
+    fail_msg: |
+      ERROR: Removing features would break dependencies:
+      {% for error in check_features_dependency_errors %}
+        - {{ error }}
+      {% endfor %}
+  vars:
+    check_features_dependency_errors: "{{ enabled_features | unsatisfied_dependencies(remove_features) }}"
+  when: remove_features | length > 0
+
 - name: Validate requested features
   ansible.builtin.assert:
     that:
-      - found_invalid_features | length == 0
+      - check_features_invalid | length == 0
     fail_msg: |
-      ERROR: Unknown feature(s) requested: {{ found_invalid_features | join(', ') }}
+      ERROR: Unknown feature(s) requested: {{ check_features_invalid | join(', ') }}
 
       Run 'foremanctl features' to list all available features.
   vars:
-    found_invalid_features: "{{ features | invalid_features }}"
+    check_features_invalid: "{{ features | invalid_features }}"
   when: features | length > 0
 
 - name: Validate feature conflicts
   ansible.builtin.assert:
     that:
-      - found_conflicts | length == 0
+      - check_features_conflicts | length == 0
     fail_msg: |
       ERROR: Conflicting features detected:
-      {% for conflict in found_conflicts %}
+      {% for conflict in check_features_conflicts %}
         - {{ conflict }}
       {% endfor %}
 
       These features cannot be enabled together.
   vars:
-    found_conflicts: "{{ enabled_features | conflicting_features }}"
+    check_features_conflicts: "{{ enabled_features | conflicting_features }}"
+
+- name: Persist feature removals
+  when: remove_features | length > 0
+  block:
+    - name: Read current persisted parameters
+      ansible.builtin.slurp:
+        src: "{{ lookup('env', 'OBSAH_STATE') }}/parameters.yaml"
+      register: check_features_persisted_params_raw
+
+    - name: Write updated parameters
+      ansible.builtin.copy:
+        content: "{{ check_features_current_params | combine({'features': check_features_updated_features}) | to_nice_yaml }}"
+        dest: "{{ lookup('env', 'OBSAH_STATE') }}/parameters.yaml"
+        mode: "0644"
+      vars:
+        check_features_current_params: "{{ check_features_persisted_params_raw.content | b64decode | from_yaml }}"
+        check_features_updated_features: "{{ (check_features_current_params.features | default([])) | difference(remove_features) }}"

src/vars/defaults.yml

@@ -3,4 +3,5 @@ certificates_source: default
 database_mode: internal
 tuning: default
 features: []
-enabled_features: "{{ (flavor_features + features) }}"
+remove_features: []
+enabled_features: "{{ (flavor_features + features) | difference(remove_features) }}"

tests/unit/filter_test.py

@@ -4,6 +4,7 @@
 from foremanctl import foreman_proxy_plugins
 from foremanctl import hammer_plugins
 from foremanctl import list_all_features
+from foremanctl import unsatisfied_dependencies
 
 
 def _asymmetric_conflicts():
@@ -68,6 +69,38 @@ def test_list_all_features_marks_dependency_as_enabled(monkeypatch):
     assert 'enabled' in child_line
 
 
+def test_unsatisfied_dependencies_none_when_nothing_removed():
+    assert unsatisfied_dependencies(['foreman', 'katello'], []) == []
+
+
+def test_unsatisfied_dependencies_ignores_transitive_deps():
+    # httpd/valkey/dynflow/tasks are never listed explicitly; with no removals
+    # requested this must not report them as missing.
+    assert unsatisfied_dependencies(['foreman', 'katello', 'pulp']) == []
+
+
+def test_unsatisfied_dependencies_detects_removed_dependency(monkeypatch):
+    monkeypatch.setitem(FEATURE_MAP, 'test-parent', {'dependencies': ['test-child']})
+    monkeypatch.setitem(FEATURE_MAP, 'test-child', {})
+    result = unsatisfied_dependencies(['test-parent'], ['test-child'])
+    assert result == ["Cannot remove 'test-child' — it is required by enabled feature 'test-parent'"]
+
+
+def test_unsatisfied_dependencies_detects_transitively_removed_dependency(monkeypatch):
+    monkeypatch.setitem(FEATURE_MAP, 'test-parent', {'dependencies': ['test-mid']})
+    monkeypatch.setitem(FEATURE_MAP, 'test-mid', {'dependencies': ['test-leaf']})
+    monkeypatch.setitem(FEATURE_MAP, 'test-leaf', {})
+    result = unsatisfied_dependencies(['test-parent'], ['test-leaf'])
+    assert any("Cannot remove 'test-leaf'" in error for error in result)
+
+
+def test_unsatisfied_dependencies_allows_unrelated_removal(monkeypatch):
+    monkeypatch.setitem(FEATURE_MAP, 'test-parent', {'dependencies': ['test-child']})
+    monkeypatch.setitem(FEATURE_MAP, 'test-child', {})
+    monkeypatch.setitem(FEATURE_MAP, 'test-other', {})
+    assert unsatisfied_dependencies(['test-parent'], ['test-other']) == []
+
+
 def test_foreman_plugins_deduplicates(monkeypatch):
     monkeypatch.setitem(FEATURE_MAP, 'test-parent', {
         'dependencies': ['test-child'],