Merge lp:~benji/lpsetup/add-hostinit into lp:lpsetup

[Benji York]

move much of the code out of the "initialize" subcommand into an "inithost" subcommand per the lpsetup LEP

This branch moves much of the code out of the "initialize" subcommand
into an "inithost" subcommand per the lpsetup LEP.

The subcommand name "hostinit" was chosen as it better communicates the
function of the command but can be easily changed to the name used in
the LEP ("init") if desired.

The initialize command was refactored as a subclass of inithost that
reuses the inithost steps and adds steps. It is expected that the
remaining code included in the initialize command will eventually be
moved into another command still and only a small backward-compatible
subcommand will be left. That subcommand will be removed when the need
for backward compatibility goes away.

The existing tests continue to pass, but few of them exercised the
initialize command. Manual testing suggests that the new and
pre-existing commands continue to function.

[Gary Poster (gary)]

Looks good, Benji.

As you've mentioned, the subcommands do not have a testing story, and the longer we wait the worse it will be. I'll request that we tackle that next.

Similarly, we need to consider how we upgrade existing installations when we learn that some new configuration or workaround is necessary. I think that primarily affects inithost and initlxc, so it can be a separate step. I'd feel more assured if we had a plan for that now, though, I must admit.

Thank you!

review: Approve

[Gary Poster (gary)]

I moved the MP back to "Needs Review" because we would like frankban to review.

[Francesco Banconi (frankban)]

Thank you for this branch Benji.
Comments follow.

18 + inithost,

+1 on renaming init.

82 +def initialize(
83 + user, full_name, email, lpuser, private_key, public_key, valid_ssh_keys,
84 + ssh_key_path, feed_random, dependencies_dir, directory):

I don't think we should need `dependencies_dir` and `directory` in the inithost command. The init(host|lxc) commands just prepare an environment without actually checking out launchpad code or dependencies. Indeed these names are not used in the code AFAICT.

I am also inclined to say that we don't need `lpuser` and `feed_random` either: the former is used in lp-login (that maybe can be run by `lpsetup get`), the latter is a parallel tests hack, and could be handled by the `testing` subcommand, but I am not sure of this at this point.

127 +def setup_bzr_locations(user, lpuser, directory, template=LP_BZR_LOCATIONS):
128 + """Set up bazaar locations."""

A corollary of what said above is that `setup_bzr_locations` should not be part of this command: when we get the code we can set up bzr location to continue supporting the "normal branches with trees" rocketfuel story. At that point you could also run `lpsetup get` several times in the same environment/container, having bzr location set up for each directory you are using.

166 + initialize_step = (initialize,
167 + 'user', 'full_name', 'email', 'lpuser',
168 + 'private_key', 'public_key', 'valid_ssh_keys', 'ssh_key_path',
169 + 'feed_random', 'dependencies_dir', 'directory')
170 +
171 + setup_apt_step = (setup_apt,
172 + 'no_repositories')
173 + setup_bzr_locations_step = (setup_bzr_locations,
174 + 'user', 'lpuser', 'directory')

I really like how you handled steps definition: this way they can be easily reused by other commands. I am wondering if it's better to have them as class attributes or module level names.

224 + parser.add_argument(
225 + '-d', '--dependencies-dir', default=DEPENDENCIES_DIR,
226 + help='The directory of the Launchpad dependencies to be created. '
227 + 'The directory must reside under the home directory of the '
228 + 'given user (see -u argument). '
229 + '[DEFAULT={0}]'.format(DEPENDENCIES_DIR))
... and the other options...

Same as above. I know that with these changes it will be more difficult to still maintain the `install` subcommand as an extension of this one. In that case, if you and Gary agree, I suggest to just leave the install one as it is, duplicating the relevant code in inithost. At the end of this refactoring the deprecated commands (including install) will be deleted anyway. And I am sorry for not having mentioned those suggestions earlier during our call, but I didn't realize them yet.

review: Needs Fixing

[Benji York (benji)]

> Thank you for this branch Benji.
> Comments follow.
>
> 18 + inithost,
>
> +1 on renaming init.

cool

> 82 +def initialize(
> 83 + user, full_name, email, lpuser, private_key, public_key,
> valid_ssh_keys,
> 84 + ssh_key_path, feed_random, dependencies_dir, directory):
>
> I don't think we should need `dependencies_dir` and `directory` in the
> inithost command.

Good catch. With all the code moving around I missed those two. Fixed.

> I am also inclined to say that we don't need `lpuser` and `feed_random`
> either: the former is used in lp-login (that maybe can be run by `lpsetup
> get`), the latter is a parallel tests hack, and could be handled by the
> `testing` subcommand, but I am not sure of this at this point.

