my_activity: Port gerrit to the new gerrit_util API.

my_activity: Port gerrit to the new gerrit_util API.

This patch adds a new function QueryAllChanges to gerrit_util.py
allowing the caller to iterate the list of changes regardless the
maximum limit of changes per request that the server supports (by
default 500 according to gerrit's documentation).

my_activity.py is ported to use this function instead of urllib2 to
manually make the request. This also adds support for authentication
since gerrit_util.py already supports it, and the internal gerrit
instance is now re-enabled.

Finally, two minor bugs are fixed on the hanlding of returned
results: The DETAILED_ACCOUNTS option is passed to gerrit to request
the email addresses of the referenced users and users without an
email address, such as the "Gerrit Code Review" user on the internal
gerrit, are now supported.

BUG=chromium:311649, chromium:281695
TEST=Manual run "my_activity.py -u USER" for some users.

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

[deymo, 2013-10-29 04:59:29]

Sorry for the SPAM... Rietveld gave me an HTTP 500 and I get a checksum error on
my change :(

Original message:
Sorry for fixing this, but I needed to write my snippets and the my_activity.py
was still broken.

I hope you like it... at least it works for me =)

[deymo, 2013-10-30 18:26:10]

On 2013/10/29 04:59:29, deymo wrote:
> Sorry for the SPAM... Rietveld gave me an HTTP 500 and I get a checksum error
on
> my change :(
> 
> Original message:
> Sorry for fixing this, but I needed to write my snippets and the
my_activity.py
> was still broken.
> 
> I hope you like it... at least it works for me =)

I can haz review? https://memegen.googleplex.com/6249218888957952

[szager1, 2013-10-30 21:05:45]

Can you please work with hinoka@ to resolve this with:

https://codereview.chromium.org/46413002/

[szager1, 2013-10-30 21:06:29]

On 2013/10/30 21:05:45, szager1 wrote:
> Can you please work with hinoka@ to resolve this with:
> 
> https://codereview.chromium.org/46413002/

s/resolve/reconcile/

[Ryan Tseng, 2013-10-30 22:20:45]

https://codereview.chromium.org/50283002/diff/50001/my_activity.py
File my_activity.py (right):

https://codereview.chromium.org/50283002/diff/50001/my_activity.py#newcode409
my_activity.py:409: return gerrit_util.QueryAllChanges(instance['url'], req,
Try/Catch gerrit_util.GerritError here, print the error like line 421 before the
change if caught (in case of 403), and exit cleanly (return [])

I think the behavior previously was to continue on 403, whereas this would just
throw and exit.

[Ryan Tseng, 2013-10-30 22:27:13]

https://codereview.chromium.org/50283002/diff/50001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/50283002/diff/50001/gerrit_util.py#newcode87
gerrit_util.py:87: 'url': '/a/%s' % path,
Add in the /a/ url change from
https://codereview.chromium.org/46413002/diff/1/gerrit_util.py (ignore the
review_auth stuff)

[deymo, 2013-10-30 22:32:31]

Try/catch added (new patch). Tested renaming my ~/.netrc and it shows:

ERROR: Looking up 'chromium-review.googlesource.com': Unauthorized

PTAL.

https://codereview.chromium.org/50283002/diff/50001/my_activity.py
File my_activity.py (right):

https://codereview.chromium.org/50283002/diff/50001/my_activity.py#newcode409
my_activity.py:409: return gerrit_util.QueryAllChanges(instance['url'], req,
On 2013/10/30 22:20:45, Ryan T. wrote:
> Try/Catch gerrit_util.GerritError here, print the error like line 421 before
the
> change if caught (in case of 403), and exit cleanly (return [])
> 
> I think the behavior previously was to continue on 403, whereas this would
just
> throw and exit.

Agreed.

[deymo, 2013-10-30 23:01:25]

Added the change to support requests to chromium-review.googlesource.com even if
~/.netrc is not found. (didn't see hinoka's comment before).

ERROR: Looking up 'chrome-internal-review.googlesource.com': Moved Temporarily

https://codereview.chromium.org/50283002/diff/50001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/50283002/diff/50001/gerrit_util.py#newcode87
gerrit_util.py:87: 'url': '/a/%s' % path,
On 2013/10/30 22:27:14, Ryan T. wrote:
> Add in the /a/ url change from
> https://codereview.chromium.org/46413002/diff/1/gerrit_util.py (ignore the
> review_auth stuff)

Done.

[Ryan Tseng, 2013-10-30 23:02:52]

lgtm + szager for approval

[szager1, 2013-10-31 18:15:44]

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode72
gerrit_util.py:72: url = '/a/%s' % path
It's always possible that the Authorization header will be passed to this
method, rather than taken from NETRC.  So, it's better to:

 if auth:
   headers.setdefault(...)
 else:
   LOGGER.debug('No authorization found in netrc')
 if 'Authorization' in headers and not path.startswith('a/'):
   url = '/a/%s' % path
 else:
   url = '/%s' % path

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode80
gerrit_util.py:80: LOGGER.debug('%s %s://%s/a/%s' % (reqtype, GERRIT_PROTOCOL,
host, path))
Use the new 'url' variable.

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode186
gerrit_util.py:186: def QueryAllChanges(host, param_dict, first_param=None,
limit=500,
This method name is not suggestive of what the code does; it sounds like it's
going to return all changes on the server.  And generator methods should have
the word 'generate' in their name.

Maybe 'GenerateChanges' ?

Also, the parameter list and docstring need not be so verbose.  I suggest
something like this:

def GenerateChanges(*args, **kwargs):
  """
  Iterate over the results of a change query, automatically making continuation
requests if necessary.

  Args: Refer to QueryChanges, above.
  """

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode216
gerrit_util.py:216: more_changes = [cl for cl in page if '_more_changes' in cl]
This logic is a bit obscure, since '_more_changes' should only be set on the
last cl.  The code would be more readable like this:

 while True:
   page = QueryChanges(...)
   for cl in page:
     yield cl
   if not page or '_more_changes' not in page[-1]:
     break
   sortkey = page[-1]['_sortkey']

https://codereview.chromium.org/50283002/diff/180001/my_activity.py
File my_activity.py (right):

https://codereview.chromium.org/50283002/diff/180001/my_activity.py#newcode410
my_activity.py:410: # Instanciate the generator to force all the requests now
and catch the
s/Instanciate/Instantiate/

[deymo, 2013-10-31 20:42:12]

Went through the comments and partially included them, PTAL.

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode72
gerrit_util.py:72: url = '/a/%s' % path
On 2013/10/31 18:15:45, szager1 wrote:
> It's always possible that the Authorization header will be passed to this
> method, rather than taken from NETRC.  So, it's better to:
> 
>  if auth:
>    headers.setdefault(...)
>  else:
>    LOGGER.debug('No authorization found in netrc')
>  if 'Authorization' in headers and not path.startswith('a/'):
>    url = '/a/%s' % path
>  else:
>    url = '/%s' % path

Done.

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode80
gerrit_util.py:80: LOGGER.debug('%s %s://%s/a/%s' % (reqtype, GERRIT_PROTOCOL,
host, path))
On 2013/10/31 18:15:45, szager1 wrote:
> Use the new 'url' variable.

Done.

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode186
gerrit_util.py:186: def QueryAllChanges(host, param_dict, first_param=None,
limit=500,
The point of having the documentation here is that you can access it not only
from the source file but also from the python interpreter (like with "python -c
'import gerrit_util; help(gerrit_util.QueryAllChanges)'"  or simply
"gerrit_util.QueryAllChanges?" if you are using ipython).

Having a pointer on the documentation and variadic arguments hides the real list
of arguments you can pass here, which is indeed fixed. On the other hand,
although the names of the arguments are the same, the meaning is not exactly the
same due to the nature of this function. The argument limit is less relevant
here: you can pass any value there and you will get the whole list anyway (at
least once the gerrit-team fixes all the bugs there ;-) ). The sortkey argument
is a starting point for the query, but other sortkey values will be passed to
QueryChanges from this function.

I'll try to make a balance here.

https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode216
gerrit_util.py:216: more_changes = [cl for cl in page if '_more_changes' in cl]
I try to avoid the "while True:" loops with breaks in the middle. The logic in
those loops is more confusing because you need to follow execution to know when
does the loop stop. The "while more_changes:" clearly shows the intention of the
loop (run while there are more changes to fetch).

The loop's body is more robust because it accepts _more_changes anywhere and
continues from there. This is because gerrit supports the P query argument to
fetch the results in reverse mode. Even if QueryChanges doesn't currently use
the P query argument (just the N), getting the _sortkey value from wherever the
_more_changes was set seems more appropriate to me.

[deymo, 2013-11-05 00:30:16]

On 2013/10/31 20:42:12, deymo wrote:
> Went through the comments and partially included them, PTAL.
> 
> https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py
> File gerrit_util.py (right):
> 
> https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode72
> gerrit_util.py:72: url = '/a/%s' % path
> On 2013/10/31 18:15:45, szager1 wrote:
> > It's always possible that the Authorization header will be passed to this
> > method, rather than taken from NETRC.  So, it's better to:
> > 
> >  if auth:
> >    headers.setdefault(...)
> >  else:
> >    LOGGER.debug('No authorization found in netrc')
> >  if 'Authorization' in headers and not path.startswith('a/'):
> >    url = '/a/%s' % path
> >  else:
> >    url = '/%s' % path
> 
> Done.
> 
> https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode80
> gerrit_util.py:80: LOGGER.debug('%s %s://%s/a/%s' % (reqtype, GERRIT_PROTOCOL,
> host, path))
> On 2013/10/31 18:15:45, szager1 wrote:
> > Use the new 'url' variable.
> 
> Done.
> 
> https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode186
> gerrit_util.py:186: def QueryAllChanges(host, param_dict, first_param=None,
> limit=500,
> The point of having the documentation here is that you can access it not only
> from the source file but also from the python interpreter (like with "python
-c
> 'import gerrit_util; help(gerrit_util.QueryAllChanges)'"  or simply
> "gerrit_util.QueryAllChanges?" if you are using ipython).
> 
> Having a pointer on the documentation and variadic arguments hides the real
list
> of arguments you can pass here, which is indeed fixed. On the other hand,
> although the names of the arguments are the same, the meaning is not exactly
the
> same due to the nature of this function. The argument limit is less relevant
> here: you can pass any value there and you will get the whole list anyway (at
> least once the gerrit-team fixes all the bugs there ;-) ). The sortkey
argument
> is a starting point for the query, but other sortkey values will be passed to
> QueryChanges from this function.
> 
> I'll try to make a balance here.
> 
> https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode216
> gerrit_util.py:216: more_changes = [cl for cl in page if '_more_changes' in
cl]
> I try to avoid the "while True:" loops with breaks in the middle. The logic in
> those loops is more confusing because you need to follow execution to know
when
> does the loop stop. The "while more_changes:" clearly shows the intention of
the
> loop (run while there are more changes to fetch).
> 
> The loop's body is more robust because it accepts _more_changes anywhere and
> continues from there. This is because gerrit supports the P query argument to
> fetch the results in reverse mode. Even if QueryChanges doesn't currently use
> the P query argument (just the N), getting the _sortkey value from wherever
the
> _more_changes was set seems more appropriate to me.

Friendly ping.

[Jorge Lucangeli Obes, 2013-11-06 00:21:36]

On 2013/11/05 00:30:16, deymo wrote:
> On 2013/10/31 20:42:12, deymo wrote:
> > Went through the comments and partially included them, PTAL.
> > 
> > https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py
> > File gerrit_util.py (right):
> > 
> >
https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode72
> > gerrit_util.py:72: url = '/a/%s' % path
> > On 2013/10/31 18:15:45, szager1 wrote:
> > > It's always possible that the Authorization header will be passed to this
> > > method, rather than taken from NETRC.  So, it's better to:
> > > 
> > >  if auth:
> > >    headers.setdefault(...)
> > >  else:
> > >    LOGGER.debug('No authorization found in netrc')
> > >  if 'Authorization' in headers and not path.startswith('a/'):
> > >    url = '/a/%s' % path
> > >  else:
> > >    url = '/%s' % path
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode80
> > gerrit_util.py:80: LOGGER.debug('%s %s://%s/a/%s' % (reqtype,
GERRIT_PROTOCOL,
> > host, path))
> > On 2013/10/31 18:15:45, szager1 wrote:
> > > Use the new 'url' variable.
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode186
> > gerrit_util.py:186: def QueryAllChanges(host, param_dict, first_param=None,
> > limit=500,
> > The point of having the documentation here is that you can access it not
only
> > from the source file but also from the python interpreter (like with "python
> -c
> > 'import gerrit_util; help(gerrit_util.QueryAllChanges)'"  or simply
> > "gerrit_util.QueryAllChanges?" if you are using ipython).
> > 
> > Having a pointer on the documentation and variadic arguments hides the real
> list
> > of arguments you can pass here, which is indeed fixed. On the other hand,
> > although the names of the arguments are the same, the meaning is not exactly
> the
> > same due to the nature of this function. The argument limit is less relevant
> > here: you can pass any value there and you will get the whole list anyway
(at
> > least once the gerrit-team fixes all the bugs there ;-) ). The sortkey
> argument
> > is a starting point for the query, but other sortkey values will be passed
to
> > QueryChanges from this function.
> > 
> > I'll try to make a balance here.
> > 
> >
https://codereview.chromium.org/50283002/diff/180001/gerrit_util.py#newcode216
> > gerrit_util.py:216: more_changes = [cl for cl in page if '_more_changes' in
> cl]
> > I try to avoid the "while True:" loops with breaks in the middle. The logic
in
> > those loops is more confusing because you need to follow execution to know
> when
> > does the loop stop. The "while more_changes:" clearly shows the intention of
> the
> > loop (run while there are more changes to fetch).
> > 
> > The loop's body is more robust because it accepts _more_changes anywhere and
> > continues from there. This is because gerrit supports the P query argument
to
> > fetch the results in reverse mode. Even if QueryChanges doesn't currently
use
> > the P query argument (just the N), getting the _sortkey value from wherever
> the
> > _more_changes was set seems more appropriate to me.
> 
> Friendly ping.

my_activity.py is broken for all Chrome OS developers, and this CL fixes it.
Would it be possible to get it reviewed? Thanks!

[szager1, 2013-11-06 00:34:44]

lgtm with nit.

https://codereview.chromium.org/50283002/diff/250001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/50283002/diff/250001/gerrit_util.py#newcode217
gerrit_util.py:217: more_changes = [cl for cl in page if '_more_changes' in cl]
Call it paranoia, but I'd like an assert here:

assert len(more_changes) < 2

[deymo, 2013-11-06 01:00:01]

Added the check for len(more_changes) < 2 raising a GerritError if not. Other
functions in this code raise a GerritError(200, ...) for spec problems with the
gerrit response and the code using gerrit_util.py expects this exception.

[commit-bot: I haz the power, 2013-11-06 01:00:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/50283002/320001

[commit-bot: I haz the power, 2013-11-06 01:02:01]

Change committed as 233166