Launchpad itself

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

lpsetup-initial
Merge into devel

Revision history for this message

Gary Poster (gary) wrote on 2012-02-27:

Hey Francesco. Thanks for doing this! This is a nice slack time project.

This review is through line 895. I will continue on this after lunch.

In join_command's docstring, there's a small typo, twice: containig -> containing.

Spelling niggle: in environ's docstring ("A context manager to temporary change environment variables."), change "temporary" to "temporarily". Same for su docstring ("A context manager to temporary run the script as a different user.").

Minor grammar niggle: in docstring of check_output, you need to convert the comma to one of three choices: semicolon, ", and", or period (new sentence): "The first argument is the path to the command to run, subsequent arguments are command-line arguments to be passed."

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.

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.

I had never used the pipes module. The quotes function is small and convenient. I'm mildly concerned that it is not part of the module's documented API (http://docs.python.org/library/pipes.html). That stuff stays pretty stable across Python IME though; I'm fine with it.

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.

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.

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. I am among the apparent minority that like them in the Python community, and I think what you have here is well done, but our team's standards are to use unit tests. As long as this is a single downloadable file, doctests make enough sense to me to be a defensible choice. Later, maybe less so.

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.

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.

Hey Francesco.  Thanks for doing this!  This is a nice slack time project.

This review is through line 895.  I will continue on this after lunch.

In join_command's docstring, there's a small typo, twice: containig -> containing.

Spelling niggle: in environ's docstring ("A context manager to temporary change environment variables."), change "temporary" to "temporarily".  Same for su docstring ("A context manager to temporary run the script as a different user.").

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.

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.

I had never used the pipes module.  The quotes function is small and convenient.  I'm mildly concerned that it is not part of the module's documented API (http://docs.python.org/library/pipes.html).  That stuff stays pretty stable across Python IME though; I'm fine with it.

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.

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.

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.  I am among the apparent minority that like them in the Python community, and I think what you have here is well done, but our team's standards are to use unit tests. As long as this is a single downloadable file, doctests make enough sense to me to be a defensible choice.  Later, maybe less so.

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.

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.

« Back to merge proposal