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

Proposed by Milo Casagrande
Status: Merged
Merged at revision: 191
Proposed branch: lp:~milo/lava-tool/server-rpc-endpoint-fix
Merge into: lp:~linaro-validation/lava-tool/trunk
Diff against target: 98 lines (+21/-24)
3 files modified
lava/helper/command.py (+2/-2)
lava_tool/tests/test_utils.py (+3/-3)
lava_tool/utils.py (+16/-19)
To merge this branch: bzr merge lp:~milo/lava-tool/server-rpc-endpoint-fix
Reviewer Review Type Date Requested Status
Antonio Terceiro Approve
Linaro Validation Team Pending
Review via email: mp+177658@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Antonio Terceiro (terceiro) wrote :

> === 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.

Revision history for this message
Milo Casagrande (milo) 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.

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

Revision history for this message
Antonio Terceiro (terceiro) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lava/helper/command.py'
2--- lava/helper/command.py 2013-07-29 10:09:09 +0000
3+++ lava/helper/command.py 2013-07-30 17:42:26 +0000
4@@ -89,10 +89,10 @@
5 rpc_endpoint_parameter = Parameter("rpc_endpoint",
6 depends=server_name_parameter)
7
8- server_url = self.config.get(server_name_parameter)
9+ self.config.get(server_name_parameter)
10 endpoint = self.config.get(rpc_endpoint_parameter)
11
12- rpc_url = verify_and_create_url(server_url, endpoint)
13+ rpc_url = verify_and_create_url(endpoint)
14 server = AuthenticatingServerProxy(rpc_url,
15 auth_backend=KeyringAuthBackend())
16 return server
17
18=== modified file 'lava_tool/tests/test_utils.py'
19--- lava_tool/tests/test_utils.py 2013-07-26 09:30:11 +0000
20+++ lava_tool/tests/test_utils.py 2013-07-30 17:42:26 +0000
21@@ -243,7 +243,7 @@
22
23 def test_verify_and_create_url_0(self):
24 expected = "https://www.example.org/"
25- obtained = verify_and_create_url("www.example.org", "")
26+ obtained = verify_and_create_url("www.example.org")
27 self.assertEquals(expected, obtained)
28
29 def test_verify_and_create_url_1(self):
30@@ -253,12 +253,12 @@
31
32 def test_verify_and_create_url_2(self):
33 expected = "http://www.example.org/RPC/"
34- obtained = verify_and_create_url("http://www.example.org", "RPC")
35+ obtained = verify_and_create_url("http://www.example.org/RPC")
36 self.assertEquals(expected, obtained)
37
38 def test_verify_and_create_url_3(self):
39 expected = "https://www.example.org/RPC/"
40- obtained = verify_and_create_url("www.example.org/", "/RPC/")
41+ obtained = verify_and_create_url("www.example.org/RPC")
42 self.assertEquals(expected, obtained)
43
44 def test_create_dir_0(self):
45
46=== modified file 'lava_tool/utils.py'
47--- lava_tool/utils.py 2013-07-26 14:12:33 +0000
48+++ lava_tool/utils.py 2013-07-30 17:42:26 +0000
49@@ -281,7 +281,7 @@
50 editor))
51
52
53-def verify_and_create_url(server, endpoint=""):
54+def verify_and_create_url(endpoint):
55 """Checks that the provided values make a correct URL.
56
57 If the server address does not contain a scheme, by default it will use
58@@ -291,25 +291,22 @@
59 :param server: A server URL to verify.
60 :return A URL.
61 """
62- scheme, netloc, path, params, query, fragment = \
63- urlparse.urlparse(server)
64- if not scheme:
65- scheme = "https"
66- if not netloc:
67- netloc, path = path, ""
68-
69- if not netloc[-1:] == "/":
70- netloc += "/"
71-
72+ url = ""
73 if endpoint:
74- if endpoint[0] == "/":
75- endpoint = endpoint[1:]
76- if not endpoint[-1:] == "/":
77- endpoint += "/"
78- netloc += endpoint
79-
80- return urlparse.urlunparse(
81- (scheme, netloc, path, params, query, fragment))
82+ scheme, netloc, path, params, query, fragment = \
83+ urlparse.urlparse(endpoint)
84+ if not scheme:
85+ scheme = "https"
86+ if not netloc:
87+ netloc, path = path, ""
88+
89+ url = urlparse.urlunparse(
90+ (scheme, netloc, path, params, query, fragment))
91+
92+ if url[-1:] != "/":
93+ url += "/"
94+
95+ return url
96
97
98 def create_dir(path, dir_name=None):

Subscribers

People subscribed via source and target branches