Reland Implement owners check in presubmit for Gerrit.

presubmit_support.py

Index: presubmit_support.py
diff --git a/presubmit_support.py b/presubmit_support.py
index 07cf7b24b088993f31f390a84bdafff4cc0acf0d..187663ea0436a76edb6eeae53e9727e9fa3614d5 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -197,6 +197,63 @@ class _MailTextResult(_PresubmitResult):
     super(_MailTextResult, self).__init__()
     raise NotImplementedError()
 
+class GerritAccessor(object):
+  """Limited Gerrit functionality for canned presubmit checks to work.
+
+  To avoid excessive Gerrit calls, caches the results.
+  """
+
+  def __init__(self, host):
+    self.host = host
+    self.cache = {}
+
+  def _FetchChangeDetail(self, issue):
+    # Separate function to be easily mocked in tests.
+    return gerrit_util.GetChangeDetail(
+        self.host, str(issue),
+        ['ALL_REVISIONS', 'DETAILED_LABELS'])
+
+  def GetChangeInfo(self, issue, force=False):
+    assert issue
+    cache_key = int(issue)
+    if force or cache_key not in self.cache:
+      self.cache[cache_key] = self._FetchChangeDetail(issue)
+    return self.cache[cache_key]
+
+  def GetChangeDescription(self, issue, patchset=None, force=False):

[Michael Achenbach, 2016/04/28 07:17:30]

Who's using force=True?

[tandrii(chromium), 2016/04/28 19:31:50]

nobody any more! Removed.

+    """If patchset is none, fetches current patchset."""
+    self.GetChangeInfo(issue, force=force)  # Ensure details are in cache.

[Michael Achenbach, 2016/04/28 07:17:30]

Why not:
info = self.GetChangeInfo(issue, force=force)

delete next line and s/self.cache[cache_key]/info/g below

[tandrii(chromium), 2016/04/28 19:31:50]

good question. I thought modifying cache explicitly, rather than implicitly, is
better. But, your suggestion + comment is even better, i think.

+    cache_key = int(issue)
+
+    if patchset:

[Michael Achenbach, 2016/04/28 07:17:30]

I assume gerrit patchset numbers are not starting with 0.

[tandrii(chromium), 2016/04/28 19:31:51]

yes, they don't.

+      for rev, rev_info in self.cache[cache_key]['revisions'].iteritems():
+        if str(rev_info['_number']) == str(patchset):
+          break
+      else:
+        raise Exception('patchset %s doesn\'t exist in issue %s' % (
+            patchset, issue))
+    else:
+      rev = self.cache[cache_key]['current_revision']
+      rev_info = self.cache[cache_key]['revisions'][rev]

[Michael Achenbach, 2016/04/28 07:17:30]

Can you explain of what each of the revisions is comprised? Above it looks like
a rev, rev_info tuple. Now you just assign to rev_info.

[tandrii(chromium), 2016/04/28 19:31:50]

rev_info is dict; rev, rev_info is a tuple, but that's from iteritems().
actual example is in test. Should a comment here be useful?

[Michael Achenbach, 2016/04/29 08:10:18]

No, just me being stupid. Got it now.

+
+    if 'real_description' not in rev_info:
+      rev_info['real_description'] = (
+          gerrit_util.GetChangeDescriptionFromGitiles(
+              rev_info['fetch']['http']['url'], rev))
+    return rev_info['real_description']
+
+  def GetChangeOwner(self, issue):
+    return self.GetChangeInfo(issue)['owner']['email']
+
+  def GetChangeReviewers(self, issue, approving_only=True):
+    # Gerrit has 'approved' sub-section, but it only lists 1 approver.
+    # So, if we look only for approvers, we have to look at all anyway.
+    # Also, assume LGTM means Code-Review label == 2. Other configurations
+    # aren't supported.
+    return [r['email']
+            for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all']

[Michael Achenbach, 2016/04/28 07:17:30]

Do you have a reference to a place that explains how the change info is built
up? Otherwise I can only rubber stamp this.

OK - later I looked at the test which explains it somewhat. So never mind.

[tandrii(chromium), 2016/04/28 19:31:50]

https://gerrit-review.googlesource.com/Documentation/rest-api.html is the 
sorta-reference, but it's not clear from it how 'labels' section looks like.
Still, it's better than rietveld API :)

+            if not approving_only or '2' == str(r.get('value', 0))]
+
 
 class OutputApi(object):
   """An instance of OutputApi gets passed to presubmit scripts so that they
@@ -265,7 +322,7 @@ class InputApi(object):
   )
 
   def __init__(self, change, presubmit_path, is_committing,
-      rietveld_obj, verbose, dry_run=None):
+      rietveld_obj, verbose, dry_run=None, gerrit_obj=None):
     """Builds an InputApi object.
 
     Args:
@@ -280,6 +337,7 @@ class InputApi(object):
     self.is_committing = is_committing
     self.rietveld = rietveld_obj
     self.dry_run = dry_run
