Merge lp:~benji/lpsetup/bug-1016645-add-command-line-options into lp:lpsetup

[Benji York]

add some command-line options to the get command from the LEP

This branch (mostly, see below) finishes the work on bug 1016645 by
adding command-line options to the "get" subcommand.

Two things from that bug were not done: the first was adding a
"repository" optional argument because --directory already does that.

The second undone thing is the adding of a "branch" argument to specify
one other than "devel". I couldn't find a non-super-invasive way of
implementing that functionality and since it is listed as optional, I
decided not to further overrun my personal timebox. If it is in fact
non-optional, then I can do that next in a different branch.

Tests: no new tests were needed but the existing tests spotted at least
one bug, so that was nice.

Lint: the pocketlint report is clean

[Gary Poster (gary)]

Thank you for this branch. Cleaning out the no-longer-working doctests is nice too, though hooking them up again, insofar as they actually were documentation, might not have been horrible. Thank you also for changing "directory" to "repository".

I don't want to forget the branch argument: if we have time, we should add it. Please add a low priority card to the kanban board for this task.

Using get to update an existing checkout is interesting, and I'm not sure it is the right thing. That's the intended domain of update, which also is supposed to shelve changes before the pull and unshelve them after the pull. I suggest that we rip that behavior out from get. (Note that IMO we can also rip the branch command too, if that makes anything easier; but that can wait till another branch too. Note further that we can rip out the install command, but that's trickier because lxcinstall defers to it ATM, so I definitely think that ripping install out needs to be its own branch.)

I was curious about the "Repository {0} is corrupted" stuff when I saw it too...did you check with frankban before you removed it? If not, please do. I think either removing it or commenting on why we think it is necessary are both good possible directions.

If there is no sandbox_dir, and only a checkout_dir, can we infer that the checkout is actually a branch (i.e., lightweight=False)?

ISTR someone was going to check on why we have --2a as an argument. That should be the default for all versions of bzr that we encounter. If it is not, I think a comment is appropriate. Otherwise, let's remove it.

I skimmed the rest, and it looked good. I'm not going to approve pending changes because I think there may need to be a discussion about one or the both of inferring the "lightweight" value and ripping out get's "update" behavior. If someone else wants to take over the rest of the review, that would be cool.

Thanks again,

Gary

[Francesco Banconi (frankban)]

Gary wrote:

> Using get to update an existing checkout is interesting, and I'm not sure it
> is the right thing. That's the intended domain of update, which also is
> supposed to shelve changes before the pull and unshelve them after the pull.
> I suggest that we rip that behavior out from get. (Note that IMO we can also
> rip the branch command too, if that makes anything easier; but that can wait
> till another branch too. Note further that we can rip out the install
> command, but that's trickier because lxcinstall defers to it ATM, so I
> definitely think that ripping install out needs to be its own branch.)

I agree with Gary: get should not update existing branches; it should call
the update command at the end of the process. Note that the update command is
not yet implemented: the current update command (intended only as a replacement
for rocketfuel-update) must be deleted or heavily modified.

> I was curious about the "Repository {0} is corrupted" stuff when I saw it
> too...did you check with frankban before you removed it? If not, please do.
> I think either removing it or commenting on why we think it is necessary are
> both good possible directions.

I've taken that check from rocketfuel-setup. We have no control on how bzr handles
things, and a sanity check after a 40 min process seemed not too bad IMHO.

> ISTR someone was going to check on why we have --2a as an argument. That
> should be the default for all versions of bzr that we encounter. If it is
> not, I think a comment is appropriate. Otherwise, let's remove it.

Same as above: rocketfuel-setup uses that option, and maybe at the time it was written,
for some reason, it made sense. It seems it doesn't now so +1 on removing --2a.

> I skimmed the rest, and it looked good. I'm not going to approve pending
> changes because I think there may need to be a discussion about one or the
> both of inferring the "lightweight" value and ripping out get's "update"
> behavior. If someone else wants to take over the rest of the review, that
> would be cool.

Anyway, the branch is good, I am waiting for this to be merged and I am inclined to
make it happen as soon as possible. We could create another card for fixing remaining things:

- checkout option (default='sandbox')
- update stuff
- remove unneeded args (ssh args, parallel tetsting args, email, full_name etc...)

[Francesco Banconi (frankban)]

@Gary: didn't we decided to also get rid of --dependencies-dir? And to just checkout them inside the branch/sandbox?

As a side note, not really related to this MP, I've started thinking that the conservative approach we are using to work on these cards creates more problems than it solves: get and update share with the old commands only the name. Of course there is some code to reuse, but I suspect there is the risk that old code is influencing too much the way we factor the new commands. That's only a feeling I have, and I can only use my experience on initlxc as evidence: I started working on it trying to change lxcinstall before realizing that initlxc is a much simpler command, without all the lxcinstall options and complications.

[Benji York (benji)]

We (Gary and I) have extensively reworked the branch to fulfil the requirements of the LEP.

[Gary Poster (gary)]

Thank you!

review: Approve

[Launchpad QA Bot (lpqabot)]

Attempt to merge into lp:lpsetup failed due to conflicts:

text conflict in lpsetup/subcommands/get.py
text conflict in lpsetup/subcommands/lxcinstall.py

[Benji York (benji)]

I fixed the conflicts.

review: Approve