Added OAuth2 authentication to apply_issue

Added OAuth2 authentication to apply_issue

Added oauth2client and httplib to third_party

BUG=348233
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=259643

[pgervais, 2014-03-06 00:07:47]

I added OAuth2 authentication support to apply_issue. Chrome for iOS needs it. 

It's kind of brutal, but it works (tested on a real Rietveld running on
appengine.) Any idea to improve/clarify are welcome.

[pgervais, 2014-03-06 02:03:20]

I added another option to avoid doing convoluted things in recipes (namely
reading the client id from a file.)

[pgervais, 2014-03-07 19:27:29]

ping (feel free to hand the CR to someone else if you're swamped)

[pgervais, 2014-03-13 01:17:09]

Sorry to ping you again, but I really need this modification to make the Bling
CQ live.

[M-A Ruel, 2014-03-13 01:31:26]

Remove test files. Sorry I was sick in the past days, OOO before.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode58
apply_issue.py:58: '-w', '--password', default=None,
Remove this one while at it.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode60
apply_issue.py:60: 'Incompatible with -k')
prefer to keep alignment consistent.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode62
apply_issue.py:62: '-k', '--private-key-file', default=None,
default=None is the default, no need to specify it.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode63
apply_issue.py:63: help='Path to file containing a private key in p12 format for
OAuth2 '
You mean for a service account? That would be for the bots? I don't see this
being used by the users.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode122
apply_issue.py:122: print('file does not exist: %s' % str(options.email_file))
parser.error()
Then you don't need to return.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode122
apply_issue.py:122: print('file does not exist: %s' % str(options.email_file))
No need to str().

https://codereview.chromium.org/183793010/diff/20001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode443
rietveld.py:443: class OAuthRpcServer():
(object)

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode460
rietveld.py:460: host_parts = host.split('://')
that's unorthodox.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode489
rietveld.py:489: 
remove one line

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode494
rietveld.py:494: content_type="application/octet-stream",
use single quote throughout.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode501
rietveld.py:501: request_path: path on the server to hit. This is concatenated
with the
Align +2

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode512
rietveld.py:512: headers.update(headers)
ugh?

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode514
rietveld.py:514: headers = self.extra_headers
I'd do the copy unconditionally for simplicity; it's also shorter in code:

headers = self.extra_headers.copy()
headers.update(extra_headers or {})

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode520
rietveld.py:520: 
never 2 empty lines inside a function

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode525
rietveld.py:525: self._http.timeout = timeout
That's a tad surprising.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode528
rietveld.py:528: print('Trying URL: ' + url)
Remove.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode529
rietveld.py:529: args = dict(kwargs)
It's a dict already.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode540
rietveld.py:540: self._http.timeout = prev_timeout
Not a fan.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode543
rietveld.py:543: class OAuthRietveld(Rietveld):
But it's really JwtOAuth2Rietveld. Vadim can confirm his preferred semantic.

[Vadim Sh., 2014-03-13 18:28:41]

https://codereview.chromium.org/183793010/diff/20001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode460
rietveld.py:460: host_parts = host.split('://')
On 2014/03/13 01:31:27, M-A Ruel wrote:
> that's unorthodox.

Yeah.. Use urlparse module.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode476
rietveld.py:476: if self.host.endswith('/'):
self.host.rstrip('/')

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode488
rietveld.py:488: self._http = creds.authorize(http)
It makes the network call here, right? How apply_issue fails if private key is
invalid?

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode543
rietveld.py:543: class OAuthRietveld(Rietveld):
On 2014/03/13 01:31:27, M-A Ruel wrote:
> But it's really JwtOAuth2Rietveld. Vadim can confirm his preferred semantic.