+    self.gerrit = gerrit_obj
     # TBD
     self.host_url = 'http://codereview.chromium.org'
     if self.rietveld:
@@ -1349,7 +1407,7 @@ def DoPostUploadExecuter(change,
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, rietveld_obj, verbose,
-               dry_run=None):
+      dry_run=None, gerrit_obj=None):

[Michael Achenbach, 2016/04/28 07:17:30]

nit: Maybe keep ( alignment? Or move all into a new line.

[tandrii(chromium), 2016/04/28 19:31:51]

Done.

     """
     Args:
       change: The Change object.
@@ -1359,6 +1417,7 @@ class PresubmitExecuter(object):
     self.change = change
     self.committing = committing
     self.rietveld = rietveld_obj
+    self.gerrit = gerrit_obj
     self.verbose = verbose
     self.dry_run = dry_run
 
@@ -1381,7 +1440,7 @@ class PresubmitExecuter(object):
     # Load the presubmit script into context.
     input_api = InputApi(self.change, presubmit_path, self.committing,
                          self.rietveld, self.verbose,
-                         dry_run=self.dry_run)
+                         self.dry_run, self.gerrit)
     context = {}
     try:
       exec script_text in context
@@ -1424,7 +1483,8 @@ def DoPresubmitChecks(change,
                       default_presubmit,
                       may_prompt,
                       rietveld_obj,
-                      dry_run=None):
+                      dry_run=None,
+                      gerrit_obj=None):

[Michael Achenbach, 2016/04/28 07:17:30]

Just some order nit: The dry_run feature is quite new, right? Maybe you can
sneak in gerrit_obj next to rietveld_obj here without breaking the world?

[tandrii(chromium), 2016/04/28 19:31:50]

Done.

   """Runs all presubmit checks that apply to the files in the change.
 
   This finds all PRESUBMIT.py files in directories enclosing the files in the
@@ -1444,6 +1504,7 @@ def DoPresubmitChecks(change,
     may_prompt: Enable (y/n) questions on warning or error.
     rietveld_obj: rietveld.Rietveld object.
     dry_run: if true, some Checks will be skipped.
+    gerrit_obj: provides basic Gerrit codereview functionality.
 
   Warning:
     If may_prompt is true, output_stream SHOULD be sys.stdout and input_stream
@@ -1471,7 +1532,7 @@ def DoPresubmitChecks(change,
       output.write("Warning, no PRESUBMIT.py found.\n")
     results = []
     executer = PresubmitExecuter(change, committing, rietveld_obj, verbose,
-                                 dry_run=dry_run)
+                                 dry_run, gerrit_obj)
     if default_presubmit:
       if verbose:
         output.write("Running default presubmit script.\n")
@@ -1696,7 +1757,8 @@ def main(argv=None):
     parser.error('For unversioned directory, <files> is not optional.')
   logging.info('Found %d file(s).' % len(files))
 
-  rietveld_obj = None
+  rietveld_obj, gerrit_obj = None, None
+
   if options.rietveld_url:
     # The empty password is permitted: '' is not None.
     if options.rietveld_private_key_file:
@@ -1718,21 +1780,11 @@ def main(argv=None):
       logging.info('Got description: """\n%s\n"""', options.description)
 
   if options.gerrit_url and options.gerrit_fetch:
-    rietveld_obj = None
     assert options.issue and options.patchset
-    props = gerrit_util.GetChangeDetail(
-        urlparse.urlparse(options.gerrit_url).netloc, str(options.issue),
-        ['ALL_REVISIONS'])
-    options.author = props['owner']['email']
-    for rev, rev_info in props['revisions'].iteritems():
-      if str(rev_info['_number']) == str(options.patchset):
-        options.description = gerrit_util.GetChangeDescriptionFromGitiles(
-            rev_info['fetch']['http']['url'], rev)
-        break
-    else:
-      print >> sys.stderr, ('Patchset %d was not found in Gerrit issue %d' %
-                            options.patchset, options.issue)
-      return 2
+    rietveld_obj = None

[Michael Achenbach, 2016/04/28 07:17:29]

So, a hybrid is not supported? :)

E.g. a CL that's uploaded to rietveld and gerrit and presubmit kicks in for
both?

Never mind this comment.

[tandrii(chromium), 2016/04/28 19:31:50]

Well, just call it twice with two sets of args :)
too late :D

+    gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
+    options.author = gerrit_obj.GetChangeOwner(options.issue)
+    options.description = gerrit_obj.GetChangeDescription(options.patchset)
     logging.info('Got author: "%s"', options.author)
     logging.info('Got description: """\n%s\n"""', options.description)
 
@@ -1754,7 +1806,8 @@ def main(argv=None):
           options.default_presubmit,
           options.may_prompt,
           rietveld_obj,
-          options.dry_run)
+          options.dry_run,
+          gerrit_obj)
     return not results.should_continue()
   except NonexistantCannedCheckFilter, e:
     print >> sys.stderr, (