Merge lp:~dpigott/lava-scheduler-tool/fix821400-add-token-file into lp:lava-scheduler-tool

Proposed by Dave Pigott on 2011-09-06
Status: Needs review
Proposed branch: lp:~dpigott/lava-scheduler-tool/fix821400-add-token-file
Merge into: lp:lava-scheduler-tool
Diff against target: 86 lines (+59/-3)
1 file modified
lava_scheduler_tool/commands.py (+59/-3)
To merge this branch: bzr merge lp:~dpigott/lava-scheduler-tool/fix821400-add-token-file
Reviewer Review Type Date Requested Status
Zygmunt Krynicki (community) Needs Information on 2011-11-09
Michael Hudson-Doyle (community) 2011-09-06 Needs Fixing on 2011-09-19
Review via email: mp+74197@code.launchpad.net

Description of the change

Added code to support --token-file to the submit-job command. In the process realised that cancel-job requires the same functionality and that there is now code duplication between auth-add and submit-job. Will open that work as a separate bug.

To post a comment you must log in.
Zygmunt Krynicki (zyga) wrote :

We can probably remove that line

81 +# server = AuthenticatingServerProxy(
82 +# self.args.SERVER, auth_backend=KeyringAuthBackend())

While this looks okay I think parsing the URL would be saner for bogus URLs.
22 +def normalize_xmlrpc_url(uri):
23 + if '://' not in uri:
24 + uri = 'http://' + uri
25 + if not uri.endswith('/'):
26 + uri += '/'
27 + if not uri.endswith('/RPC2/'):
28 + uri += 'RPC2/'
29 + return uri

Instead of checking this manually you can use MutuallyExclusiveGroup from argparse. It makes --help document this and allows you to express the same limitation without client-side code. See: http://argparse.googlecode.com/svn/trunk/doc/other-methods.html#mutual-exclusion

56 + if parsed_host.password:
57 + raise LavaCommandError(
58 + "Token specified in url but --token-file also passed.");

Dave Pigott (dpigott) wrote :

> We can probably remove that line
>
> 81 +# server = AuthenticatingServerProxy(
> 82 +# self.args.SERVER, auth_backend=KeyringAuthBackend())

I thought I had before I checked in. It certainly went just after! I'll push it up again.

> While this looks okay I think parsing the URL would be saner for bogus URLs.

In what way do you mean? This is just so that people can put in a truncated URL. Are you suggesting I should verify that the URL is actually a legitimate structure?

> 22 +def normalize_xmlrpc_url(uri):
> 23 + if '://' not in uri:
> 24 + uri = 'http://' + uri
> 25 + if not uri.endswith('/'):
> 26 + uri += '/'
> 27 + if not uri.endswith('/RPC2/'):
> 28 + uri += 'RPC2/'
> 29 + return uri

> Instead of checking this manually you can use MutuallyExclusiveGroup from
> argparse. It makes --help document this and allows you to express the same
> limitation without client-side code. See:
> http://argparse.googlecode.com/svn/trunk/doc/other-methods.html#mutual-
> exclusion
>
> 56 + if parsed_host.password:
> 57 + raise LavaCommandError(
> 58 + "Token specified in url but --token-file also passed.");

OK. Noted.

Zygmunt Krynicki (zyga) wrote :

> > While this looks okay I think parsing the URL would be saner for bogus URLs.
>
> In what way do you mean? This is just so that people can put in a truncated
> URL. Are you suggesting I should verify that the URL is actually a legitimate
> structure?

Well instead of using "://" in url and things like that we could parse the url with things like urlparse/urlsplit and then only act on the path component. I'm not advocating for it but it would seem cleaner.

Dave Pigott (dpigott) wrote :

> Instead of checking this manually you can use MutuallyExclusiveGroup from
> argparse. It makes --help document this and allows you to express the same
> limitation without client-side code. See:
> http://argparse.googlecode.com/svn/trunk/doc/other-methods.html#mutual-
> exclusion
>
> 56 + if parsed_host.password:
> 57 + raise LavaCommandError(
> 58 + "Token specified in url but --token-file also passed.");