The above sounds fine to me, but I don't think moving either is
actionable right now because they involve moving them to places that
don't exist yet.

> 127 +def setup_bzr_locations(user, lpuser, directory,
> template=LP_BZR_LOCATIONS):
> 128 + """Set up bazaar locations."""
>
> A corollary of what said above is that `setup_bzr_locations` should not be
> part of this command: when we get the code we can set up bzr location to
> continue supporting the "normal branches with trees" rocketfuel story. At that
> point you could also run `lpsetup get` several times in the same
> environment/container, having bzr location set up for each directory you are
> using.

Agreed. I have moved setup_bzr_locations back to install.

> 166 + initialize_step = (initialize,
> 167 + 'user', 'full_name', 'email', 'lpuser',
> 168 + 'private_key', 'public_key', 'valid_ssh_keys',
> 'ssh_key_path',
> 169 + 'feed_random', 'dependencies_dir', 'directory')
> 170 +
> 171 + setup_apt_step = (setup_apt,
> 172 + 'no_repositories')
> 173 + setup_bzr_locations_step = (setup_bzr_locations,
> 174 + 'user', 'lpuser', 'directory')
>
> I really like how you handled steps definition: this way they can be easily
> reused by other commands. I am wondering if it's better to have them as class
> attributes or module level names.

I prefer things be outside classes more than inside, but I made them
class attributes because they are used only by subclasses. I left them
as-is. We can move them if they begin being used by non-subclasses. I
don't feel strongly about it.

> 224 + parser.add_argument(
> 225 + '-d', '--dependencies-dir', default=DEPENDENCIES_DIR,
> 226 + help='The directory of the Launchpad dependencies to be
> created. '
> 227 + 'The directory must reside under the home directory
> of the '
> 228 + 'given user (see -u argument). '
> 229 + '[DEFAULT={0}]'.format(DEPENDENCIES_DIR))
> ... and the other options...

I moved the --dependencies-dir and --direcrory arguments back to
install.

> Same as above. I know that with these changes it will be more difficult to
> still maintain the `install` subcommand as an extension of this one.

It took a little additional work, but I managed to keep "install" as a
subclass of "inithost" witho...

[lp:~benji/lpsetup/add-hostinit]

34. By Benji York on 2012-06-21

make review changes

[Francesco Banconi (frankban)]

> > I am also inclined to say that we don't need `lpuser` and `feed_random`
> > either: the former is used in lp-login (that maybe can be run by `lpsetup
> > get`), the latter is a parallel tests hack, and could be handled by the
> > `testing` subcommand, but I am not sure of this at this point.
>
> The above sounds fine to me, but I don't think moving either is
> actionable right now because they involve moving them to places that
> don't exist yet.

+1, I will add a card to move them elsewhere, blocked by `lpsetup get`.

Looks good, just some lint/imports observations.

65 +from lpsetup.settings import (
66 + APT_REPOSITORIES,
67 + BASE_PACKAGES,
68 + LP_CHECKOUT,
69 + LP_PACKAGES,
70 + SSH_KEY_NAME,
71 + )
72 +from lpsetup.utils import (
73 + call,
74 + ConfigParser,
75 + )

LP_CHECKOUT and ConfigParser are no longer used here. ConfigParser must be imported in install.py.

211 __all__ = [

'fetch' missing.

243 +from lpsetup import argparser

Imported but not used.

285 with su(user) as env:

env is no longer used.

handlers.handle_directories must be added in install and lxcinstall sub commands: you can override
`def get_validators(self, namespace):` to do that (it's already done in lxcinstall).

Thanks Benji.

review: Approve

[lp:~benji/lpsetup/add-hostinit]

35. By Benji York on 2012-06-22

fix lint; add notes about how to install and use linter

36. By Benji York on 2012-06-22

add "fetch" to exported names

37. By Benji York on 2012-06-22

add directory validators to install and lxcinstall

[Benji York (benji)]

> Looks good, just some lint/imports observations.

I have fixed all the lint and added instructions on how to install and
use pocketlint.

> 211 __all__ = [
>
> 'fetch' missing.

It is not used by other modules so it doesn't make much since to add it
to __all__, but since you asked so nicely I did it anyway.

> handlers.handle_directories must be added in install and lxcinstall sub
> commands: you can override
> `def get_validators(self, namespace):` to do that (it's already done in
> lxcinstall).

Done. This is the sort of thing that it would have been nice to have
functional tests for. That way when I omitted the validators when
rearranging the code I could have been informed of the problem.

This suggests a potential design improvement: validators should fail
closed instead of failing open. In other words, if a validator for a
particular parameter is not present, that parameter should not be
visible to the program. One way to achieve that would be to rephrase
the validators as parameter parsers which populate an output options
structure with validated values from an input options structure.