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.