From df5806db87c5fb94b703a5313357715741b3ea6e Mon Sep 17 00:00:00 2001 From: "Eric D. Helms" Date: Mon, 10 Aug 2026 15:06:03 -0400 Subject: [PATCH] Add feature removal validation and removability metadata Co-Authored-By: Claude --- docs/developer/checks.md | 6 +- docs/developer/deployment.md | 15 +++ docs/developer/feature-metadata.md | 1 + docs/developer/playbooks-and-roles.md | 5 +- docs/user/parameters.md | 3 + src/features.yaml | 3 + src/filter_plugins/foremanctl.py | 92 ++++++++++++++++-- .../_flavor_features/metadata.obsah.yaml | 3 +- src/playbooks/features/features.yaml | 2 +- src/playbooks/pull-images/pull-images.yaml | 1 + src/requirements.txt | 2 +- src/roles/check_features/tasks/main.yaml | 38 ++++++-- src/vars/defaults.yml | 1 + tests/conftest.py | 31 +++++-- tests/features_test.py | 18 ++++ tests/flavor/katello/feature_removal_test.py | 54 +++++++++++ tests/unit/check_role_test.py | 18 ++++ tests/unit/filter_test.py | 93 +++++++++++++++++++ 18 files changed, 356 insertions(+), 30 deletions(-) create mode 100644 tests/flavor/katello/feature_removal_test.py diff --git a/docs/developer/checks.md b/docs/developer/checks.md index 772495980..5dd145f8d 100644 --- a/docs/developer/checks.md +++ b/docs/developer/checks.md @@ -15,9 +15,9 @@ Please update this file as check usage evolves. - **Rationale**: A validation was incorrectly removed which prevented users from creating duplicate Foreman permissions, causing upgrade failure. This check will need to be included until https://projects.theforeman.org/issues/38465 is addressed. ### check_features -- **Description**: Ensures that all foremanctl features requested (via the `--feature` flag) are valid. -- **Fail state**: Fails when requested features are not recognized by foremanctl. -- **Rationale**: This check handles input sanitization for `--feature`. +- **Description**: Ensures that all foremanctl features requested via `--add-feature` and `--remove-feature` are valid and that removals do not violate flavor or dependency rules. +- **Fail state**: Fails when a requested feature is unknown, non-removable, protected by the current flavor, or required by another enabled feature. +- **Rationale**: This check handles feature input sanitization. Feature state persistence remains owned by `obsah` and is committed only after the complete playbook succeeds. ### check_foreman_api - **Description**: Pings Foreman API (/api/v2/ping) to verify it responds. Verifies foreman_tasks service status is 'ok' when Katello/content feature is enabled. diff --git a/docs/developer/deployment.md b/docs/developer/deployment.md index 77dcab024..d7aadd7c1 100644 --- a/docs/developer/deployment.md +++ b/docs/developer/deployment.md @@ -100,6 +100,21 @@ These base features control which plugins are enabled when a feature is requeste A deployment can have multiple base features enabled. +### Adding and removing features + +Feature selection is persisted in the `features` list in `parameters.yaml`. Add and remove flags operate on that same list, so a removal is durable without creating a separate deny-list; Obsah saves the resulting list after the playbook succeeds: + +```bash +./foremanctl deploy --add-feature bmc +./foremanctl deploy --remove-feature bmc +``` + +The second command removes `bmc` from the persisted user list. Running the first command again re-adds it. Repeated operations are idempotent; for a removable, non-flavor feature named by both flags, the last flag in command-line order determines whether it remains in the final list. + +A known removable feature that is already absent is an idempotent no-op. Unknown features, features without `removable: true`, flavor features, and features required by an enabled direct or transitive parent are rejected. Flavor features are baseline capabilities and cannot be disabled with `--remove-feature`; use a different flavor instead. + +Obsah updates the feature parameter file only after the playbook returns success. Validation or playbook failures leave the previous feature list in place; state-file write errors are reported by foremanctl. + ### Enabling IOP IOP (Insights Operating Platform) deploys on-premise Insights services for advisor, vulnerability, and remediation. It requires internal database mode and depends on the `rh-cloud` and `katello` features. diff --git a/docs/developer/feature-metadata.md b/docs/developer/feature-metadata.md index 1446d9577..19cfe48dc 100644 --- a/docs/developer/feature-metadata.md +++ b/docs/developer/feature-metadata.md @@ -20,6 +20,7 @@ feature_two: The following properties are defined: * `description` (_String_): A human-readable description of the feature, can be used in documentation/help output. * `internal` (_Boolean_): Whether the feature is user visible (shows up in documentation/help) or internal (just to perform additional configuration without user interaction). +* `removable` (_Boolean_): Whether users may request removal of the feature. Flavor features are protected and cannot be removed even when this is `true`; omitted or `false` values are not removable. * `foreman` (_Hash_): How this feature should be applied on the "main" system that offers the main user interaction via UI/API. * `plugin_name` (_String_): The name of the Foreman plugin to be enabled (via `FOREMAN_ENABLED_PLUGINS`). If `roles/foreman/tasks/feature/{{ foreman_plugin }}.yaml` exists, it will be executed to perform any plugin-specific setup. diff --git a/docs/developer/playbooks-and-roles.md b/docs/developer/playbooks-and-roles.md index 2176a1e29..570c06a06 100644 --- a/docs/developer/playbooks-and-roles.md +++ b/docs/developer/playbooks-and-roles.md @@ -120,11 +120,14 @@ variables: action: append_unique remove_features: parameter: --remove-feature - help: Additional features to disable in this deployment. + help: Features to remove from the persisted user feature list. action: remove dest: features + persist: false ``` +`features` is the single persisted user feature list. `--remove-feature` mutates that same list during argument parsing, while the distinct `remove_features` value contains only the raw operands for playbook validation. Because it is marked `persist: false`, a removal request is never stored as a second deny-list. A successful removal is saved by the normal `obsah` persistence path; a failed playbook leaves the previous list unchanged. + ### Constraints for validating flag combinations Foremanctl uses `constraints` to enforce rules between related flags: diff --git a/docs/user/parameters.md b/docs/user/parameters.md index 48ea1626d..c0f4d45ea 100644 --- a/docs/user/parameters.md +++ b/docs/user/parameters.md @@ -142,12 +142,15 @@ There are multiple use cases from the users perspective that dictate what parame | `--foreman-fqdn` | FQDN of the Foreman server this proxy connects to | `--foreman-proxy-foreman-base-url` | | `--auth-bundle` | Path to the auth bundle tar file (generated by `foremanctl auth-bundle`) to be used by proxy | `--certs-tar-file` | | `--add-feature bmc` | Enable BMC feature | `--foreman-proxy-bmc` | +| `--remove-feature bmc` | Remove BMC from the persisted user feature list | N/A | | `--add-feature content/ostree` | Enable OSTree content type | `--foreman-proxy-content-enable-ostree` | | `--bmc-ipmi-implementation` | IPMI implementation to use for BMC | `--foreman-proxy-bmc-default-provider` | | `--bmc-redfish-verify-ssl` | Verify SSL certificates for Redfish BMC connections | `--foreman-proxy-bmc-redfish-verify-ssl` | | `--add-feature templates` | Enable Templates feature on Smart Proxy | `--foreman-proxy-templates` | +| `--remove-feature templates` | Remove Templates when it is not a protected flavor feature | N/A | | `--templates-url` | URL that hosts will use to contact the proxy for provisioning templates | `--foreman-proxy-templates-url` | | `--add-feature registration` | Enable Registration feature | `--foreman-proxy-registration` | +| `--remove-feature registration` | Remove Registration when it is not a protected flavor feature | N/A | | `--registration-url` | URL that hosts use to reach the registration endpoint | `--foreman-proxy-registration-url` | ### Unmapped diff --git a/src/features.yaml b/src/features.yaml index e4a96c1e8..8795c85f6 100644 --- a/src/features.yaml +++ b/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: diff --git a/src/filter_plugins/foremanctl.py b/src/filter_plugins/foremanctl.py index d1ab22bd4..e81ab8b8f 100644 --- a/src/filter_plugins/foremanctl.py +++ b/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,63 @@ 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 requested removals without requiring them to be enabled. + + A known removable feature that is already absent is an idempotent no-op. + Flavor features remain protected regardless of their registry metadata. + Returns a list of error message strings. Empty list means all valid. + """ + errors = [] + flavor_features = set(flavor_features or []) + + for feature in remove_features: + if feature not in FEATURE_MAP: + errors.append( + f"Cannot remove unknown feature '{feature}'. " + f"Run 'foremanctl features' to see available features." + ) + elif 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 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 final feature list after command-line add/remove + operations. ``remove_features`` contains the raw removal operands; a feature + re-added later in command-line order is not considered removed. Because + dependencies are auto-included, a dependency is only ever "unsatisfied" + when the user removes it while a still-enabled feature depends on it. + + Returns a list of error strings; an empty list means all removals are safe. + """ + enabled_features = list(enabled_features or []) + remove_set = set(remove_features or []) - set(enabled_features) + 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 +236,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, diff --git a/src/playbooks/_flavor_features/metadata.obsah.yaml b/src/playbooks/_flavor_features/metadata.obsah.yaml index 532a92ddf..67d88bfe5 100644 --- a/src/playbooks/_flavor_features/metadata.obsah.yaml +++ b/src/playbooks/_flavor_features/metadata.obsah.yaml @@ -6,6 +6,7 @@ variables: action: append_unique remove_features: parameter: --remove-feature - help: Additional features to disable in this deployment. + help: Features to remove from the persisted user feature list. action: remove dest: features + persist: false diff --git a/src/playbooks/features/features.yaml b/src/playbooks/features/features.yaml index 12e501c0d..2d05cb611 100644 --- a/src/playbooks/features/features.yaml +++ b/src/playbooks/features/features.yaml @@ -14,4 +14,4 @@ tasks: - name: Print features ansible.builtin.debug: - msg: "{{ enabled_features | list_all_features(list_enabled) }}" + msg: "{{ enabled_features | list_all_features(list_enabled, flavor_features) }}" diff --git a/src/playbooks/pull-images/pull-images.yaml b/src/playbooks/pull-images/pull-images.yaml index fe2a26b8d..5ec137f35 100644 --- a/src/playbooks/pull-images/pull-images.yaml +++ b/src/playbooks/pull-images/pull-images.yaml @@ -11,6 +11,7 @@ - "../../vars/base.yaml" become: true roles: + - role: check_features - role: pre_install post_tasks: - name: Deploy Foreman image units diff --git a/src/requirements.txt b/src/requirements.txt index 9e1528223..11be5ec9d 100644 --- a/src/requirements.txt +++ b/src/requirements.txt @@ -1,3 +1,3 @@ -obsah >= 1.11.0 +obsah @ git+https://github.com/theforeman/obsah.git@refs/pull/140/head requests requests-oauthlib diff --git a/src/roles/check_features/tasks/main.yaml b/src/roles/check_features/tasks/main.yaml index 6d05c1b57..471313cdc 100644 --- a/src/roles/check_features/tasks/main.yaml +++ b/src/roles/check_features/tasks/main.yaml @@ -1,26 +1,52 @@ --- +- 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 }}" diff --git a/src/vars/defaults.yml b/src/vars/defaults.yml index 407b9eebf..9585752d8 100644 --- a/src/vars/defaults.yml +++ b/src/vars/defaults.yml @@ -3,5 +3,6 @@ certificates_source: default database_mode: internal tuning: default features: [] +remove_features: [] enabled_features: "{{ (flavor_features + features) }}" air_gapped: false diff --git a/tests/conftest.py b/tests/conftest.py index 1613b375d..57cd11404 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -31,33 +31,48 @@ class UserParameters: def __init__(self, config): self._config = config + @staticmethod + def _is_internal(value): + return value.lower() in {'1', 'true'} + @cached_property def features(self): # foremanctl outputs - # FEATURE STATE DESCRIPTION - # $feature enabled/available $description + # FEATURE STATE INTERNAL REMOVABLE DESCRIPTION + # $feature enabled/available 0/1 flavor/yes/no $description + # Internal values may be rendered as either numeric or boolean strings. output = subprocess.check_output(['./foremanctl', 'features'], cwd=self._config.rootdir, universal_newlines=True, env=os.environ | {'FOREMANCTL_FEATURES_LIST_INTERNAL': 'true'}) lines = output.splitlines(keepends=False) - # feature, status, internal, description - return [line.split(None, 3) for line in lines[1:]] + # feature, status, internal, removable, description + features = [] + for line in lines[1:]: + fields = line.split(None, 4) + features.append(fields + [''] * (5 - len(fields))) + return features @cached_property def all_available_features(self): - return set(feature for feature, _status, _internal, _desc in self.features) + return set(feature for feature, _status, _internal, _removable, _desc in self.features) @cached_property def available_features(self): - return set(feature for feature, _status, internal, _desc in self.features if internal == '0') + return set( + feature for feature, _status, internal, _removable, _desc in self.features + if not self._is_internal(internal) + ) @cached_property def enabled_features(self): - return set(feature for feature, status, internal, _desc in self.features if status == 'enabled') + return set(feature for feature, status, _internal, _removable, _desc in self.features if status == 'enabled') @cached_property def user_enabled_features(self): - return set(feature for feature, status, internal, _desc in self.features if status == 'enabled' and internal == '0') + return set( + feature for feature, status, internal, _removable, _desc in self.features + if status == 'enabled' and not self._is_internal(internal) + ) @cached_property def flavor(self): diff --git a/tests/features_test.py b/tests/features_test.py index f8fdcf589..f840a33b0 100644 --- a/tests/features_test.py +++ b/tests/features_test.py @@ -1,4 +1,6 @@ +import os import subprocess +from pathlib import Path import pytest @@ -87,6 +89,9 @@ def test_foremanctl_features_list_enabled(user_enabled_features): def test_invalid_feature_rejected(): + parameters_file = Path(os.environ.get('OBSAH_STATE', '.var/lib/foremanctl')) / 'parameters.yaml' + original_state = parameters_file.read_bytes() + command = ['./foremanctl', 'deploy', '--add-feature', 'invalid-feature'] result = subprocess.run(command, capture_output=True, text=True) @@ -94,6 +99,19 @@ def test_invalid_feature_rejected(): assert 'Unknown feature(s) requested: invalid-feature' in result.stdout assert "Run 'foremanctl features' to list all available features." in result.stdout + assert parameters_file.read_bytes() == original_state + + +def test_invalid_feature_removal_rejected_without_persisting(): + parameters_file = Path(os.environ.get('OBSAH_STATE', '.var/lib/foremanctl')) / 'parameters.yaml' + original_state = parameters_file.read_bytes() + + command = ['./foremanctl', 'deploy', '--remove-feature', 'invalid-feature'] + result = subprocess.run(command, capture_output=True, text=True) + + assert result.returncode == 2 + assert "Cannot remove unknown feature 'invalid-feature'" in result.stdout + assert parameters_file.read_bytes() == original_state def test_enabled_features(pytestconfig, enabled_features): diff --git a/tests/flavor/katello/feature_removal_test.py b/tests/flavor/katello/feature_removal_test.py new file mode 100644 index 000000000..76827dd49 --- /dev/null +++ b/tests/flavor/katello/feature_removal_test.py @@ -0,0 +1,54 @@ +import os +import subprocess +from pathlib import Path + +import yaml + +PARAMETERS_FILE = Path(os.environ.get('OBSAH_STATE', '.var/lib/foremanctl')) / 'parameters.yaml' + + +def _deploy(*arguments): + return subprocess.run( + ['./foremanctl', 'deploy', *arguments], + capture_output=True, + text=True, + ) + + +def _features(): + if not PARAMETERS_FILE.exists(): + return [] + with PARAMETERS_FILE.open() as parameters_file: + return (yaml.safe_load(parameters_file) or {}).get('features', []) + + +def test_feature_add_remove_and_readd_persist_across_invocations(): + original_state = PARAMETERS_FILE.read_bytes() if PARAMETERS_FILE.exists() else None + initially_enabled = 'bmc' in _features() + + try: + add_result = _deploy('--add-feature', 'bmc') + assert add_result.returncode == 0, add_result.stderr + assert 'bmc' in _features() + + remove_result = _deploy('--remove-feature', 'bmc') + assert remove_result.returncode == 0, remove_result.stderr + assert 'bmc' not in _features() + + with PARAMETERS_FILE.open() as parameters_file: + persisted = yaml.safe_load(parameters_file) or {} + assert 'remove_features' not in persisted + + readd_result = _deploy('--add-feature', 'bmc') + assert readd_result.returncode == 0, readd_result.stderr + assert 'bmc' in _features() + finally: + try: + if not initially_enabled: + cleanup_result = _deploy('--remove-feature', 'bmc') + assert cleanup_result.returncode == 0, cleanup_result.stderr + finally: + if original_state is None: + PARAMETERS_FILE.unlink(missing_ok=True) + else: + PARAMETERS_FILE.write_bytes(original_state) diff --git a/tests/unit/check_role_test.py b/tests/unit/check_role_test.py index c7ba35990..835e93191 100644 --- a/tests/unit/check_role_test.py +++ b/tests/unit/check_role_test.py @@ -29,3 +29,21 @@ def test_check_foreman_api_has_feature_guards(): def test_check_foreman_tasks_has_feature_guards(): ensure_role_has_feature_guards('check_foreman_tasks', ['tasks']) + + +def test_check_features_does_not_persist_parameters(): + role_path = os.path.join(ROLES_DIR, 'check_features', 'tasks', 'main.yaml') + with open(role_path, 'r') as role_file: + role_text = role_file.read() + + assert 'parameters.yaml' not in role_text + assert 'ansible.builtin.slurp' not in role_text + + +def test_pull_images_validates_feature_changes_before_pre_install(): + playbook_path = os.path.join(SRC_DIR, 'playbooks', 'pull-images', 'pull-images.yaml') + with open(playbook_path, 'r') as playbook_file: + roles = yaml.safe_load(playbook_file)[0]['roles'] + + role_names = [role if isinstance(role, str) else role['role'] for role in roles] + assert role_names.index('check_features') < role_names.index('pre_install') diff --git a/tests/unit/filter_test.py b/tests/unit/filter_test.py index 4aeb0d6e9..d629fd907 100644 --- a/tests/unit/filter_test.py +++ b/tests/unit/filter_test.py @@ -1,9 +1,13 @@ +import pytest from foremanctl import FEATURE_MAP from foremanctl import conflicting_features from foremanctl import foreman_plugins from foremanctl import foreman_proxy_plugins from foremanctl import hammer_plugins +from foremanctl import is_feature_removable from foremanctl import list_all_features +from foremanctl import unsatisfied_dependencies +from foremanctl import validate_feature_removals def _asymmetric_conflicts(): @@ -68,6 +72,95 @@ def test_list_all_features_marks_dependency_as_enabled(monkeypatch): assert 'enabled' in child_line +def test_list_all_features_marks_flavor_features(monkeypatch): + monkeypatch.setitem(FEATURE_MAP, 'test-feature', {'removable': True}) + output = list_all_features(['test-feature'], flavor_features=['test-feature']) + feature_line = next(line for line in output.splitlines() if line.startswith('test-feature')) + assert 'flavor' in feature_line + + +def test_is_feature_removable_defaults_to_false(monkeypatch): + monkeypatch.setitem(FEATURE_MAP, 'test-feature', {}) + assert is_feature_removable('test-feature') is False + + +def test_validate_feature_removals_rejects_unknown(): + errors = validate_feature_removals(['missing'], []) + assert errors == [ + "Cannot remove unknown feature 'missing'. Run 'foremanctl features' to see available features." + ] + + +def test_validate_feature_removals_rejects_non_removable(monkeypatch): + monkeypatch.setitem(FEATURE_MAP, 'test-feature', {}) + errors = validate_feature_removals(['test-feature'], []) + assert errors == [ + "Cannot remove feature 'test-feature' — this feature does not support removal. " + "Run 'foremanctl features' to see which features can be removed." + ] + + +def test_validate_feature_removals_rejects_flavor_feature(monkeypatch): + monkeypatch.setitem(FEATURE_MAP, 'test-feature', {'removable': True}) + errors = validate_feature_removals(['test-feature'], ['test-feature']) + assert errors == [ + "Cannot remove 'test-feature' — it is a core feature of the current flavor. " + "Flavor features cannot be removed." + ] + + +def test_validate_feature_removals_allows_absent_removable_feature(monkeypatch): + monkeypatch.setitem(FEATURE_MAP, 'test-feature', {'removable': True}) + assert validate_feature_removals(['test-feature'], []) == [] + + +@pytest.mark.parametrize( + 'feature', + [name for name, metadata in FEATURE_MAP.items() if metadata.get('removable')], +) +def test_registered_removable_features_are_accepted(feature): + assert validate_feature_removals([feature], []) == [] + + +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_ignores_removal_canceled_by_readd(monkeypatch): + monkeypatch.setitem(FEATURE_MAP, 'test-parent', {'dependencies': ['test-child']}) + monkeypatch.setitem(FEATURE_MAP, 'test-child', {}) + result = unsatisfied_dependencies(['test-parent', 'test-child'], ['test-child']) + assert result == [] + + +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'],