Code review comment for lp:~frankban/launchpad/lpsetup-initial

Thank you Gary for reviewing this, it's a huge file.

> Unless you give me a good reason why not, I'd like all of the contextmanagers
> to have a try:finally block placed around the yield. environ and cd are the
> only ones without it, I think.

No good reason, I will add the try/finally block to them.

> Why do you need your own check_output, rather than using subprocess'
> check_output? Ah! Because it is not in Python 2.6? If that's the reason,
> what would you think of trying to get it from the module, and if there's an
> AttributeError (or ImportError, however you want to work it) then you define
> your function? That way we get the newer version, with possible bugfixes, if
> it exists; and we have a chance to clarify why we are not using the stdlib's
> function.

That's one of the reasons, the others, like the first, aren't essential, just nice to have:
- the exception raised on errors is the one subcommand handlers trap (but of course I could also trap subprocess.CalledProcessError)
- the positional arguments thing (really not even a "nice to have", more a "look, we're lazy and cool")
- None is ignored if given as argument, so that you can write `check_output('ls', '-l' if something else None)
- the error contains a better representation of the executed command

I can get rid of check_output; my point is: like `subprocess.check_output`, it's just a little wrapper around `subprocess.Popen`, that does the real stuff (and we will not miss future bugfixes for that object).

> So many of these helpers are generically useful. It's a shame they are not
> somewhere more central. Many of them look helpful for charms too. I don't
> have a suggested action for that; it's just an observation.

I agree.

> As we discussed already, in your shoes people have advocated to me that I
> should use bzr's command framework (and I mentioned Jamu Kakar's commandant;
> that project hasn't seen any updates, but it looks like Landscape is still
> using it because they have done some recent packaging work). I like the fact
> that what you have is self-contained and small. Again, I merely offer that as
> an avenue that I suggest you investigate in the future.

Thanks Gary, sure, I will take a look at commandant, although I like my tiny subcommand objects.

> I look forward to this being a separate project with an ability to divide
> things up into support files and so on. In addition to making it possible to
> organize the code more obviously, it also might then be reasonable to
> reconsider the use of doctests.

Yes, you are right, I will add a slack time card for unit tests.

>
> Random thought: I was thinking about your other branch that had LANG=C.
> Probably obvious, but you could change apt_get_install to accept keyword
> arguments and include those as environment variables so you could easily do
> the LANG=C thing, maybe? That maybe looks too much like command options.
> Just a thought.

Nice idea IMHO.

> As I'm sure you've realized, we should use generate_ssh_keys with descriptive
> names per the webops request for setuplxc. If these become merely
> "root_ssh_key" and "buildbot_ssh_key" then that's not what they want. They
> want to know "why" the key is used (e.g., "lxc_communication_key" or similar).
> If there's not a good name for this key that would distinguish it from other
> keys a user might have, we should push back; but AFAICT there should be a good
> name here.

I propose 'id_for_lxc'... I know: we can do better.