I don't have preferred semantic, but I agree that JwtOAuth2Rietveld is more
fitting name.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode550
rietveld.py:550: def __init__(self,  # pylint: disable=W0231
Put pylint disable after ':'.

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
File third_party/oauth2client/client.py (right):

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
third_party/oauth2client/client.py:26: from .. import httplib2
Did you modify this code? I remember it used to be 'import httplib2' (I had to
hack sys.path to make it work). You need to document your modifications
somewhere (like provide revision number of original code plus *.diff you made).
I think we use README.chromium files for this?..

[pgervais, 2014-03-13 22:38:37]

Some answers to your comments, fixes on the way.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode58
apply_issue.py:58: '-w', '--password', default=None,
On 2014/03/13 01:31:27, M-A Ruel wrote:
> Remove this one while at it.

I think we're still using it for the public rietveld. I'd like to double-check
before removing it, at least.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode60
apply_issue.py:60: 'Incompatible with -k')
On 2014/03/13 01:31:27, M-A Ruel wrote:
> prefer to keep alignment consistent.

Done.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode62
apply_issue.py:62: '-k', '--private-key-file', default=None,
On 2014/03/13 01:31:27, M-A Ruel wrote:
> default=None is the default, no need to specify it.

alright, but explicit is better than implicit.
Done.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode63
apply_issue.py:63: help='Path to file containing a private key in p12 format for
OAuth2 '
On 2014/03/13 01:31:27, M-A Ruel wrote:
> You mean for a service account? That would be for the bots? I don't see this
> being used by the users.

It is indeed for the bots. No user will use that. 
I can use optparse.SUPPRESS_HELP if you think it makes more sense.

https://codereview.chromium.org/183793010/diff/20001/apply_issue.py#newcode122
apply_issue.py:122: print('file does not exist: %s' % str(options.email_file))
On 2014/03/13 01:31:27, M-A Ruel wrote:
> parser.error()
> Then you don't need to return.

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode443
rietveld.py:443: class OAuthRpcServer():
On 2014/03/13 01:31:27, M-A Ruel wrote:
> (object)

Good catch, thanks.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode460
rietveld.py:460: host_parts = host.split('://')
On 2014/03/13 18:28:42, Vadim Sh. wrote:
> On 2014/03/13 01:31:27, M-A Ruel wrote:
> > that's unorthodox.
> 
> Yeah.. Use urlparse module.

That's better indeed. But calling split() does exactly what is necessary :-)

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode476
rietveld.py:476: if self.host.endswith('/'):
On 2014/03/13 18:28:42, Vadim Sh. wrote:
> self.host.rstrip('/')

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode488
rietveld.py:488: self._http = creds.authorize(http)
On 2014/03/13 18:28:42, Vadim Sh. wrote:
> It makes the network call here, right? How apply_issue fails if private key is
> invalid?

This only decorates the http object, no network access is performed here. It
could fail only line 533.

But the question is valid. I have to double-check everything before actually
committing that.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode489
rietveld.py:489: 
On 2014/03/13 01:31:27, M-A Ruel wrote:
> remove one line

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode494
rietveld.py:494: content_type="application/octet-stream",
On 2014/03/13 01:31:27, M-A Ruel wrote:
> use single quote throughout.

I don't know why, but I find it horribly difficult to force myself to use single
quotes only. And double quotes are actually more difficult to get on a qwerty
layout!

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode501
rietveld.py:501: request_path: path on the server to hit. This is concatenated
with the
On 2014/03/13 01:31:27, M-A Ruel wrote:
> Align +2

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode512
rietveld.py:512: headers.update(headers)
On 2014/03/13 01:31:27, M-A Ruel wrote:
> ugh?

It took me some time to recall what I meant here :-).

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode514
rietveld.py:514: headers = self.extra_headers
On 2014/03/13 01:31:27, M-A Ruel wrote:
> I'd do the copy unconditionally for simplicity; it's also shorter in code:
> 
> headers = self.extra_headers.copy()
> headers.update(extra_headers or {})

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode520
rietveld.py:520: 
On 2014/03/13 01:31:27, M-A Ruel wrote:
> never 2 empty lines inside a function

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode529
rietveld.py:529: args = dict(kwargs)
On 2014/03/13 01:31:27, M-A Ruel wrote:
> It's a dict already.

