Issue 217380043: Swarming client code switched to support Cloud Endpoints API.
|
Created:
9 years, 1 month ago by cmassaro Modified:
9 years ago CC:
swarming-eng_googlegroups.com, csharp+cc_chromium.org, vadimsh+cc_chromium.org, pgervais Base URL:
https://code.google.com/p/swarming.client@master Visibility:
Public. |
DescriptionSwarming client code switched to support Cloud Endpoints API.
R=maruel@chromium.org,vadimsh@chromium.org
BUG=
Patch Set 1 #Patch Set 2 : Bot Delete Support #
Total comments: 14
Patch Set 3 : Updated with Requested Changes #Patch Set 4 : Fixed a Long Line #
Total comments: 16
Patch Set 5 : Fixed URL quoting; code cleanup. #
Total comments: 5
Patch Set 6 : Subparser-Style Frontend #Patch Set 7 : Slight Usage Change (Needed --) #MessagesTotal messages: 20
All tests are passing locally.
Sign in to reply to this message.
Just updating the bot_delete code; should I freeze this until the LUCI thing is settled?
Sign in to reply to this message.
Just updating the bot_delete code; should I freeze this until the LUCI thing is settled?
Sign in to reply to this message.
Sorry I totally missed the CL yesterday https://codereview.appspot.com/217380043/diff/20001/swarming.py File swarming.py (right): https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode8 swarming.py:8: __version__ = '0.6.2' bump to 0.7 https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode47 swarming.py:47: SWARMING_BASE = '_ah/api/swarming/v1' I'd just inline them all, I don't think using global variable increases overall code readability. In particular, line 1423 could append this string to options.swarming, so copy-pasting it everywhere would not be needed. Better is to create something like: options.swarming_api = options.swarming + '_ah/api/swarming/v1' then just replace the usage. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode69 swarming.py:69: '='.join(map(str, item)) for item in sorted(parameters.items()))) iteritems() you also need to urllib.quote_plus() https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode74 swarming.py:74: if method == 'GET': Method is determined implicitly based on the data argument https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode76 swarming.py:76: return json.loads(net.url_read(url, method=method)) I don't understand why you do not use url_read_json here https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode85 swarming.py:85: def _select_query_api(resource): We'll have to tell users to update their code. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode1386 swarming.py:1386: I generally prefer compact layout, so I'd recommend against this line since this class is not being modified.
Sign in to reply to this message.
On 2015/03/27 17:33:50, cmassaro wrote: > Just updating the bot_delete code; should I freeze this until the LUCI thing is > settled? If we can get it in and rolled into src/DEPS by Monday, it'd be great. It's not a big deal either way.
Sign in to reply to this message.
On 2015/03/27 18:41:49, M-A wrote: > On 2015/03/27 17:33:50, cmassaro wrote: > > Just updating the bot_delete code; should I freeze this until the LUCI thing > is > > settled? > > If we can get it in and rolled into src/DEPS by Monday, it'd be great. It's not > a big deal either way. +pgervais
Sign in to reply to this message.
https://codereview.appspot.com/217380043/diff/20001/swarming.py File swarming.py (right): https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode8 swarming.py:8: __version__ = '0.6.2' On 2015/03/27 17:58:30, M-A wrote: > bump to 0.7 Done. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode47 swarming.py:47: SWARMING_BASE = '_ah/api/swarming/v1' On 2015/03/27 17:58:30, M-A wrote: > I'd just inline them all, I don't think using global variable increases overall > code readability. > > In particular, line 1423 could append this string to options.swarming, so > copy-pasting it everywhere would not be needed. Better is to create something > like: > options.swarming_api = options.swarming + '_ah/api/swarming/v1' > > then just replace the usage. Done. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode69 swarming.py:69: '='.join(map(str, item)) for item in sorted(parameters.items()))) On 2015/03/27 17:58:30, M-A wrote: > iteritems() > you also need to urllib.quote_plus() Done. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode74 swarming.py:74: if method == 'GET': On 2015/03/27 17:58:30, M-A wrote: > Method is determined implicitly based on the data argument Done. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode76 swarming.py:76: return json.loads(net.url_read(url, method=method)) On 2015/03/27 17:58:30, M-A wrote: > I don't understand why you do not use url_read_json here Done. https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode85 swarming.py:85: def _select_query_api(resource): On 2015/03/27 17:58:30, M-A wrote: > We'll have to tell users to update their code. Hmm, in order to avoid this mapping? Should I get rid of all this and just accept the new endpoints as command-line arguments instead? https://codereview.appspot.com/217380043/diff/20001/swarming.py#newcode1386 swarming.py:1386: On 2015/03/27 17:58:30, M-A wrote: > I generally prefer compact layout, so I'd recommend against this line since this > class is not being modified. Done.
Sign in to reply to this message.
https://codereview.appspot.com/217380043/diff/60001/swarming.py File swarming.py (right): https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode61 swarming.py:61: url = '%s%s' % (url, urllib.quote_plus('?' + '&'.join('='.join( That's not doing what you think it does. In particular, this will quote everything. url += '?' + '&'.join( '%s=%s' % (urllib.quote_plus(str(k)), urllib.quote_plus(str(v))) for k, v in sorted(parameters.iteritems()) https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode66 swarming.py:66: def _format_and_make_request(url, request, method): I'd prefer to not have this function and just inline it instead https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode74 swarming.py:74: return {pair[u'key']: pair[u'value'] for pair in pair_list} Inline this one too, since it's exactly used once. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1030 swarming.py:1030: if net.url_read_json(url, data={'bot_id': bot}, method='DELETE') is None: Remove method='DELETE' since cloud endpoints do not use HTTP verbs. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1059 swarming.py:1059: request = {} request = { 'limit': limit, } https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1062 swarming.py:1062: request['cursor'] = urllib.quote(cursor) That's not needed, it's json encoded. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1064 swarming.py:1064: data = _format_and_make_request(url, request, 'GET') inline the function https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1177 swarming.py:1177: sys.stdout.write(request + '\n') Is that an error message?
Sign in to reply to this message.
https://codereview.appspot.com/217380043/diff/60001/swarming.py File swarming.py (right): https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode61 swarming.py:61: url = '%s%s' % (url, urllib.quote_plus('?' + '&'.join('='.join( On 2015/03/27 21:38:25, M-A wrote: > That's not doing what you think it does. In particular, this will quote > everything. > > url += '?' + '&'.join( > '%s=%s' % (urllib.quote_plus(str(k)), urllib.quote_plus(str(v))) > for k, v in sorted(parameters.iteritems()) Oh, I thought that's what was wanted. Fixed! https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode66 swarming.py:66: def _format_and_make_request(url, request, method): On 2015/03/27 21:38:25, M-A wrote: > I'd prefer to not have this function and just inline it instead Done. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode74 swarming.py:74: return {pair[u'key']: pair[u'value'] for pair in pair_list} On 2015/03/27 21:38:25, M-A wrote: > Inline this one too, since it's exactly used once. Done. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1059 swarming.py:1059: request = {} On 2015/03/27 21:38:25, M-A wrote: > request = { > 'limit': limit, > } Done. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1062 swarming.py:1062: request['cursor'] = urllib.quote(cursor) On 2015/03/27 21:38:25, M-A wrote: > That's not needed, it's json encoded. Done. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1064 swarming.py:1064: data = _format_and_make_request(url, request, 'GET') On 2015/03/27 21:38:25, M-A wrote: > inline the function Done. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1177 swarming.py:1177: sys.stdout.write(request + '\n') On 2015/03/27 21:38:25, M-A wrote: > Is that an error message? Done. https://codereview.appspot.com/217380043/diff/60001/swarming.py#newcode1177 swarming.py:1177: sys.stdout.write(request + '\n') On 2015/03/27 21:38:25, M-A wrote: > Is that an error message? Hmm, yes. Changed it to stderr. I kept the return code 0 for compatibility.
Sign in to reply to this message.
https://codereview.appspot.com/217380043/diff/80001/swarming.py File swarming.py (right): https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode84 swarming.py:84: mapping = zip(patterns, [ The more I think about it, the more: - Do not try to be backward compatible. query wasn't used much yet in automated ways. Send an internal email to alert people and that's it. - Stop accepting arbitrary stuff and code actual subcommands for each of the things that can be queried. In particular, I want to clarify the distinction between "high level" and "low level" commands. 'query' was the low level command, everything else being high level. In addition, only query has stable output (as json) so it's the only one that can be safely used in script. This difference should be clarified in the way the commands are designed. Maybe "query" isn't the best name. The reason I'm asking this right away is that _format_get_request() won't work with command line like: ./swarming.py query -S foo-bar "tasks?tag=asan:1" So I think it'd be better to code all the valid urls that can be used with query and encode the accepted parameters, which means implementing subcommands accordingly. https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode1171 swarming.py:1171: return 0 return 1 https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode1313 swarming.py:1313: cmd = properties.get('command') command is required, there must be one
Sign in to reply to this message.
On 2015/03/29 22:43:01, M-A wrote: > https://codereview.appspot.com/217380043/diff/80001/swarming.py > File swarming.py (right): > > https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode84 > swarming.py:84: mapping = zip(patterns, [ > The more I think about it, the more: > - Do not try to be backward compatible. query wasn't used much yet in automated > ways. Send an internal email to alert people and that's it. > - Stop accepting arbitrary stuff and code actual subcommands for each of the > things that can be queried. > > In particular, I want to clarify the distinction between "high level" and "low > level" commands. 'query' was the low level command, everything else being high > level. In addition, only query has stable output (as json) so it's the only one > that can be safely used in script. > > This difference should be clarified in the way the commands are designed. Maybe > "query" isn't the best name. > > The reason I'm asking this right away is that _format_get_request() won't work > with command line like: > ./swarming.py query -S foo-bar "tasks?tag=asan:1" Alright, that makes sense. If I understand correctly, we should now have a structure like ./swarming.py query* -S foo-bar <endpoint> <parameter(s)> * or its replacement(s); I think something like "query-bots" and "query-tasks" might work? and use subparsers (or sub-subparsers, as the case may be) to constrain access to endpoints. Is that correct? > > So I think it'd be better to code all the valid urls that can be used with query > and encode the accepted parameters, which means implementing subcommands > accordingly. > > https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode1171 > swarming.py:1171: return 0 > return 1 > > https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode1313 > swarming.py:1313: cmd = properties.get('command') > command is required, there must be one
Sign in to reply to this message.
On 2015/03/30 16:51:58, cmassaro wrote: > On 2015/03/29 22:43:01, M-A wrote: > > https://codereview.appspot.com/217380043/diff/80001/swarming.py > > File swarming.py (right): > > > > https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode84 > > swarming.py:84: mapping = zip(patterns, [ > > The more I think about it, the more: > > - Do not try to be backward compatible. query wasn't used much yet in > automated > > ways. Send an internal email to alert people and that's it. > > - Stop accepting arbitrary stuff and code actual subcommands for each of the > > things that can be queried. > > > > In particular, I want to clarify the distinction between "high level" and "low > > level" commands. 'query' was the low level command, everything else being high > > level. In addition, only query has stable output (as json) so it's the only > one > > that can be safely used in script. > > > > This difference should be clarified in the way the commands are designed. > Maybe > > "query" isn't the best name. > > > > The reason I'm asking this right away is that _format_get_request() won't work > > with command line like: > > ./swarming.py query -S foo-bar "tasks?tag=asan:1" > > Alright, that makes sense. If I understand correctly, we should now have a > structure like > > ./swarming.py query* -S foo-bar <endpoint> <parameter(s)> > > * or its replacement(s); I think something like "query-bots" and "query-tasks" > might work? > > and use subparsers (or sub-subparsers, as the case may be) to constrain access > to endpoints. Is that correct? Yes. query-bot / query-tasks sounds great but subcommands doesn't currently support '-' in names. So I used '-' for bot_delete, which is suboptimal, '_' is ugly. So I can add support for '-', it won't take long. I'll cc you on the CL.
Sign in to reply to this message.
I've implemented query-bots and query-tasks by sending the args blindly to a separate parser. Since OptionParserWithLogging uses optparse, rather than argparse, we don't have "real" subparser support, so I've kind of hacked this together, but I would be happy to implement that instead. However, if this is the right/an acceptable way to go, I'll update the tests accordingly (once M-A's hyphen support is landed) and try to make the help messages a bit more reasonable. https://codereview.appspot.com/217380043/diff/80001/swarming.py File swarming.py (right): https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode1171 swarming.py:1171: return 0 On 2015/03/29 22:43:01, M-A wrote: > return 1 Done. https://codereview.appspot.com/217380043/diff/80001/swarming.py#newcode1313 swarming.py:1313: cmd = properties.get('command') On 2015/03/29 22:43:00, M-A wrote: > command is required, there must be one Done.
Sign in to reply to this message.
I'll do the review after dinner. In the meantime please run the following; cd .. mv client ../luci-py cd ../luci-py git remote rm origin git remote add origin git@github.com:luci/luci-py git fetch origin git branch --set-upstream-to=origin/master git rebase origin/master git cl issue 0 git cl upl This renames your client directory to luci-py, removes the current origin and adds a new one. You'll have to send to me your github account via IM (or create one if you do not have one yet) so you can clone the repository with write access.
Sign in to reply to this message.
On 2015/03/30 21:09:32, M-A wrote: > I'll do the review after dinner. In the meantime please run the following; > > cd .. > mv client ../luci-py > cd ../luci-py > git remote rm origin > git remote add origin git@github.com:luci/luci-py > git fetch origin > git branch --set-upstream-to=origin/master > git rebase origin/master > git cl issue 0 > git cl upl > > This renames your client directory to luci-py, removes the current origin and > adds a new one. You'll have to send to me your github account via IM (or create > one if you do not have one yet) so you can clone the repository with write > access. Hmmm. This didn't work for me. Moving client a directory up and renaming to luci-py gives me this: https://paste.googleplex.com/4844919000662016. Just renaming (so leaving it a sister to appengine/) gives me this: https://paste.googleplex.com/5604902290063360. Do I have to modify .git manually?
Sign in to reply to this message.
On 2015/03/30 21:38:06, cmassaro wrote: > On 2015/03/30 21:09:32, M-A wrote: > > I'll do the review after dinner. In the meantime please run the following; > > > > cd .. > > mv client ../luci-py > > cd ../luci-py > > git remote rm origin > > git remote add origin git@github.com:luci/luci-py > > git fetch origin > > git branch --set-upstream-to=origin/master > > git rebase origin/master > > git cl issue 0 > > git cl upl > > > > This renames your client directory to luci-py, removes the current origin and > > adds a new one. You'll have to send to me your github account via IM (or > create > > one if you do not have one yet) so you can clone the repository with write > > access. > > Hmmm. This didn't work for me. Moving client a directory up and renaming to > luci-py gives me this: https://paste.googleplex.com/4844919000662016. > Just renaming (so leaving it a sister to appengine/) gives me this: > https://paste.googleplex.com/5604902290063360. > Do I have to modify .git manually?
Sign in to reply to this message.
On 2015/03/30 22:30:24, cmassaro wrote: > On 2015/03/30 21:38:06, cmassaro wrote: > > On 2015/03/30 21:09:32, M-A wrote: > > > I'll do the review after dinner. In the meantime please run the following; > > > > > > cd .. > > > mv client ../luci-py > > > cd ../luci-py > > > git remote rm origin > > > git remote add origin git@github.com:luci/luci-py > > > git fetch origin > > > git branch --set-upstream-to=origin/master > > > git rebase origin/master > > > git cl issue 0 > > > git cl upl > > > > > > This renames your client directory to luci-py, removes the current origin > and > > > adds a new one. You'll have to send to me your github account via IM (or > > create > > > one if you do not have one yet) so you can clone the repository with write > > > access. > > > > Hmmm. This didn't work for me. Moving client a directory up and renaming to > > luci-py gives me this: https://paste.googleplex.com/4844919000662016. > > Just renaming (so leaving it a sister to appengine/) gives me this: > > https://paste.googleplex.com/5604902290063360. > > Do I have to modify .git manually? Alright, luci-py is in place and is recognized as a git repo. I've got cleanup to do there, but it seems to have worked. Once I have access to the github repo, I should be alright.
Sign in to reply to this message.
On 2015/03/30 22:31:27, cmassaro wrote: > On 2015/03/30 22:30:24, cmassaro wrote: > > On 2015/03/30 21:38:06, cmassaro wrote: > > > On 2015/03/30 21:09:32, M-A wrote: > > > > I'll do the review after dinner. In the meantime please run the following; > > > > > > > > cd .. > > > > mv client ../luci-py > > > > cd ../luci-py > > > > git remote rm origin > > > > git remote add origin git@github.com:luci/luci-py > > > > git fetch origin > > > > git branch --set-upstream-to=origin/master > > > > git rebase origin/master > > > > git cl issue 0 > > > > git cl upl > > > > > > > > This renames your client directory to luci-py, removes the current origin > > and > > > > adds a new one. You'll have to send to me your github account via IM (or > > > create > > > > one if you do not have one yet) so you can clone the repository with write > > > > access. > > > > > > Hmmm. This didn't work for me. Moving client a directory up and renaming to > > > luci-py gives me this: https://paste.googleplex.com/4844919000662016. > > > Just renaming (so leaving it a sister to appengine/) gives me this: > > > https://paste.googleplex.com/5604902290063360. > > > Do I have to modify .git manually? Ah sorry, I forgot this wouldn't work due to the fact that this was checked out as a submodule. > Alright, luci-py is in place and is recognized as a git repo. I've got cleanup > to do there, but it seems to have worked. Once I have access to the github repo, > I should be alright. Done.
Sign in to reply to this message.
Fuvk u shandon <3 dev
Sign in to reply to this message.
On 3/30/15, ShandonHildreth09@gmail.com <ShandonHildreth09@gmail.com> wrote: > Fuvk u shandon <3 dev > > > https://codereview.appspot.com/217380043/ >
Sign in to reply to this message.
|
