Merge lp:~clint-fewbar/txaws/fix-s3-port into lp:txaws

[Clint Byrum]

Always use port for S3 so OpenStack works

[Robert Collins (lifeless)]

Two things:

 - only include :port if the port is not the default port for the
protocol. This (may) matter for signing with ec2 etc.
 - a test or two would be massively preferred.

-Rob

[Gustavo Niemeyer (niemeyer)]

Any news on this?

[Gustavo Niemeyer (niemeyer)]

This is sitting idle for more than two weeks. Clint, what is the plan on it? Are we getting this merged, or should we push it back?

[Gustavo Niemeyer (niemeyer)]

Actually, I'll move it to Work In Progress for the moment.

[Gustavo Niemeyer (niemeyer)]

If I could..

[Clint Byrum (clint-fewbar)]

Pushed up a commit adding two tests to verify that the port is added into the url, should cover all changed lines and a few others. :)

[Gustavo Niemeyer (niemeyer)]

Rob, any comments on the new changeset?

[Free Ekanayaka (free.ekanayaka)]

Looks good, I don't have much to add to what Robert pointed, except for the two minor comments below ([1] is automatically resolved if [2] is adopted).

+1!

[1]

+ self.endpoint.scheme, self.get_host(), self.endpoint.port, self.get_path())

This line is too long, it makes pyflakes/pep8 unhappy.

[2]

+ if self.endpoint.port is not None:
+ return "%s://%s:%d%s" % (
+ self.endpoint.scheme, self.get_host(), self.endpoint.port, self.get_path())
+ else:
+ return "%s://%s%s" % (
+ self.endpoint.scheme, self.get_host(), self.get_path())

You can save a tiny bit of logic duplication by doing:

    def get_port(self):
        if self.endpoint.port is not None:
            return ":%d" % self.endpoint.port
        else:
            return ""

    def get_url(self):
        return "%s://%s%s%s" % (self.endpoint.scheme,
                                self.get_host(),
                                self.get_port(),
                                self.get_path())

review: Approve

=== modified file 'txaws/s3/client.py'
--- txaws/s3/client.py	2011-04-14 19:50:30 +0000
+++ txaws/s3/client.py	2011-08-29 19:06:23 +0000
@@ -57,8 +57,12 @@
         return path
 
     def get_url(self):
-        return "%s://%s%s" % (
-            self.endpoint.scheme, self.get_host(), self.get_path())
+        if self.endpoint.port is not None:
+            return "%s://%s:%d%s" % (
+                self.endpoint.scheme, self.get_host(), self.endpoint.port, self.get_path())
+        else:
+            return "%s://%s%s" % (
+                self.endpoint.scheme, self.get_host(), self.get_path())
 
 
 class S3Client(BaseClient):

=== modified file 'txaws/s3/tests/test_client.py'
--- txaws/s3/tests/test_client.py	2011-04-14 19:50:30 +0000
+++ txaws/s3/tests/test_client.py	2011-08-29 19:06:23 +0000
@@ -62,6 +62,29 @@
             url_context.get_url(),
             "http://localhost/mydocs/notes.txt")
 
+    def test_custom_port_endpoint(self):
+        test_uri='http://0.0.0.0:12345/'
+        endpoint = AWSServiceEndpoint(uri=test_uri)
+        self.assertEquals(endpoint.port, 12345)
+        self.assertEquals(endpoint.scheme, 'http')
+        context = client.URLContext(service_endpoint=endpoint,
+                bucket="foo",
+                object_name="bar")
+        self.assertEquals(context.get_host(), '0.0.0.0')
+        self.assertEquals(context.get_url(), test_uri + 'foo/bar')
+
+    def test_custom_port_endpoint_https(self):
+        test_uri='https://0.0.0.0:12345/'
+        endpoint = AWSServiceEndpoint(uri=test_uri)
+        self.assertEquals(endpoint.port, 12345)
+        self.assertEquals(endpoint.scheme, 'https')
+        context = client.URLContext(service_endpoint=endpoint,
+                bucket="foo",
+                object_name="bar")
+        self.assertEquals(context.get_host(), '0.0.0.0')
+        self.assertEquals(context.get_url(), test_uri + 'foo/bar')
+
+
 URLContextTestCase.skip = s3clientSkip