Actually, when I came to look at this, it doesn't apply. One is a command line option, the other is a part of the url as broken up by url parse, so I can't make a mutual exclusion. Pity. It's a really nice feature.

Zygmunt Krynicki (zyga) wrote :

> > Instead of checking this manually you can use MutuallyExclusiveGroup from
> > argparse. It makes --help document this and allows you to express the same
> > limitation without client-side code. See:
> > http://argparse.googlecode.com/svn/trunk/doc/other-methods.html#mutual-
> > exclusion
> >
> > 56 + if parsed_host.password:
> > 57 + raise LavaCommandError(
> > 58 + "Token specified in url but --token-file also passed.");
>
> Actually, when I came to look at this, it doesn't apply. One is a command line
> option, the other is a part of the url as broken up by url parse, so I can't
> make a mutual exclusion. Pity. It's a really nice feature.

Ah, quite right. My mistake, sorry about that.

ZK

Michael Hudson-Doyle (mwhudson) wrote :

Apologies for not commenting earlier. I really thought I had :/

This change means that the keyring backend will never be used! I don't think that's right. I've made some changes in https://code.launchpad.net/~dpigott/lava-scheduler-tool/fix821400-add-token-file (extracting fiddling around for a token to a function, and use the keyring backend if no token is otherwise supplied). Perhaps a function that gets an AuthenticatingServerProxy given a server url and a token file name (or even an argparse options object) needs to be added to lava-tool...

review: Needs Fixing
Dave Pigott (dpigott) wrote :

> Apologies for not commenting earlier. I really thought I had :/
>
> This change means that the keyring backend will never be used! I don't think
> that's right. I've made some changes in https://code.launchpad.net/~dpigott
> /lava-scheduler-tool/fix821400-add-token-file (extracting fiddling around for
> a token to a function, and use the keyring backend if no token is otherwise
> supplied).

I think I've misunderstood something. Pulled that branch and it looks like what I checked in. Is that not what you meant?

> Perhaps a function that gets an AuthenticatingServerProxy given a
> server url and a token file name (or even an argparse options object) needs to
> be added to lava-tool...

That's kind of the direction I've been going in on the cancel-job addition and code consolidation.

Michael Hudson-Doyle (mwhudson) wrote :

On Mon, 19 Sep 2011 08:02:34 -0000, Dave Pigott <email address hidden> wrote:
> > Apologies for not commenting earlier. I really thought I had :/
> >
> > This change means that the keyring backend will never be used! I don't think
> > that's right. I've made some changes in https://code.launchpad.net/~dpigott
> > /lava-scheduler-tool/fix821400-add-token-file (extracting fiddling around for
> > a token to a function, and use the keyring backend if no token is otherwise
> > supplied).
>
> I think I've misunderstood something. Pulled that branch and it looks like what I checked in. Is that not what you meant?

Aaargh I meant
https://code.launchpad.net/~mwhudson/lava-scheduler-tool/fix821400-add-token-file

Sorry about that.

> > Perhaps a function that gets an AuthenticatingServerProxy given a
> > server url and a token file name (or even an argparse options object) needs to
> > be added to lava-tool...
>
> That's kind of the direction I've been going in on the cancel-job addition and code consolidation.

Cool.

Cheers,
mwh

Zygmunt Krynicki (zyga) wrote :

Michael, what is the status of this MP, is it going to land or can it be rejected?

review: Needs Information
Michael Hudson-Doyle (mwhudson) wrote :

There have been no revisions pushed since I made my comments, I've been waiting for Dave to take a look at my branch.

Dave Pigott (dpigott) wrote :

Other things got in the way. When everything quietens down here I'll review. I think Michael's branch is probably right, but need to look.

Unmerged revisions

9. By Dave Pigott on 2011-09-06

