Merge lp:~milo/lava-tool/server-rpc-endpoint-fix into lp:~linaro-validation/lava-tool/trunk

[Milo Casagrande]

This merge proposal fixed the problem with how the server and rpc_endpoint parameters have been intended.
With these changes now "server" is just a name, and "rpc_endpoint" is the actual URL.

[Antonio Terceiro (terceiro)]

> === modified file 'lava_tool/utils.py'
> --- lava_tool/utils.py 2013-07-26 14:12:33 +0000
> +++ lava_tool/utils.py 2013-07-30 17:42:26 +0000
> @@ -281,7 +281,7 @@
> editor))
>
>
> -def verify_and_create_url(server, endpoint=""):
> +def verify_and_create_url(endpoint):
> """Checks that the provided values make a correct URL.
>
> If the server address does not contain a scheme, by default it will use
> @@ -291,25 +291,22 @@
> :param server: A server URL to verify.
> :return A URL.
> """
> - scheme, netloc, path, params, query, fragment = \
> - urlparse.urlparse(server)
> - if not scheme:
> - scheme = "https"
> - if not netloc:
> - netloc, path = path, ""
> -
> - if not netloc[-1:] == "/":
> - netloc += "/"
> -
> + url = ""
> if endpoint:
> - if endpoint[0] == "/":
> - endpoint = endpoint[1:]
> - if not endpoint[-1:] == "/":
> - endpoint += "/"
> - netloc += endpoint
> -
> - return urlparse.urlunparse(
> - (scheme, netloc, path, params, query, fragment))
> + scheme, netloc, path, params, query, fragment = \
> + urlparse.urlparse(endpoint)
> + if not scheme:
> + scheme = "https"
> + if not netloc:
> + netloc, path = path, ""
> +
> + url = urlparse.urlunparse(
> + (scheme, netloc, path, params, query, fragment))
> +
> + if url[-1:] != "/":
> + url += "/"
> +
> + return url

I don't understand why you need to parse the url just to unparse it a
few lines below. This function seems rather pointless to me.

[Milo Casagrande (milo)]

On Thu, Aug 1, 2013 at 2:04 AM, Antonio Terceiro
<email address hidden> wrote:
>
> I don't understand why you need to parse the url just to unparse it a
> few lines below. This function seems rather pointless to me.

Because if somebody when prompted for the server URL enters just
"127.0.0.1/RPC2", withouth the scheme, we will get a rather unpleasant
"IOError: unsupported XML-RPC protocol" error.

So, I need to look into the given URL and make sure we have a scheme,
or default to HTTPS, and that the URL has a trailing slash (since
looks like lava-tool auth-add adds it by default when adding an auth
token). That's why I parse the URL first, check what's necessary, and
then unparse it to create back the full URL.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Antonio Terceiro (terceiro)]

On Tue, Aug 06, 2013 at 09:37:08AM -0000, Milo Casagrande wrote:
> On Thu, Aug 1, 2013 at 2:04 AM, Antonio Terceiro
> <email address hidden> wrote:
> >
> > I don't understand why you need to parse the url just to unparse it a
> > few lines below. This function seems rather pointless to me.
>
> Because if somebody when prompted for the server URL enters just
> "127.0.0.1/RPC2", withouth the scheme, we will get a rather unpleasant
> "IOError: unsupported XML-RPC protocol" error.
>
> So, I need to look into the given URL and make sure we have a scheme,
> or default to HTTPS, and that the URL has a trailing slash (since
> looks like lava-tool auth-add adds it by default when adding an auth
> token). That's why I parse the URL first, check what's necessary, and
> then unparse it to create back the full URL.

OK - bring it in then

 review approve

review: Approve

=== modified file 'lava/helper/command.py'
--- lava/helper/command.py	2013-07-29 10:09:09 +0000
+++ lava/helper/command.py	2013-07-30 17:42:26 +0000
@@ -89,10 +89,10 @@
         rpc_endpoint_parameter = Parameter("rpc_endpoint",
                                            depends=server_name_parameter)
 
-        server_url = self.config.get(server_name_parameter)
+        self.config.get(server_name_parameter)
         endpoint = self.config.get(rpc_endpoint_parameter)
 
-        rpc_url = verify_and_create_url(server_url, endpoint)
+        rpc_url = verify_and_create_url(endpoint)
         server = AuthenticatingServerProxy(rpc_url,
                                            auth_backend=KeyringAuthBackend())
         return server

=== modified file 'lava_tool/tests/test_utils.py'
--- lava_tool/tests/test_utils.py	2013-07-26 09:30:11 +0000
+++ lava_tool/tests/test_utils.py	2013-07-30 17:42:26 +0000
@@ -243,7 +243,7 @@
 
     def test_verify_and_create_url_0(self):
         expected = "https://www.example.org/"
-        obtained = verify_and_create_url("www.example.org", "")
+        obtained = verify_and_create_url("www.example.org")
         self.assertEquals(expected, obtained)
 
     def test_verify_and_create_url_1(self):
@@ -253,12 +253,12 @@
 
     def test_verify_and_create_url_2(self):
         expected = "http://www.example.org/RPC/"
-        obtained = verify_and_create_url("http://www.example.org", "RPC")
+        obtained = verify_and_create_url("http://www.example.org/RPC")
         self.assertEquals(expected, obtained)
 
     def test_verify_and_create_url_3(self):
         expected = "https://www.example.org/RPC/"
-        obtained = verify_and_create_url("www.example.org/", "/RPC/")
+        obtained = verify_and_create_url("www.example.org/RPC")
         self.assertEquals(expected, obtained)
 
     def test_create_dir_0(self):

=== modified file 'lava_tool/utils.py'
--- lava_tool/utils.py	2013-07-26 14:12:33 +0000
+++ lava_tool/utils.py	2013-07-30 17:42:26 +0000
@@ -281,7 +281,7 @@
                                                            editor))
 
 
-def verify_and_create_url(server, endpoint=""):
+def verify_and_create_url(endpoint):
     """Checks that the provided values make a correct URL.
 
     If the server address does not contain a scheme, by default it will use
@@ -291,25 +291,22 @@
     :param server: A server URL to verify.
     :return A URL.
     """
-    scheme, netloc, path, params, query, fragment = \
-        urlparse.urlparse(server)
-    if not scheme:
-        scheme = "https"
-    if not netloc:
-        netloc, path = path, ""
-
-    if not netloc[-1:] == "/":
-        netloc += "/"
-
+    url = ""
     if endpoint:
-        if endpoint[0] == "/":
-            endpoint = endpoint[1:]
-        if not endpoint[-1:] == "/":
-            endpoint += "/"
-        netloc += endpoint
-
-    return urlparse.urlunparse(
-        (scheme, netloc, path, params, query, fragment))
+        scheme, netloc, path, params, query, fragment = \
+            urlparse.urlparse(endpoint)
+        if not scheme:
+            scheme = "https"
+        if not netloc:
+            netloc, path = path, ""
+
+        url = urlparse.urlunparse(
+            (scheme, netloc, path, params, query, fragment))
+
+        if url[-1:] != "/":
+            url += "/"
+
+    return url
 
 
 def create_dir(path, dir_name=None):