yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

The proposal to merge lp:~frankban/lpsetup/out-of-control into lp:lpsetup has been updated.

Description changed to:

= Summary =

This branch is the result of a previous experimental branch. The goal is to reduce the inversion of control taking place in the argparser/subcommands machinery.

Before:

    We had a customized *ArgumentParser* being able to register subcommands.
    Subcommands were subclasses of *BaseSubCommand*, registered using
    *ArgumentParser.register_subcommand*. From that point on, the subcommands
    framework took care of the process of validating, restarting as root and
    actual running each sub command.

In this branch:

    The *ArgumentParser* is more or less a default *argparse.ArgumentParser*.
    The only change there is a method override done to store actions in
    a "public" attribute. Subcommands are just objects implementing a pre-defined
    protocol, that is described in the branch. So subcommands can be instances,
    or just modules.
    The process of registering and running a subcommand is now handled by functions
    in `cli.py`, and the code flow should be easier to follow because several    
    actions are now executed in a procedural way: prepare the parser, add common 
    arguments, add user-interaction related arguments, restart as root, namespace 
    initialization and validation, ... 
    However, the inversion of control is still present, but it's moved forward to 
    the point of executing actual subcommand stuff: i.e. when *subcommand.run* is  
    called. As a consequence, while the subcommand base classes are still there, 
    they are now more lightweight, and they represent only a flexible and easy way 
    to implement the subcommand protocol. They still introduce and handle the 
    concepts of handlers and steps, which are more well-documented in the code.

If we will decide in the future to follow the path of further reducing the inversion
of control (e.g. getting rid of subcommands base classes), this branch could be
considered as an incremental step.
For now, my opinion is that this could be a good trade off between the need for simplicity
and the generality/"DRYness" of OOP.

This branch adds documentation and massively reorganizes code: methods are now
functions, tests have been changed accordingly, some unused modules have been removed,
etc... so the diff is HUGE, and I am very sorry.

The integration test `test_install_lxc.py` passes.

== Other changes ==

Introduced a basic documentation for subcommands.

Modified *ArgumentParser* and base subcommand classes as described above. Also
removed no longer valid doctests. The code exercised by removed doctests is fully
covered by unittests.

*subcommand.get_description(namespace) is now called to display subcommands'
descriptions in a generic way: steps based subcommands override that method so that
the resulting description is created collecting steps' descriptions.

The `argparser` module now defines several functions explicitly called by cli:
*add_common_arguments*, *get_args_from_namespace*, *init_namespace*, *restart_as_root*.
The `cli `module itself now contains some helper functions:

    - *get_parse*r returns an argument parser with all subcommands registered
    - *prepare_parser* takes a parser (or a subparser) and a subcommand, adds common
      arguments to the parsers and registers a callback pointing to the subcommand.
      Note that the callback is a closure: that way we can immediately regain the
      control of the code flow. Also note that this function can be used passing an
      arbitrary parser. Usually a subparser is passed, but providing a normal
      *ArgumentParser* this function could be reused to prepare a parser executing
      separate subcommands (e.g. `lp-init-host` or `lp-init-repo`).
    - *run*: taking care of:
      - namespace initialization and validation
      - interactive arguments handling
      - executing a dry run, if requested
      - restarting the same sub command as root, if required
      - actual sub command execution

Removed the subcommand dynamic dispatcher: RIP.
This is how we usually define steps::

    steps = [(step1, 'arg1', 'arg2'), (step2...)]

In this branch:

    steps = [(step1, ['arg1', 'arg2']), (step2, []...)]

So the second item of each sequence is now a sequence of arg names, and a third
optional element is also allowed: a filter function that takes a namespace and must
return True if the step can be executed, False otherwise. As a side note, thanks
to this change we can now be more precise when collecting steps' descriptions.
Updated all the subcommands accordingly.

s/needs_root/root_required: needs_root is now a callable, part of the subcommands protocol.

Removed the `lpsetup.tests.examples` module: several steps and subcommands were defined there.
Now we have two factory test mixins in `lpsetup.tests.utils` that are used to make
steps or subcommands when needed in tests.

Added a new context manager (*call_replaced_by*) that temporarily mocks *subprocess.call*.
This is used to test the "restart as root" functions.

Removed the *root_not_needed* context manager: it was only used in dry run smoke tests:
now the dry execution does not want to restart as root, because it is handled before
`sudo` is called.

Removed no longer used *SubCommandTestMixin*, and fixed *StepsBasedSubCommandTestMixin*:
the latter can be safely removed once we get rid of the copy/paste smoke tests.

Moved *get_step_description* from `lpsetup.utils` to `lpsetup.argparser`.

Bumped version up a little.

For more details, see:
https://code.launchpad.net/~frankban/lpsetup/out-of-control/+merge/118982
-- 
https://code.launchpad.net/~frankban/lpsetup/out-of-control/+merge/118982
Your team Yellow Squad is requested to review the proposed merge of lp:~frankban/lpsetup/out-of-control into lp:lpsetup.