I copy-pasted part of this code from upload.py, and that very line is in there.
There might have been a reason for it at some point...

Fixed.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode540
rietveld.py:540: self._http.timeout = prev_timeout
On 2014/03/13 01:31:27, M-A Ruel wrote:
> Not a fan.

I have no strong opinion on that matter. I'm fine with any solution you think
would be best.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode543
rietveld.py:543: class OAuthRietveld(Rietveld):
On 2014/03/13 18:28:42, Vadim Sh. wrote:
> On 2014/03/13 01:31:27, M-A Ruel wrote:
> > But it's really JwtOAuth2Rietveld. Vadim can confirm his preferred semantic.
> 
> I don't have preferred semantic, but I agree that JwtOAuth2Rietveld is more
> fitting name.

Done.

https://codereview.chromium.org/183793010/diff/20001/rietveld.py#newcode550
rietveld.py:550: def __init__(self,  # pylint: disable=W0231
On 2014/03/13 18:28:42, Vadim Sh. wrote:
> Put pylint disable after ':'.

Done.

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
File third_party/oauth2client/client.py (right):

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
third_party/oauth2client/client.py:26: from .. import httplib2
On 2014/03/13 18:28:42, Vadim Sh. wrote:
> Did you modify this code? I remember it used to be 'import httplib2' (I had to
> hack sys.path to make it work). You need to document your modifications
> somewhere (like provide revision number of original code plus *.diff you
made).
> I think we use README.chromium files for this?..

I haven't modified anything in httplib2, I just don't want the users (and the
bots) to be required to install it. Feel free to disagree.

[pgervais, 2014-03-13 22:44:40]

New patchset uploaded

[Vadim Sh., 2014-03-13 22:46:25]

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
File third_party/oauth2client/client.py (right):

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
third_party/oauth2client/client.py:26: from .. import httplib2
On 2014/03/13 22:38:38, pgervais wrote:
> On 2014/03/13 18:28:42, Vadim Sh. wrote:
> > Did you modify this code? I remember it used to be 'import httplib2' (I had
to
> > hack sys.path to make it work). You need to document your modifications
> > somewhere (like provide revision number of original code plus *.diff you
> made).
> > I think we use README.chromium files for this?..
> 
> I haven't modified anything in httplib2, I just don't want the users (and the
> bots) to be required to install it. Feel free to disagree.

Modifying is fine. What I say is that it's useful to have a diff of that
modification, so that it can be applied when third party code is updated.

Here's how I pulled httplib2 in swarming:
https://code.google.com/p/swarming/source/browse/?repo=client#git%2Fthird_par...

I propose you do something similar here (i.e. specify original revision and
modifications you made in README.chromium).

[pgervais, 2014-03-13 23:04:25]

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
File third_party/oauth2client/client.py (right):

https://codereview.chromium.org/183793010/diff/20001/third_party/oauth2client...
third_party/oauth2client/client.py:26: from .. import httplib2
On 2014/03/13 22:46:26, Vadim Sh. wrote:

> Modifying is fine. What I say is that it's useful to have a diff of that
> modification, so that it can be applied when third party code is updated.
> 
> Here's how I pulled httplib2 in swarming:
>
https://code.google.com/p/swarming/source/browse/?repo=client#git%252Fthird_p...
> 
> I propose you do something similar here (i.e. specify original revision and
> modifications you made in README.chromium).

What you've done looks great. I'll do the same.

[Vadim Sh., 2014-03-14 01:55:34]

Looks good.

Waiting for a diff to oauth2client :P

https://codereview.chromium.org/183793010/diff/40001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/40001/apply_issue.py#newcode131
apply_issue.py:131: options.email,
align indentation

[pgervais, 2014-03-14 15:41:27]

On 2014/03/14 01:55:34, Vadim Sh. wrote:
> Looks good.
> 
> Waiting for a diff to oauth2client :P
> 
I've only removed files, not modified any. I can add a diff that contains file
suppression, but it's kind of pointless. 
Is an empty diff file relevant?

[Vadim Sh., 2014-03-14 17:28:56]

On 2014/03/14 15:41:27, pgervais wrote:
> On 2014/03/14 01:55:34, Vadim Sh. wrote:
> > Looks good.
> > 
> > Waiting for a diff to oauth2client :P
> > 
> I've only removed files, not modified any. I can add a diff that contains file
> suppression, but it's kind of pointless. 
> Is an empty diff file relevant?

I look here:
https://code.google.com/p/google-api-python-client/source/browse/oauth2client...
And see 'import httplib2', and in code you're including it's "from .. import
httplib2".
If 'from .. import httplib2' were in some older revision, than fine, just
specify this revision. No need for empty diff.

[pgervais, 2014-03-14 17:36:57]

On 2014/03/14 17:28:56, Vadim Sh. wrote:
> On 2014/03/14 15:41:27, pgervais wrote:
> > On 2014/03/14 01:55:34, Vadim Sh. wrote:
> > > Looks good.
> > > 
> > > Waiting for a diff to oauth2client :P
> > > 
> > I've only removed files, not modified any. I can add a diff that contains
file
> > suppression, but it's kind of pointless. 
> > Is an empty diff file relevant?
> 
> I look here:
>
https://code.google.com/p/google-api-python-client/source/browse/oauth2client...
> And see 'import httplib2', and in code you're including it's "from .. import
> httplib2".
> If 'from .. import httplib2' were in some older revision, than fine, just
> specify this revision. No need for empty diff.

Sorry, I wasn't listening obviously. I was still thinking of httplib2. 
Will do it soon.

[pgervais, 2014-03-14 18:09:46]

Uploaded a new patchset, which hopefully addresses all your concerns.

[Vadim Sh., 2014-03-14 18:15:19]

https://codereview.chromium.org/183793010/diff/60001/third_party/oauth2client...
File third_party/oauth2client/README.depot_tools (right):

https://codereview.chromium.org/183793010/diff/60001/third_party/oauth2client...
third_party/oauth2client/README.depot_tools:1: Name: oauth2client
depot_tools seems to be using .chromium suffix: README.chromium
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/third_party/col...

https://codereview.chromium.org/183793010/diff/60001/third_party/oauth2client...
third_party/oauth2client/README.depot_tools:11: This library has been extracted
from the google appengine SDK.
Hm... Why not copy it from original repo?

[pgervais, 2014-03-14 18:31:50]

On 2014/03/14 18:15:19, Vadim Sh. wrote:
>
https://codereview.chromium.org/183793010/diff/60001/third_party/oauth2client...
> File third_party/oauth2client/README.depot_tools (right):
> 
>
https://codereview.chromium.org/183793010/diff/60001/third_party/oauth2client...
> third_party/oauth2client/README.depot_tools:1: Name: oauth2client
> depot_tools seems to be using .chromium suffix: README.chromium

Will fix that

>
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/third_party/col...
> 
>
https://codereview.chromium.org/183793010/diff/60001/third_party/oauth2client...
> third_party/oauth2client/README.depot_tools:11: This library has been
extracted
> from the google appengine SDK.
> Hm... Why not copy it from original repo?

So there is a separate repo :-)

