Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
236093002:
Added POST capability to oauth Rietveld (Closed)
|
Created:
6 years, 8 months ago by pgervais Modified:
6 years, 8 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionAdded POST capability to oauth Rietveld
BUG=319446
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=263750
Patch Set 1 #
Total comments: 11
Patch Set 2 : Code cleanup #Patch Set 3 : Fixed log message #
Total comments: 8
Patch Set 4 : Style fixes #Messages
Total messages: 14 (0 generated)
I defer review to Sergey
Some comments. https://codereview.chromium.org/236093002/diff/1/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/236093002/diff/1/presubmit_support.py#newcode... presubmit_support.py:1560: parser.error("Only one of --rietveld_email and --rietveld_email_file " English: 'and' --> 'or' ("only one of foo or bar can be...") https://codereview.chromium.org/236093002/diff/1/presubmit_support.py#newcode... presubmit_support.py:1563: parser.error("Only one of --rietveld_private_key_file and " English: 'and' --> 'or' https://codereview.chromium.org/236093002/diff/1/rietveld.py File rietveld.py (right): https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode519 rietveld.py:519: # raise NotImplementedError('POST requests are not yet supported.') Let's remove this comment. https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode530 rietveld.py:530: logging.info(url) Please add a meaningful message to the log. https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode577 rietveld.py:577: logging.info('Using login: %s' % client_email) Would it make sense to say 'Using OAuth login: %s' to make logs more self-explanatory?
https://codereview.chromium.org/236093002/diff/1/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/236093002/diff/1/presubmit_support.py#newcode... presubmit_support.py:1560: parser.error("Only one of --rietveld_email and --rietveld_email_file " On 2014/04/14 20:59:42, Sergey Berezin wrote: > English: 'and' --> 'or' ("only one of foo or bar can be...") Done. https://codereview.chromium.org/236093002/diff/1/presubmit_support.py#newcode... presubmit_support.py:1563: parser.error("Only one of --rietveld_private_key_file and " On 2014/04/14 20:59:42, Sergey Berezin wrote: > English: 'and' --> 'or' Done. https://codereview.chromium.org/236093002/diff/1/rietveld.py File rietveld.py (right): https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode519 rietveld.py:519: # raise NotImplementedError('POST requests are not yet supported.') On 2014/04/14 20:59:42, Sergey Berezin wrote: > Let's remove this comment. Done. https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode530 rietveld.py:530: logging.info(url) On 2014/04/14 20:59:42, Sergey Berezin wrote: > Please add a meaningful message to the log. Removed it (was for testing, and forgotten)
LGTM + one remaining comment (optional suggestion). BTW, you still need someone else's stamp, as I'm not an owner of depot_tools. https://codereview.chromium.org/236093002/diff/1/rietveld.py File rietveld.py (right): https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode577 rietveld.py:577: logging.info('Using login: %s' % client_email) On 2014/04/14 20:59:42, Sergey Berezin wrote: > Would it make sense to say 'Using OAuth login: %s' to make logs more > self-explanatory? What about this comment?
I've fixed the last issue. Adding stip@ and maruel@ to reviewers because they're owners on depot_tools, and I've not bothered them with a review for a while. https://codereview.chromium.org/236093002/diff/1/rietveld.py File rietveld.py (right): https://codereview.chromium.org/236093002/diff/1/rietveld.py#newcode577 rietveld.py:577: logging.info('Using login: %s' % client_email) On 2014/04/14 22:49:46, Sergey Berezin wrote: > On 2014/04/14 20:59:42, Sergey Berezin wrote: > > Would it make sense to say 'Using OAuth login: %s' to make logs more > > self-explanatory? > > What about this comment? > Sorry, I've overlooked it completely. I definitely makes sense. I'll take care of that.
https://codereview.chromium.org/236093002/diff/40001/rietveld.py File rietveld.py (right): https://codereview.chromium.org/236093002/diff/40001/rietveld.py#newcode447 rietveld.py:447: client_email, you sure this is always an email? https://codereview.chromium.org/236093002/diff/40001/rietveld.py#newcode567 rietveld.py:567: if not bot_url.endswith('/bots'): why not just specify /bots originally?
https://codereview.chromium.org/236093002/diff/40001/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/236093002/diff/40001/presubmit_support.py#new... presubmit_support.py:1544: # These are for OAuth2 authentication for bots. See also apply_issue.py Then make it an option group? https://codereview.chromium.org/236093002/diff/40001/presubmit_support.py#new... presubmit_support.py:1588: raise ValueError("No password or secret key has been provided for " parser.error()
Answered some comments, and uploaded another CL. https://codereview.chromium.org/236093002/diff/40001/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/236093002/diff/40001/presubmit_support.py#new... presubmit_support.py:1544: # These are for OAuth2 authentication for bots. See also apply_issue.py On 2014/04/14 23:29:10, M-A Ruel wrote: > Then make it an option group? I don't really see the point here since there is no help displayed. However, grouping all the above --rietveld options together could be useful. https://codereview.chromium.org/236093002/diff/40001/presubmit_support.py#new... presubmit_support.py:1588: raise ValueError("No password or secret key has been provided for " On 2014/04/14 23:29:10, M-A Ruel wrote: > parser.error() Done. https://codereview.chromium.org/236093002/diff/40001/rietveld.py File rietveld.py (right): https://codereview.chromium.org/236093002/diff/40001/rietveld.py#newcode447 rietveld.py:447: client_email, On 2014/04/14 23:23:39, stip wrote: > you sure this is always an email? When I think of it, no. But appengine provides to different kinds of identifier: an id and an email, and it's always very confusing. Stating that the email should be used here helps a lot. But your point is valid: on other systems than appengine, it might be something else than an email :-/ There does not seem to be any constraint in the OAuth2 RFC at least. https://codereview.chromium.org/236093002/diff/40001/rietveld.py#newcode567 rietveld.py:567: if not bot_url.endswith('/bots'): On 2014/04/14 23:23:39, stip wrote: > why not just specify /bots originally? In practice, I found it more natural to specify the root URL, because it does not depend on the authentication method. Adding /bots here also makes it easier to change that prefix later on. Adding '/bots' when it's missing is rather bad practice. As a reviewer I think I would complain about it :-) Maybe I'll drop it and prey that doesn't break any other code that I've written.
lgtm
The CQ bit was checked by pgervais@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pgervais@chromium.org/236093002/60001
Message was sent while issue was closed.
Change committed as 263750
Message was sent while issue was closed.
On 2014/04/15 00:30:39, I haz the power (commit-bot) wrote: > Change committed as 263750 This change may have broken the presubmit trybots. http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3300 http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu... Presubmit log: Usage: presubmit_support.py [options] <files...> presubmit_support.py: error: No password or secret key has been provided for Rietveld. Unable to connect. |
