Use python fire for all cli commands

[uberspot]

Some minor refactoring in cli.py to use https://github.com/google/python-fire will allow for much easier expansion in functionality later on with less code.

[pranavgupta1234]

Hi @uberspot, I am planning to work on this issue. I was checking into this python-fire guide https://github.com/google/python-fire/blob/master/docs/guide.md and although there are different possible implementations, one of them can be, as we have 7 different subparsers, so we can make a function for each one of them with corresponding arguments included in it which can then be handled by fire. One other way can be to make class for each subparser. What are your thoughts on this ?

[uberspot]

Sounds like a class for each subparser is an overkill.
It might be simpler to follow their example like here:

if __name__ == '__main__':
  fire.Fire({
      'add': add,
      'multiply': multiply,
  }) 

And import functions in cli.py that implement each part.
Some more refactoring might be necessary in this process.

[pranavgupta1234]

We can shift each subparser code block inside our current if-else tree into separate functions along with necessary arguments and then just fire using a dict. Should I proceed with this implementation ?

[uberspot]

If you wish yes. We can continue comments on the PR. 👍

[pranavgupta1234]

Hey @uberspot, I wanted to discuss on some issues before creating a PR. Like:

1. Fire uses arguments from function to create flags so we can use only one flag (either longhand or shorthand notation)
2. Features like, choices=('yaml', 'json') provided by argparse, such checks now have to be done manually inside function and appropriate error has to be raised.
3. Check for nargs also inside function
   I am using python fire for the first time and if I am missing some features please let me know. One example for lint subparser now looks like:

def lint(fail_on_warning=from_dot_kapitan('lint', 'fail-on-warning', False), 
         skip_class_checks=from_dot_kapitan('lint', 'skip-class-checks', False),
         search_secrets=from_dot_kapitan('lint', 'search-secrets', False),
         secrets_path=from_dot_kapitan('lint', 'secrets-path', './secrets'),
         compiled_path=from_dot_kapitan('lint', 'compiled-path', './compiled'),
         inventory_path=from_dot_kapitan('lint', 'inventory-path', './inventory')):
    '''
    linter for inventory and secrets

    Args:
        fail_on_warning: bool
            exit with failure code if warnings exist, default is False
        skip_class_checks: bool
            skip checking for unused classes, default is False
        search_secrets: bool
            searches for plaintext secrets in inventory, default is False
        secrets_path: str
            set secrets path, default is "./secrets"
        compiled_path: str
            set compiled path, default is "./compiled"
        inventory_path: str
            set inventory path, default is "./inventory"
    '''
    start_lint(fail_on_warning, skip_class_checks, inventory_path, search_secrets, secrets_path, compiled_path)

Edit : For the first issue after searching a bit more I found this google/python-fire#108 . Some shorthand ability is indeed present but its not fully flexible.

[uberspot]

That seems like it would work. I can't really comment without seeing code but for 2) it's ok if we move that basic check in the function. We only use it in one place. For nargs a similar check will be needed but that at least can be re-used in a function, so code changes should be minimal.

[pranavgupta1234]

Hey @uberspot , can you go over above discussion in python-fire about some current difficulties. They have some few suggestions and we will have to decide on a specific way of implementation.

[uberspot]

dbieber's suggestion in google/python-fire#169 (comment)_ sounds pretty good actually. Some minor parsing for specific types is also acceptable given that this will anyway decrease the code size significantly.
One last thing I'd recommend verifying before you get into this more is to make sure the kapitan cli interface doesn't break with these changes. A user should be able to use exactly the same commands with kapitan as before (the ones mentioned in kapitan --help ) and run it with kapitan as the name of the binary.

[uberspot]

Not relevant anymore