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
1=== modified file 'lava_scheduler_tool/commands.py'
2--- lava_scheduler_tool/commands.py 2011-08-19 04:32:11 +0000
3+++ lava_scheduler_tool/commands.py 2011-09-06 10:45:37 +0000
4@@ -16,11 +16,27 @@
5 # You should have received a copy of the GNU Lesser General Public License
6 # along with lava-scheduler-tool. If not, see <http://www.gnu.org/licenses/>.
7
8+import getpass
9+import urlparse
10+import xmlrpclib
11
12-from lava_tool.authtoken import AuthenticatingServerProxy, KeyringAuthBackend
13-from lava_tool.interface import Command
14+from lava_tool.authtoken import (
15+ AuthenticatingServerProxy,
16+ KeyringAuthBackend,
17+ MemoryAuthBackend,
18+ )
19+from lava_tool.interface import Command, LavaCommandError
20 from lava_tool.commands import ExperimentalCommandMixIn
21
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
30+
31
32 class submit_job(ExperimentalCommandMixIn, Command):
33 """
34@@ -32,11 +48,51 @@
35 super(submit_job, cls).register_arguments(parser)
36 parser.add_argument("SERVER")
37 parser.add_argument("JSON_FILE")
38+ parser.add_argument(
39+ "--token-file", default=None,
40+ help="Read the password from a file rather than prompting for it.")
41
42 def invoke(self):
43+ uri = normalize_xmlrpc_url(self.args.SERVER)
44+ parsed_host = urlparse.urlparse(uri)
45+
46+ if parsed_host.username:
47+ username = parsed_host.username
48+ else:
49+ username = getpass.getuser()
50+
51+ host = parsed_host.hostname
52+ if parsed_host.port:
53+ host += ':' + str(parsed_host.port)
54+
55+ if self.args.token_file:
56+ if parsed_host.password:
57+ raise LavaCommandError(
58+ "Token specified in url but --token-file also passed.");
59+ else:
60+ try:
61+ token_file = open(self.args.token_file)
62+ except IOError as ex:
63+ raise LavaCommandError(
64+ "opening %r failed: %s" % (self.args.token_file, ex))
65+ token = token_file.read().strip()
66+ else:
67+ if parsed_host.password:
68+ token = parsed_host.password
69+ else:
70+ token = getpass.getpass("Paste token for %s: " % uri)
71+
72+ userless_uri = '%s://%s%s' % (
73+ parsed_host.scheme, host, parsed_host.path)
74+
75 self.print_experimental_notice()
76+
77 server = AuthenticatingServerProxy(
78- self.args.SERVER, auth_backend=KeyringAuthBackend())
79+ uri, auth_backend = MemoryAuthBackend(
80+ [(username, userless_uri, token)]))
81+# server = AuthenticatingServerProxy(
82+# self.args.SERVER, auth_backend=KeyringAuthBackend())
83+
84 with open(self.args.JSON_FILE, 'rb') as stream:
85 command_text = stream.read()
86 print "submitted as job id:", server.scheduler.submit_job(command_text)

Subscribers

People subscribed via source and target branches