Merge lp:~bac/lpsetup/mv-initialize-lxc into lp:lpsetup

Proposed by Brad Crittenden
Status: Merged
Approved by: Brad Crittenden
Approved revision: 57
Merged at revision: 49
Proposed branch: lp:~bac/lpsetup/mv-initialize-lxc
Merge into: lp:lpsetup
Diff against target: 0 lines
To merge this branch: bzr merge lp:~bac/lpsetup/mv-initialize-lxc
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+114239@code.launchpad.net

Commit message

Move most of initialize_lxc to init-host. Rename lxc-install to install-lxc. Add commands.rst. Updated the docstrings, and thus the help strings, for the subcommands. NOTE: finish-init-host and install-lxc are broken for now.

Description of the change

Move most of initialize_lxc to init-host.

Rename lxc-install to install-lxc.

Add commands.rst, which lays out what each subcommand does, its relationships to other commands, the environment in which it is to be run, and whether it performs actions as root or the user.

Updated the docstrings, and thus the help strings, for the subcommands. Also updated the module-level docstrings for each subcommand.

NOTE:

finish-init-host and install-lxc are broken as a result of these changes and need to be refactored. Their tests have been disabled.

Since I modified setup.cfg for nosetest options to ignore 'disabled_' tests, I decided to move with-doctest to that file from the pre-commit.sh. For some reason this is now triggering a warning about the version of distribute being used.

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

Since commands.rst is reST, we can reST-style definition lists for the
"Terminology" and "install-lxc" sections (see
http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#definition-lists).

The line length in commands.rst seems inconsistent. The longest line
looks to be 76 characters wide, which would suggest a 79 character max.
In that case there are several lines that seem to be prematurely
wrapped. I suggest 79 character lines to match our source files.

The steps list in lpsetup/subcommands/inithost.py (line 381 of the diff)
should end with a comma after the last item in the list and the closing
parenthesis on its own line in order to be consistent with the other
subcommands.

The docstring for call_initialize_lxc can't decide if "lxc" should be
capitalized (and the subject and verb agreement is off, too).

You have a XXX comment on line 592 of the diff.

In running_in_container, if neither of the running-in-container or
lxc-is-container commands exist it will just return false. It seems to
me that if that happens an exception should be raised instead.

Revision history for this message
Brad Crittenden (bac) wrote :

Thanks for the review Benji.

The tips for better formatting the reST file are appreciated and I've made the changes. The style, formatting, and grammar fixes have been made.

The XXX needs to stay but I have provided an explanatory comment.

As for the running-in-container question, I defer to Gary's blog post, where he quotes Serge:

gary_poster: how can code determine if it is being run within an LXC container?
gary_poster asked Serge Hallyn if there were a reliable way for code to determine if it is being run within an LXC container. Serge said yes, and gave these steps (note that this may be Ubuntu-specific; Ubuntu-specific is good enough for us right now).
if /bin/running-in-container is present (precise and above, always), run it and check for 0 return value
else, if lxc-is-container is not present, assume lxcguest is not installed and you're not in a container (or are in trimmed container)
else, run lxc-is-container, if 0, you're in a container, if 1 you're not

(http://codesinger.blogspot.com/2012/07/yellow-squad-weekly-retrospective.html)

So, according to Serge, the absence of either of those test executables should indicate false.

Revision history for this message
Launchpad QA Bot (lpqabot) wrote :

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Revision history for this message
Launchpad QA Bot (lpqabot) wrote :

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0. Got: 1 Pending.

Revision history for this message
Benji York (benji) wrote :

These changes look good. It might be a good idea to add a comment about the running-in-container bit.

review: Approve (code)
Revision history for this message
Launchpad QA Bot (lpqabot) wrote :

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Revision history for this message
Launchpad QA Bot (lpqabot) wrote :

The attempt to merge lp:~bac/lpsetup/mv-initialize-lxc into lp:lpsetup failed. Below is the output from the failed tests.

nose.plugins.cover: ERROR: Coverage not available: unable to import coverage module
............................................................................................The required version of distribute (>=0.6.27) is not available,
and can't be installed while this script is running. Please
install a more recent version first, using
'easy_install -U distribute'.

(Currently using distribute 0.6.24dev-r0 (/usr/lib/python2.7/dist-packages))

lp:~bac/lpsetup/mv-initialize-lxc updated
57. By Brad Crittenden

Work-around for ignore-files

Preview Diff

Empty

Subscribers

People subscribed via source and target branches