add check to avoid deployment with invalid features

[arvind4501]

Add feature validation to prevent invalid features from being enabled during deployment:

 ./foremanctl deploy --add-feature invalid-feature

[ERROR]: Task failed: Action failed: ERROR: Unknown feature(s) requested: invalid-feature

Run 'foremanctl features' to list all available features.

[evgeni]

We shouldn't touch parameters.yaml from Ansible, that's obsah's space.

[arvind4501]

My thinking behind this is that when we do --add-feature invalid-feature it adds it to parameters.yaml and then also gives error to user that its not valid but does not remove from that file. which forces the same invalid-feature to be passed on next deploy

[evgeni]

We have this problem in other places too. Like when the user provides a tuning that has requirements higher than the system they are deploying on.

I wonder if it would make sense to enhance obsah so that it can have two playbooks for a command:

1. a validate playbook, that runs checks and only if that passes, params are persisted
2. a run playbook, that contains the actual "do things" part.

[evgeni]

(this starts to feel like hooks in kafo, which I am not sure I like… 👀 @ehelms @ekohl )

[arvind4501]

I think if we fix it in obsah, it will be a centralized solution and would make these user mistakes be handled more explictely

[arvind4501]

a validate playbook, that runs checks and only if that passes, params are persisted

i have one doubt here like what are we validating against? do we have to defined whats valid args looks like before actually validating

[ehelms]

Can argparse / Obsah metadata handle this for us? I tend to agree this feels more like a CLI level check and error situation rather than a runtime error.

This might present like a kafo hook, and I would like to avoid "hooks" but this does feel like a CLI-level validation that should occur to the user early on and provide quick feedback.

[evgeni]

argparse has choices, but that'd require either having a static list (duplicating data in features.yaml) or a way to dynamically read features.yaml at runtime, which we don't have today.

[arvind4501]

I have removed the manual removal of parameters.yaml by ansible and created a issue theforeman/obsah#109 that can be handled. Do we need to handle this issue with this PR, or is it fine it can be fixed standalone?

[evgeni]

I think we don't need to handle it here and can do it standalone

[evgeni]

Should be squashed on merge. Works nicely.

[ekohl]

Maybe nitpicking, but the commit says "only valid features can be enabled" but the actual implementation is more "add check to refuse to run with invalid features". At least, my understanding is that it still adds the feature to the parameters.

To be clear, I'm OK with that for now.

Long term, if we ever remove a feature this will also catch that.

[evgeni]

Oh you mean the commit message should be updated? Yeah!

[ekohl]

That and the PR title itself. Other that that, no objections from my side.

[evgeni]

this needs a rebase after the merge of #309

[evgeni]

@arvind4501 needs another rebase 🙈

why have we been doing so much work on the filter plugin? 😄

[arvind4501]

yes, filter plugins are overwhelmed, and with current design its appeared to be best place to do all calculations 😄

[Gauravtalreja1]

ACK, Tested with foremanctl-1.2.0-1.20260324082716183495.pr405.40.g154bcff.el9.noarch and it works as expected 🍏

# foremanctl deploy --add-feature invalid-feature1 --add-feature invalid-feature2
..
...
TASK [checks : Report status of checks] ************************************************************************************************
fatal: [localhost]: FAILED! =>
    changed: false
    msg:
    -   assertion: found_invalid_features | length == 0
        changed: false
        evaluated_to: false
        failed: true
        msg: |-
            ERROR: Unknown feature(s) requested: invalid-feature1, invalid-feature2

            To remove them, run:
              foremanctl deploy --remove-feature=invalid-feature1 --remove-feature=invalid-feature2
            Run 'foremanctl features' to list all available features.

PLAY RECAP *****************************************************************************************************************************
localhost                  : ok=16   changed=0    unreachable=0    failed=1    skipped=1    rescued=1    ignored=0

Note: currently, invalid features names gets added to parameters.yaml on failure, which would be improved in theforeman/obsah#109