[pgervais, 2014-03-14 18:40:37]

New patchset uploaded.
I took the occasion to check that apply_issue.py still works on
codereview.chromium.org.

[Vadim Sh., 2014-03-14 18:56:46]

https://codereview.chromium.org/183793010/diff/80001/third_party/oauth2client...
File third_party/oauth2client/MODIFICATIONS.diff (right):

https://codereview.chromium.org/183793010/diff/80001/third_party/oauth2client...
third_party/oauth2client/MODIFICATIONS.diff:20: -  from oauth2client.crypt
import Signer
I'm sorry for being extra pedantic, but IIUC this diff doesn't apply to version
1.2. See
https://code.google.com/p/google-api-python-client/source/browse/oauth2client...

It doesn't have 'from oauth2client.crypt import Signer' etc lines...

My end goal here is to simplify update of oauth2client in the future to:
1. Checkout a new version of oauth2client from it's home repo.
2. Apply diff and other mentioned modifications.
3. Copy to third_party/
4. ...
5. Profit!

For this to be easy, README.chromium should provide a reproducible way to get
the source that's in third_party/: i.e. checkout some specified revision, from
specified repository, apply diff and get code that is exact copy of what is in
third_party/ now.

Sorry again for being stubborn.

[pgervais, 2014-03-14 20:43:50]

