> Looks good. This is excellent stuff to get reporting working with
> Launchpad, and it's apparent that a lot of work has gone into this.
> Things like the Launchpad blob prep and upload code could have been
> reviewed and landed independently, but things don't always work out as
> clear cut as we'd like. I have more comments to pile onto the ones that
> Raffers has already made, but I'm not going to binary fissionate
> post-midnight-snacking mogwai if you disagree with them.

Best review comment ever.
 
> [1]
> 
> @@ -70,6 +71,8 @@
>              # just use that proxy for everything.
>              proxy_url = cls.args.http_proxy
>              logging.info("Using external proxy %s." % proxy_url)
> +            os.environ['http_proxy'] = cls.args.http_proxy
> +            os.environ['https_proxy'] = cls.args.https_proxy
>          elif cls.args.disable_cache:
>              # The user has passed --disable-cache, so don't start polipo
>              # and don't try to use an external proxy.
> @@ -81,6 +84,8 @@
>              proxy = LocalProxyFixture()
>              cls.fixtures.useFixture(proxy)
>              proxy_url = proxy.get_url()
> +            os.environ['http_proxy'] = proxy_url
> +            os.environ['https_proxy'] = proxy_url
> 
> Can you use fixtures.EnvironmentVariable here?

I don't think so, because (rather filthily; I don't like it and it needs
refactoring in some way) doing the os.environ dance means that when we
do the reporting, which happens outside of the TestCase, the proxy still
gets used automatically by urllib2. Of course, we could handle it
manually, but that doesn't feel any nicer.

> [2]
> 
>         cls.machine = KVMFixture(
>             series=cls.args.maas_series, architecture=architecture,
>             proxy_url=proxy_url, direct_interface=cls.args.interface,
>             archives=cls.args.archive)
> 
> You've not changed this, but it seem odd now to set the proxy in the
> environment *and* pass it into KVMFixture.

That's true, and I'm going to address that in a follow-up branch.

> Also, KVMFixture has a few commands where it sets http_proxy in the
> environment, for example:
> 
>         return [
>             'sudo', 'http_proxy=%s' % self.proxy_url,
>             'DEBIAN_FRONTEND=noninteractive',
>             'apt-get', 'install', '-y'
>             ] + list(packages)
> 
> Now that we're setting these in the environment, it ought to work to do:
> 
>         return [
>             'sudo', '-E',
>             ...
> 
> to pass through the caller's environment.
> 
> Don't do this now though, just wondering what your thoughts are.

That would work for some of the commands but not all of them. For 
example, as Raffers and I discovered earlier this week,
uvtool-simplestreams, to which we shell out, uses the requests module,
which doesn't handle having an https_proxy at all well (in point of
fact, it pukes on your shoes should you specify one). So we could use -E
in some places but not others. I'm fine with that if it's just for the 
sync command, where we're downloading the remote images.VGV

> [3]
> 
> +    results_payload = make_file_payload(
> +        'maas-test.log', test_results.encode('ascii'))
> 
> This encoding might go horribly wrong. How about encoding as UTF-8, with
> a "replace" errors strategy? It might be lossy, but something's better
> than nothing.

Much better idea, thanks.
 
> [4]
> 
> +def build_upload_request(url, launchpad_form_data):
> +    data_flat = BytesIO()
> +    generator = email.generator.Generator(data_flat, mangle_from_=False)
> +    generator.flatten(launchpad_form_data)
> 
> In apiclient.multipart.encode_multipart_message(), there's an extra step
> here:
> 
>     buf = BytesIO()
>     generator = Generator(buf, False)  # Don't mangle "^From".
> --> generator._write_headers = lambda self: None  # Ignore.
>     generator.flatten(message)
> 
> This suppresses the leading headers, so that it's creating only a
> message body. Otherwise the generator and the HTTP library both write
> out leading headers.
> 
> The former's won't interfere with the latter's because they're being
> written out in the body. However, it might mean that the body is
> somewhat garbled, though not enough to break things it seems. However,
> maybe something is going subtly wrong?
> 
> If this actually cargo-culted, then leave it as is I guess, but please
> add a comment noting the source for each and every function that has
> been copied in.

Yeah, this bit is cargo-culted from apport — having the ._write_headers
bit in causes Launchpad to just ignore the blob; I couldn't figure out
why for sure, but I'm 99% certain that it just doesn't recognise the
blob without the leading headers. I've added a comment.

> [5]
> 
> +    except Exception as error:
> +        logging.error("Unable to connect to Launchpad: %s" % error.message)
> 
> Consider using:
> 
>     except Exception as error:
>         logging.exception("Unable to connect to Launchpad")
> 
> It will capture the stack trace for you too, though you might not want
> that.

Sweet, thanks.

> [6]
> 
> +        self.mock_opener = mock.MagicMock()
> +        self.mock_result = mock.MagicMock()
> +        self.mock_opener.open = mock.MagicMock(return_value=self.mock_result)
> 
> Fwiw, this can be written as:
> 
>         self.mock_opener = mock.MagicMock()
>         self.mock_result = mock.MagicMock()
>         self.mock_opener.open.return_value = self.mock_result
> 
> In fact, you can also do:
> 
>         self.mock_opener = mock.MagicMock()
>         self.mock_result = self.mock_opener.open.return_value
> 
> or refer to it later in the test; it remembers:
> 
>     def setUp(self):
>         super(TestUploadBlob, self).setUp()
>         self.patch(report, 'build_opener', mock.MagicMock())
> 
>     def test_builds_opener(self):
>         results = self.getUniqueString()
>         blob = report.create_launchpad_blob(results)
>         report.upload_blob(blob)
>         report.build_opener.assert_has_calls(
>             [mock.call(urllib2.HTTPSHandler)])

That's awesome.

> [7]
> 
> +    def test_builds_opener(self):
> +        results = self.getUniqueString()
> +        blob = report.create_launchpad_blob(results)
> +        mock_build_opener = mock.MagicMock(return_value=self.mock_opener)
> +        self.patch(report, 'build_opener', mock_build_opener)
> 
> The last two lines repeat what's in setUp().

Yep, fixed.

> [8]
> 
> +        self.patch(logging, 'info', mock_logger)
> 
> Another way to do this is using fixtures.FakeLogger:
> 
>         with fixtures.FakeLogger() as logger:
>             ... do stuff ...
>         self.assertEqual("expected log messages", logger.output)

Yes, but then one finds oneself cocking around with stack traces (now
that we're using logging.exception()) so  for now I've avoided it.

> [9]
> 
> +        with open(os.path.join(temp_dir, log_file_name), 'r') as log_file:
> +            self.assertEqual(results, log_file.read().strip())
> 
> Please remember to use the 'b' flag to open(), and bear in mind that
> getUniqueString() probably returns a byte string on Python 2 and a
> unicode string on Python 3.

Oy vey, and other phrases to which I have no ethnic right. Fixed.

> [10]
> 
> +class BufferingOutputStream:
> +
> +    def __init__(self, stream):
> +        from StringIO import StringIO
> +        self.buffer = StringIO()
> 
> Consider using io.BytesIO here instead of StringIO. The latter allows
> byte and unicode strings to be intermixed with implicit encoding.
> 
> If you need to allow writing of both byte and unicode strings, you can
> do so explicitly with something like so:
> 
>

BytesIO suits fine. Thanks for the tip.