Added authentiction token file support to submit-job

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lava_scheduler_tool/commands.py'
--- lava_scheduler_tool/commands.py 2011-08-19 04:32:11 +0000
+++ lava_scheduler_tool/commands.py 2011-09-06 10:45:37 +0000
@@ -16,11 +16,27 @@
16# You should have received a copy of the GNU Lesser General Public License16# You should have received a copy of the GNU Lesser General Public License
17# along with lava-scheduler-tool. If not, see <http://www.gnu.org/licenses/>.17# along with lava-scheduler-tool. If not, see <http://www.gnu.org/licenses/>.
1818
19import getpass
20import urlparse
21import xmlrpclib
1922
20from lava_tool.authtoken import AuthenticatingServerProxy, KeyringAuthBackend23from lava_tool.authtoken import (
21from lava_tool.interface import Command24 AuthenticatingServerProxy,
25 KeyringAuthBackend,
26 MemoryAuthBackend,
27 )
28from lava_tool.interface import Command, LavaCommandError
22from lava_tool.commands import ExperimentalCommandMixIn29from lava_tool.commands import ExperimentalCommandMixIn
2330
31def normalize_xmlrpc_url(uri):
32 if '://' not in uri:
33 uri = 'http://' + uri
34 if not uri.endswith('/'):
35 uri += '/'
36 if not uri.endswith('/RPC2/'):
37 uri += 'RPC2/'
38 return uri
39
2440
25class submit_job(ExperimentalCommandMixIn, Command):41class submit_job(ExperimentalCommandMixIn, Command):
26 """42 """
@@ -32,11 +48,51 @@
32 super(submit_job, cls).register_arguments(parser)48 super(submit_job, cls).register_arguments(parser)
33 parser.add_argument("SERVER")49 parser.add_argument("SERVER")
34 parser.add_argument("JSON_FILE")50 parser.add_argument("JSON_FILE")
51 parser.add_argument(
52 "--token-file", default=None,
53 help="Read the password from a file rather than prompting for it.")
3554
36 def invoke(self):55 def invoke(self):
56 uri = normalize_xmlrpc_url(self.args.SERVER)
57 parsed_host = urlparse.urlparse(uri)
58
59 if parsed_host.username:
60 username = parsed_host.username
61 else:
62 username = getpass.getuser()
63
64 host = parsed_host.hostname
65 if parsed_host.port:
66 host += ':' + str(parsed_host.port)
67
68 if self.args.token_file:
69 if parsed_host.password:
70 raise LavaCommandError(
71 "Token specified in url but --token-file also passed.");
72 else:
73 try:
74 token_file = open(self.args.token_file)
75 except IOError as ex:
76 raise LavaCommandError(
77 "opening %r failed: %s" % (self.args.token_file, ex))
78 token = token_file.read().strip()
79 else:
80 if parsed_host.password:
81 token = parsed_host.password
82 else:
83 token = getpass.getpass("Paste token for %s: " % uri)
84
85 userless_uri = '%s://%s%s' % (
86 parsed_host.scheme, host, parsed_host.path)
87
37 self.print_experimental_notice()88 self.print_experimental_notice()
89
38 server = AuthenticatingServerProxy(90 server = AuthenticatingServerProxy(
39 self.args.SERVER, auth_backend=KeyringAuthBackend())91 uri, auth_backend = MemoryAuthBackend(
92 [(username, userless_uri, token)]))
93# server = AuthenticatingServerProxy(
94# self.args.SERVER, auth_backend=KeyringAuthBackend())
95
40 with open(self.args.JSON_FILE, 'rb') as stream:96 with open(self.args.JSON_FILE, 'rb') as stream:
41 command_text = stream.read()97 command_text = stream.read()
42 print "submitted as job id:", server.scheduler.submit_job(command_text)98 print "submitted as job id:", server.scheduler.submit_job(command_text)

Subscribers

People subscribed via source and target branches