On 2014/03/14 18:56:46, Vadim Sh. wrote:
>
https://codereview.chromium.org/183793010/diff/80001/third_party/oauth2client...
> File third_party/oauth2client/MODIFICATIONS.diff (right):
> 
>
https://codereview.chromium.org/183793010/diff/80001/third_party/oauth2client...
> third_party/oauth2client/MODIFICATIONS.diff:20: -  from oauth2client.crypt
> import Signer
> I'm sorry for being extra pedantic, but IIUC this diff doesn't apply to
version
> 1.2. See
>
https://code.google.com/p/google-api-python-client/source/browse/oauth2client...
> 
> It doesn't have 'from oauth2client.crypt import Signer' etc lines...
> 
> My end goal here is to simplify update of oauth2client in the future to:
> 1. Checkout a new version of oauth2client from it's home repo.
> 2. Apply diff and other mentioned modifications.
> 3. Copy to third_party/
> 4. ...
> 5. Profit!
> 
> For this to be easy, README.chromium should provide a reproducible way to get
> the source that's in third_party/: i.e. checkout some specified revision, from
> specified repository, apply diff and get code that is exact copy of what is in
> third_party/ now.
> 
> Sorry again for being stubborn.

You're not stubborn, you're doing your job right, and I don't :-)

[pgervais, 2014-03-14 21:24:42]

Alright, this time everything should be consistent (and work).

[Vadim Sh., 2014-03-14 21:55:24]

One final nit :)
Also, you answered all M-A's comments, right?

https://codereview.chromium.org/183793010/diff/100001/MODIFICATIONS.diff
File MODIFICATIONS.diff (right):

https://codereview.chromium.org/183793010/diff/100001/MODIFICATIONS.diff#newc...
MODIFICATIONS.diff:1: diff --git a/third_party/oauth2client/client.py
b/third_party/oauth2client/client.py
I think this file should be in third_party/oauth2client/MODIFICATIONS.diff, not
in the root?..

[pgervais, 2014-03-14 22:06:38]

On 2014/03/14 21:55:24, Vadim Sh. wrote:
> One final nit :)
> Also, you answered all M-A's comments, right?

I addressed all the critical ones, but didn't really know what to do with 'not a
fan' about setting the http timeout. I don't know what the concern is precisely.

> https://codereview.chromium.org/183793010/diff/100001/MODIFICATIONS.diff
> File MODIFICATIONS.diff (right):
> 
>
https://codereview.chromium.org/183793010/diff/100001/MODIFICATIONS.diff#newc...
> MODIFICATIONS.diff:1: diff --git a/third_party/oauth2client/client.py
> b/third_party/oauth2client/client.py
> I think this file should be in third_party/oauth2client/MODIFICATIONS.diff,
not
> in the root?..

I definitely need some sleep...

[Vadim Sh., 2014-03-14 23:04:40]

lgtm, but please wait for lgtm from M-A as well and be careful when landing this
change.

It has a potential to break all try bots, so probably it's better to land it
later in the day.

[pgervais, 2014-03-14 23:13:39]

On 2014/03/14 23:04:40, Vadim Sh. wrote:
> lgtm, but please wait for lgtm from M-A as well and be careful when landing
this
> change.
> 
> It has a potential to break all try bots, so probably it's better to land it
> later in the day.

