Code review comment for lp:~benji/lpsetup/add-hostinit

> 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" without copy/pasting code or doing other horrible
things.