Code review comment for lp:~gmb/maas-test/add-reporting

> [0]
>
> 40 + # We specifically use 2047 for the memory size here
> because
> 41 + # using 2048 causes qemu to raise an error on i386 hosts.
> 42 + "--memory", "2047",
>
> This will conflict with what's in trunk now. Please merge trunk.

Done.

>
> [1]
>
> The module-level docstrings of the new modules need to be changed.

Done.

> [2]
>
> maastest/main.py
>
> 80 + from maastest import report
> 81 + encoded_results = (
> 82 + output_stream.buffer.getvalue().encode('utf-8'))
> 83 + blob = report.create_launchpad_blob(encoded_results)
> 84 + blob_token = report.upload_blob(blob)
> [...]
> 100 + "Visit https://bugs.launchpad.net/maas-test/+filebug/%s "
> 101 + "to file a bug and complete the maas-test reporting
> process."
> 102 + % blob_token)
>
> Wouldn't it be better if all of this was in a separate helper method so that
> it can be easily tested?

Yes, Done.

> [3]
>
> 27 +__all__ = [
> 128 + "upload_blob",
> 129 + ]
>
> 'create_launchpad_blob' is missing from the list.

Fixed.

> [4]
>
> This while thing is a pretty serious change to what maas-test does. A good
> one I mean, but a serious one and I don't see any changes to the
> documentation. Maybe you're planning to do it in a follow-up branch but it's
> pretty important that this behavior is documented.

I was planning to do it in a follow-up branch, for the sake of keeping this one as bloated as it is right now.

> [5]
>
> Questions about testing: what the code does is pretty tricky as it interacts
> with an external system (Launchpad). Can I ask what kind of testing you've
> done already? Could we have sort of a QA plan for this? In particular, I
> think it's pretty important to make sure that everything works fine when maas-
> test can't access Launchpad; I don't think there is provision for that in the
> code: what happens if upload_blob.opener.open(request) errors for instance?

I've done plenty of testing of how it interacts with Launchpad, but only when Launchpad was available (foolishly enough). It now handles correctly not being able to connect to Launchpad.

I'll add more in a subsequent branch.

> [6]
>
> This code doesn't use the optionally-provided proxy option. This does not
> seem right to me but I'd like to hear your thoughts about this.

No, you're right it, it doesn't. That's fixed now by setting the http(s)_proxy in os.environ (though I now feel the proxy setup is done in slightly the wrong place, but that's a concern for another time).

> [7]
>
> Maybe something we can do in a follow-up branch but, although I think it's
> sane to do the reporting by default, I'm also convinced it would be pretty
> useful to have a "--no-report" option.

There'll be a follow up branch for that.