I'm not crazy :-) I won't land a patch that can break all try bots and some
developers at 5pm on a Friday, right before a large maintenance operation :-)

[Vadim Sh., 2014-03-14 23:18:29]

On 2014/03/14 23:13:39, pgervais wrote:
> On 2014/03/14 23:04:40, Vadim Sh. wrote:
> > lgtm, but please wait for lgtm from M-A as well and be careful when landing
> this
> > change.
> > 
> > It has a potential to break all try bots, so probably it's better to land it
> > later in the day.
> 
> I'm not crazy :-) I won't land a patch that can break all try bots and some
> developers at 5pm on a Friday, right before a large maintenance operation :-)

I meant "later in a day, on a day you land it", i.e. at >5pm :)

[M-A Ruel, 2014-03-15 01:15:30]

few questiosn

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py#newcode131
apply_issue.py:131: options.email,
alignment

That's why I prefer +4.

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py#newcode133
apply_issue.py:133: private_key_password=options.password)
So password is the private key password in this case? It's not clear to the
user.

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py#newcode159
apply_issue.py:159: if sys.stdout.closed:
stdout?

https://codereview.chromium.org/183793010/diff/120001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/120001/rietveld.py#newcode469
rietveld.py:469: 
optional nit: I would not put these empty lines, it's not long enough IMHO

https://codereview.chromium.org/183793010/diff/120001/rietveld.py#newcode486
rietveld.py:486: self._http = creds.authorize(http)
self._http = creds.authorize(httplib2.Http(timeout=timeout))

[pgervais, 2014-03-17 17:35:10]

I've answered some of your comments, and pushed another patchset.

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py#newcode133
apply_issue.py:133: private_key_password=options.password)
On 2014/03/15 01:15:31, M-A Ruel wrote:
> So password is the private key password in this case? It's not clear to the
> user.

I've fixed the help for the -w option.

https://codereview.chromium.org/183793010/diff/120001/apply_issue.py#newcode159
apply_issue.py:159: if sys.stdout.closed:
On 2014/03/15 01:15:31, M-A Ruel wrote:
> stdout?

It was used like that up to now. Might be wrong though :-)

https://codereview.chromium.org/183793010/diff/120001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/120001/rietveld.py#newcode469
rietveld.py:469: 
On 2014/03/15 01:15:31, M-A Ruel wrote:
> optional nit: I would not put these empty lines, it's not long enough IMHO

Done.

https://codereview.chromium.org/183793010/diff/120001/rietveld.py#newcode486
rietveld.py:486: self._http = creds.authorize(http)
On 2014/03/15 01:15:31, M-A Ruel wrote:
> self._http = creds.authorize(httplib2.Http(timeout=timeout))

Done.

[pgervais, 2014-03-21 23:02:46]

Unless someone strongly objects, I'll commit this next Monday, around 4pm (low
tryserver usage and still someone in the office to help if something goes
wrong.)

[M-A Ruel, 2014-03-22 01:09:09]

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode59
apply_issue.py:59: help='Password for email addressed.  Use - to read password
from stdin. '
one space between . and next letter.

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode143
apply_issue.py:143: except urllib2.HTTPError, e:
Use style "except Foo as e:" everywhere

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode146
apply_issue.py:146: elif options.no_auth:
s/elif/if/

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode175
apply_issue.py:175: print('Downloading the patch.')
BTW, this function is well past what I consider a function too long. The
Connect, Download and Applying seems to be good places to cut it in functions.
Not requiring you to do this change right now but it should be considered
eventually.

https://codereview.chromium.org/183793010/diff/160001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/160001/rietveld.py#newcode538
rietveld.py:538: # The parent__init__ is not called on purpose.
Not a fan. Either split up the code into a third base class or make it work out.
I don't think it's a good design to skip calling the super class constructor.

[pgervais, 2014-03-24 20:27:35]

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py
File apply_issue.py (right):

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode59
apply_issue.py:59: help='Password for email addressed.  Use - to read password
from stdin. '
On 2014/03/22 01:09:09, M-A Ruel wrote:
> one space between . and next letter.

