Validate features - only valid features can be enabled#405
Validate features - only valid features can be enabled#405arvind4501 wants to merge 7 commits intotheforeman:masterfrom
Conversation
| @@ -0,0 +1,40 @@ | |||
| --- | |||
| - name: Remove invalid features from parameters.yaml | |||
There was a problem hiding this comment.
We shouldn't touch parameters.yaml from Ansible, that's obsah's space.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- a
validateplaybook, that runs checks and only if that passes, params are persisted - a
runplaybook, that contains the actual "do things" part.
There was a problem hiding this comment.
I think if we fix it in obsah, it will be a centralized solution and would make these user mistakes be handled more explictely
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think we don't need to handle it here and can do it standalone
evgeni
left a comment
evgeni
left a comment
There was a problem hiding this comment.
Should be squashed on merge. Works nicely.
| fail_msg: | | ||
| ERROR: Unknown feature(s) requested: {{ found_invalid_features | join(', ') }} | ||
|
|
||
| Run 'foremanctl features' to list all available features. |
There was a problem hiding this comment.
I would assume with this here, this shouldn't be merged until #404 is merged.
4 participants
Add feature validation to prevent invalid features from being enabled during deployment: