Code review comment for lp:~jtv/pyjuju/maas-anon-urls

Looks good! [1] and [2] were head-scratchers but I don't want to block
you on resolving these, hence +1.

[1]

+    def _download(self, url, intention=None):
...
+        try:
+            body = yield self.dispatch_query(url)
+        except Error:
+            if hasattr(self.client, 'status'):
+                # This is an error response from the server.  We'll
+                # handle it by looking at the http status code.
+                pass
...
+        status = int(self.client.status)
+        if status == httplib.NOT_FOUND:
+            raise FileNotFound(url)
+        if (status < 200) or (status > 299):
+            # Something else went wrong.  Pass it on.
+            raise ProviderError("%s: unexpected HTTP %s" % (intention, status))

Would this last conditional be needed if this block were moved up to
where the `pass` is made?

        try:
            body = yield self.dispatch_query(url)
        except Error:
            if hasattr(self.client, 'status'):
                # This is an error response from the server.  We'll
                # handle it by looking at the http status code.
                status = int(self.client.status)
                if status == httplib.NOT_FOUND:
                    raise FileNotFound(url)
                else:
                    # Something else went wrong.  Pass it on.
                    raise ProviderError(...
            else:
                raise

[2]

+        except Error:
+            if hasattr(self.client, 'status'):
+                # This is an error response from the server.  We'll
+                # handle it by looking at the http status code.
+                pass
+            else:
+                # This is a client-side failure.  Pass it on.
+                raise

This got me wondering...

Error always has a status attribute. In twisted.web.client it only
ever appears to be raised when there's a status to report, which
implies that some response has been received.

Also, client.status (where client is an instance of HTTPClientFactory)
is set when twisted.web.http.HTTPClient.lineReceived calls
gotStatus(), when the first line back from the server is received, but
is not reset when there's a 30[123] redirect, so it's not a wholly
reliable test for a client-side failure.

Might the following do what you need?

        try:
            body = yield self.dispatch_query(url)
        except Error as error:
            if int(error.status) == httplib.NOT_FOUND:
                raise FileNotFound(url)
            else:
                # Something else went wrong.  Pass it on.
                raise ProviderError(
                    "%s: unexpected HTTP %s" % (intention, status))
        except Exception as e:
            raise convert_unknown_error(e)

        returnValue(body)

Btw, I like the intention stuff. I think it'll make user's lives a
little nicer.

[3]

+        anon_url = attributes.get('anon_url')

The handling of this will need to change; rvba's changing this to
anon_resource_uri (iirc, better check), which also means you'll need
to join it to self._base_url.

[4]

+        try:
+            yield storage.get_url("kaboom")
+        except ProviderError:
+            # This is what we expect: an error response from the server means
+            # that the Deferred returned by get_url fails.
+            pass
+        except:
+            # Any other error?  That's unexpected.  Pass it on.
+            raise
+        else:
+            self.fail("Error was not propagated.")

Fwiw, testtools.ExpectedException can help here, assuming we're
allowed testtools (ah, maybe not):

        with ExpectedException(ProviderError):
            yield storage.get_url("kaboom")