Done.

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode143
apply_issue.py:143: except urllib2.HTTPError, e:
On 2014/03/22 01:09:09, M-A Ruel wrote:
> Use style "except Foo as e:" everywhere

Done.

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode146
apply_issue.py:146: elif options.no_auth:
On 2014/03/22 01:09:09, M-A Ruel wrote:
> s/elif/if/

Done.

https://codereview.chromium.org/183793010/diff/160001/apply_issue.py#newcode175
apply_issue.py:175: print('Downloading the patch.')
On 2014/03/22 01:09:09, M-A Ruel wrote:
> BTW, this function is well past what I consider a function too long. The
> Connect, Download and Applying seems to be good places to cut it in functions.
> Not requiring you to do this change right now but it should be considered
> eventually.

I agree. Adding a TODO to keep track of that.

https://codereview.chromium.org/183793010/diff/160001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/183793010/diff/160001/rietveld.py#newcode538
rietveld.py:538: # The parent__init__ is not called on purpose.
On 2014/03/22 01:09:09, M-A Ruel wrote:
> Not a fan. Either split up the code into a third base class or make it work
out.
> I don't think it's a good design to skip calling the super class constructor.

Well, this class is kind of a hack anyway, because the base class design is
-hum- bad. I would have fixed the base class if I ever knew what/who was using
it, and how (tests?) Since depot_tools does not define a clear boundary, I
expect some random code I'm not aware of to break if I change anything in the
existing classes/functions. I might be overthinking it, but I have been bitten
by that in the past.

If you have a solution to fix the base class while not breaking anything, I'd be
happy to implement it.

[pgervais, 2014-03-24 23:15:30]

The CQ bit was checked by pgervais@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:15:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pgervais@chromium.org/183793010/180001

[commit-bot: I haz the power, 2014-03-24 23:16:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:16:51]

Presubmit check for 183793010-180001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/apply_issue.py
    depot_tools/rietveld.py
    depot_tools/third_party/httplib2/LICENSE
    depot_tools/third_party/httplib2/README.chromium
    depot_tools/third_party/httplib2/__init__.py
    depot_tools/third_party/httplib2/cacerts.txt
    depot_tools/third_party/httplib2/iri2uri.py
    depot_tools/third_party/httplib2/socks.py
    depot_tools/third_party/oauth2client/LICENSE
    depot_tools/third_party/oauth2client/MODIFICATIONS.diff
    depot_tools/third_party/oauth2client/README.chromium
    depot_tools/third_party/oauth2client/__init__.py
    depot_tools/third_party/oauth2client/anyjson.py
    depot_tools/third_party/oauth2client/appengine.py
    depot_tools/third_party/oauth2client/client.py
    depot_tools/third_party/oauth2client/clientsecrets.py
    depot_tools/third_party/oauth2client/crypt.py
    depot_tools/third_party/oauth2client/django_orm.py
    depot_tools/third_party/oauth2client/file.py
    depot_tools/third_party/oauth2client/gce.py
    depot_tools/third_party/oauth2client/keyring_storage.py
    depot_tools/third_party/oauth2client/locked_file.py
    depot_tools/third_party/oauth2client/multistore_file.py
    depot_tools/third_party/oauth2client/old_run.py
    depot_tools/third_party/oauth2client/tools.py
    depot_tools/third_party/oauth2client/util.py
    depot_tools/third_party/oauth2client/xsrfutil.py

tests/gcl_unittest.py (0.44s) failed
Traceback (most recent call last):
  File "tests/gcl_unittest.py", line 17, in <module>
    import gcl
  File "/b/commit-queue/workdir/tools/depot_tools/gcl.py", line 28, in <module>
    import git_cl
  File "/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line 38, in
<module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials


tests/presubmit_unittest.py (0.52s) failed
Traceback (most recent call last):
  File "tests/presubmit_unittest.py", line 26, in <module>
    import presubmit_support as presubmit
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials


