Code review comment for lp:~vila/u1-test-utils/lxc

> I think LXC would be a good default for vm.class.
>
> 29 + help='''Where lxc definitions are stored.'''
>
> Why does it have three quotes?

So that all helps have 3 quotes.

>
> 94 + :return: False if the file is in the download cache, True if a
> download
> 95 + occurred.
>
> This is more like: True if a download ocurred. False if the file is in the
> download cache and we are not forcing the download.

Well, if we are forcing the download the file can't be in the cache, that's what 'force' do, it clears the cache.

>
> 137 + def download(self):
> 138 + raise NotImplementedError(self.download)
>
> Shouldn't this be the same call to wget you do on _download_in_cache?
> If this should be implemented on the children classes, I think that would be a
> better errror message for the exception raised.

Well, implementing lxc I discovered that lxc-create provides its own caching mechanism that cannot be bypassed.

So the API provided by setup_vm doesn't fit: the download will occur during lxc-create (called by --install) and not during --download.

We probably want to tweak setup_vm instead as I think lxc-create provides a better user experience:
- download if needed when creating a vm and requires it,
- find a way to force a new download if the user requires it (I've yet to see how to get that for lxc)

I filed hhtp://pad.lv/1210428 to track that.

>
> 526 + # Create an lxc, relying on cloud-init to customize the base image.
>
> This would be better as a docstring.

Done.
>
> 951 + tests.requires_feature(self, tests.sudo_feature)
>
> This is already on the setUp.

Fixed. Thanks for catching that one, these tests were changed multiple times
in this proposal and I lost track ;)

>
> 1395 +[lxc1]
>
> What's this lxc machine for? If you are using it for something, it should have
> a better name.

Sorry, that was for manual tests and I should have deleted it before
submitting (or even better, defined it in my ~/vms.conf...). Thanks for the
heads-up.

I'll leave lxc-precise-server-pristine in place for now as an example or
base for future uses but I don't need it myself (comments added to clarify).

>
> The branch looks really good. There are things I don't yet understand, from
> the libraries you are using or from cloud-init. But you have my +1. Next week
> I hope I will be able to give it a try.

Feel free to continue the discussion about the things you don't understand,
that may reveal bugs or at least lack of documentation.

Thanks for the review !