tests/trychange_unittest.py (0.45s) failed
Traceback (most recent call last):
  File "tests/trychange_unittest.py", line 17, in <module>
    import trychange
  File "/b/commit-queue/workdir/tools/depot_tools/trychange.py", line 29, in
<module>
    import gcl
  File "/b/commit-queue/workdir/tools/depot_tools/gcl.py", line 28, in <module>
    import git_cl
  File "/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line 38, in
<module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials


tests/rietveld_test.py (0.43s) failed
Traceback (most recent call last):
  File "tests/rietveld_test.py", line 22, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials


tests/git_cl_test.py (0.36s) failed
Traceback (most recent call last):
  File "tests/git_cl_test.py", line 18, in <module>
    import git_cl
  File "/b/commit-queue/workdir/tools/depot_tools/git_cl.py", line 38, in
<module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials


save-description-on-failure.sh failed
Command
/b/commit-queue/workdir/tools/depot_tools/tests/save-description-on-failure.sh
returned non-zero exit status 1 in
/b/commit-queue/workdir/tools/depot_tools/tests
Setting up test upstream git repo...
Setting up test git repo...
TESTING: uploading to bogus server
TESTING: description was backed up
grep: /home/chrome-bot/.git_cl_description_backup: No such file or directory
FAILURE: description was backed up


push-basic.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/push-basic.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test upstream git repo...
Setting up test git repo...
TESTING: git-cl upload wants a server
FAILURE: git-cl upload wants a server


upstream.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/upstream.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
The --set-upstream flag is deprecated and will be removed. Consider using
--track or --set-upstream-to
Branch feature_branch set up to track remote branch trunk from origin.
TESTING: Guessing upstream branch for refs/remotes/origin/trunk
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line 38,
in <module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials
FAILURE: Guessing upstream branch for refs/remotes/origin/trunk


submit-from-new-dir.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/submit-from-new-dir.sh
returned non-zero exit status 1 in
/b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: upload succeeds
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line 38,
in <module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials
FAILURE: upload succeeds


abandon.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/abandon.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: upload succeeds
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line 38,
in <module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials
FAILURE: upload succeeds


submodule-merge-test.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/submodule-merge-test.sh
returned non-zero exit status 1 in
/b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test remote git-svn-submodule repo...
Switched to branch 'master'
TESTING: dcommitted code
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line 38,
in <module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials
FAILURE: dcommitted code


upload-local-tracking-branch.sh failed
Command
/b/commit-queue/workdir/tools/depot_tools/tests/upload-local-tracking-branch.sh
returned non-zero exit status 1 in
/b/commit-queue/workdir/tools/depot_tools/tests
Setting up test upstream git repo...
Setting up test git repo...
TESTING: upload succeeds (needs a server running on localhost)
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line 38,
in <module>
    import presubmit_support
  File "/b/commit-queue/workdir/tools/depot_tools/presubmit_support.py", line
46, in <module>
    import rietveld
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 30, in
<module>
    from third_party.oauth2client.client import SignedJwtAssertionCredentials
ImportError: cannot import name SignedJwtAssertionCredentials
FAILURE: upload succeeds (needs a server running on …
(message too large)

[pgervais, 2014-03-26 16:56:31]

I've fixed the issue by making the OpenSSL package optional, and added some more
logging. 

maruel@, can I have you opinion on this?

[M-A Ruel, 2014-03-26 18:27:13]

On 2014/03/26 16:56:31, pgervais wrote:
> I've fixed the issue by making the OpenSSL package optional, and added some
more
> logging. 

Good.

> maruel@, can I have you opinion on this?

God may have killed a kitten due to skipping the call to the super class'
__init__() but beside that, lgtm.

[pgervais, 2014-03-26 18:41:38]

The CQ bit was checked by pgervais@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:41:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pgervais@chromium.org/183793010/220001

[commit-bot: I haz the power, 2014-03-26 18:44:45]

Change committed as 259643