aurora-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jco...@apache.org
Subject aurora git commit: Support in-flight review chains via `depends-on`.
Date Thu, 24 Dec 2015 16:26:17 GMT
Repository: aurora
Updated Branches:
  refs/heads/master 9176b6423 -> 1ae77d596


Support in-flight review chains via `depends-on`.

This adds support for following `depends-on` chains of in-flight RBs to
form patch sets ultimately based off master.

Request processing logic is factored up into a helper class that main
drives in a loop over pending RBs.

Reviewed at https://reviews.apache.org/r/41659/


Project: http://git-wip-us.apache.org/repos/asf/aurora/repo
Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/1ae77d59
Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/1ae77d59
Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/1ae77d59

Branch: refs/heads/master
Commit: 1ae77d59654f7a4d5610fe016f6a1ef76f73f993
Parents: 9176b64
Author: John Sirois <john.sirois@gmail.com>
Authored: Thu Dec 24 10:25:51 2015 -0600
Committer: Joshua Cohen <jcohen@apache.org>
Committed: Thu Dec 24 10:25:51 2015 -0600

----------------------------------------------------------------------
 build-support/jenkins/review_feedback.py | 250 +++++++++++++++-----------
 1 file changed, 147 insertions(+), 103 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/aurora/blob/1ae77d59/build-support/jenkins/review_feedback.py
----------------------------------------------------------------------
diff --git a/build-support/jenkins/review_feedback.py b/build-support/jenkins/review_feedback.py
index ee37742..aa2c836 100755
--- a/build-support/jenkins/review_feedback.py
+++ b/build-support/jenkins/review_feedback.py
@@ -25,6 +25,7 @@ import sys
 import urllib
 import urllib2
 
+
 class ReviewBoard(object):
   def __init__(self, host, user, password):
     self._base_url = 'https://%s' % host if not host.startswith('http') else host
@@ -51,68 +52,153 @@ class ReviewBoard(object):
     return json.loads(self.get_resource_data(href, args=args, data=data))
 
 
-# Thrown when the patch from a review diff could not be applied.
-class PatchApplyError(Exception):
-  pass
+class RequestProcessor(object):
+  def __init__(self, server, command, git_clean_excludes=None, tail_lines=20):
+    self._server = server
+    self._command = command
+    self._git_clean_excludes = git_clean_excludes or []
+    self._tail_lines = tail_lines
 
+  class PatchApplyError(Exception):
+    """Indicates the patch from a review diff could not be applied."""
 
-def _apply_patch(patch_data, clean_excludes):
-  subprocess.check_call(['git', 'clean', '-fdx'] + ['--exclude=%s' % e for e in clean_excludes])
-  subprocess.check_call(['git', 'reset', '--hard', 'origin/master'])
+  def _apply_patch(self, patch_data, basis=None, reset=True):
+    if reset:
+      subprocess.check_call(
+          ['git', 'clean', '-fdx'] + ['--exclude=%s' % e for e in self._git_clean_excludes])
+      subprocess.check_call(['git', 'reset', '--hard', 'origin/master'])
 
-  patch_file = 'diff.patch'
-  with open(patch_file, 'w') as f:
-    f.write(patch_data)
-  try:
-    subprocess.check_call(['git', 'apply', patch_file])
-  except subprocess.CalledProcessError:
-    raise PatchApplyError()
+    sha = subprocess.check_output(['git', 'rev-parse', '--short', 'HEAD']).strip()
+    patch_file = 'diff.patch'
+    with open(patch_file, 'w') as f:
+      f.write(patch_data)
+    try:
+      subprocess.check_call(['git', 'apply', '-3', patch_file])
+      return sha
+    except subprocess.CalledProcessError:
+      raise self.PatchApplyError(
+          'This patch does not apply cleanly against %s (%s), do you need to rebase?'
+          % (basis or 'master', sha))
 
+  def _get_latest_diff_time(self, review_request):
+    diffs = self._server.get_resource(review_request['links']['diffs']['href'])['diffs']
+    return diffs[-1]['timestamp']
 
-def _get_latest_diff_time(server, request):
-  diffs = server.get_resource(request['links']['diffs']['href'])['diffs']
-  return diffs[-1]['timestamp']
+  REPLY_REQUEST = '@ReviewBot retry'
 
+  def _get_latest_user_request(self, reviews):
+    reply_requests = [r for r in reviews if self.REPLY_REQUEST.lower() in r['body_top'].lower()]
+    if reply_requests:
+      return reply_requests[-1]['timestamp']
 
-REPLY_REQUEST = '@ReviewBot retry'
+  def _needs_reply(self, review_request):
+    print('Inspecting review %d: %s' % (review_request['id'], review_request['summary']))
+    reviews_response = self._server.get_resource(review_request['links']['reviews']['href'])
+    reviews = reviews_response['reviews']
+    # The default response limit is 25.  When the responses are limited, a 'next' link will
be
+    # included.  When that happens, continue to walk until there are no reviews left.
+    while 'next' in reviews_response['links']:
+      print('Fetching next page of reviews.')
+      reviews_response = self._server.get_resource(reviews_response['links']['next']['href'])
+      reviews.extend(reviews_response['reviews'])
+    feedback_reviews = [r for r in reviews if r['links']['user']['title'] == self._server.user]
+    if feedback_reviews:
+      # Determine whether another round of feedback is necessary.
+      latest_feedback_time = feedback_reviews[-1]['timestamp']
+      latest_request = self._get_latest_user_request(reviews)
+      latest_diff = self._get_latest_diff_time(review_request)
+      print('Latest feedback was given at        %s' % latest_feedback_time)
+      print('Latest build request from a user at %s' % latest_request)
+      print('Latest diff was posted at           %s' % latest_diff)
+      return ((latest_request and (latest_request > latest_feedback_time)) or
+              (latest_diff and (latest_diff > latest_feedback_time)))
+    return True
 
+  def _missing_tests(self, diff):
+    # Get files that were modified by the change, flag if test coverage appears lacking.
+    diff_files = self._server.get_resource(diff['links']['files']['href'])['files']
+    paths = [f['source_file'] for f in diff_files]
+    return (filter(lambda p: p.startswith('src/main/'), paths) and not
+            filter(lambda p: p.startswith('src/test/'), paths))
 
-def _get_latest_user_request(reviews):
-  reply_requests = [r for r in reviews if REPLY_REQUEST.lower() in r['body_top'].lower()]
-  if reply_requests:
-    return reply_requests[-1]['timestamp']
+  def _patch(self, review_request, basis=None, reset=True):
+    diffs = self._server.get_resource(review_request['links']['diffs']['href'])['diffs']
+    if not diffs:
+      return None
 
+    dependencies = review_request['depends_on']
+    if len(dependencies) > 1:
+      raise self.PatchApplyError(
+          'Can only test reviews with 1 parent, found %d: %s' %
+          (len(dependencies), ' '.join('RB#%d' % d['id'] for d in dependencies)))
+    elif len(dependencies) == 1:
+      parent_request = self._server.get_resource(dependencies[0]['href'])['review_request']
+      basis = 'RB#%d' % parent_request['id']
+      parent_status = parent_request['status']
+      if parent_status == 'submitted':
+        print('Skipping application of %s diffs - dependency already submitted to master.'
% basis)
+      elif parent_status == 'pending':
+        self._patch(parent_request, basis=basis, reset=reset)
+        print('Applied patch for %s dependency.' % basis)
+        reset = False
+      elif parent_status == 'discarded':
+        raise self.PatchApplyError(
+            "RB#%d depends on discarded %s. Please adjust the 'depends-on' field and rebase
"
+            "as-needed." % (review_request['id'], basis))
+      else:
+        print('WARNING: unexpected RB status %s for %s' % (parent_status, basis))
 
-def _needs_reply(server, request):
-  print('Inspecting review %d: %s' % (request['id'], request['summary']))
-  reviews_response = server.get_resource(request['links']['reviews']['href'])
-  reviews = reviews_response['reviews']
-  # The default response limit is 25.  When the responses are limited, a 'next' link will
be
-  # included.  When that happens, continue to walk until there are no reviews left.
-  while 'next' in reviews_response['links']:
-    print('Fetching next page of reviews.')
-    reviews_response = server.get_resource(reviews_response['links']['next']['href'])
-    reviews.extend(reviews_response['reviews'])
-  feedback_reviews = [r for r in reviews if r['links']['user']['title'] == server.user]
-  if feedback_reviews:
-    # Determine whether another round of feedback is necessary.
-    latest_feedback_time = feedback_reviews[-1]['timestamp']
-    latest_request = _get_latest_user_request(reviews)
-    latest_diff = _get_latest_diff_time(server, request)
-    print('Latest feedback was given at        %s' % latest_feedback_time)
-    print('Latest build request from a user at %s' % latest_request)
-    print('Latest diff was posted at           %s' % latest_diff)
-    return ((latest_request and (latest_request > latest_feedback_time))
-            or (latest_diff and (latest_diff > latest_feedback_time)))
-  return True
+    latest_diff = diffs[-1]
+    print('Applying diff %d' % latest_diff['id'])
+    patch_data = self._server.get_resource_data(
+        latest_diff['links']['self']['href'],
+        accept='text/x-patch')
+    sha = self._apply_patch(patch_data, basis=basis, reset=reset)
+    return sha, latest_diff
+
+  def process(self, review_request):
+    if not self._needs_reply(review_request):
+      return
+
+    ship = False
+    try:
+      result = self._patch(review_request)
+      if not result:
+        return
+      sha, latest_diff = result
 
+      print('Running build command.')
+      build_output = 'build_output'
+      # Pipe to a file in case output is large, also tee the output to simplify
+      # debugging.  Since we pipe the output, we must set pipefail to ensure
+      # a failing build command fails the bash pipeline.
+      result = subprocess.call([
+        'bash',
+        '-c',
+        'set -o pipefail; %s 2>&1 | tee %s' % (self._command, build_output)])
+      if result == 0:
+        review_text = 'Master (%s) is green with this patch.\n  %s' % (sha, self._command)
+        if self._missing_tests(latest_diff):
+          review_text = '%s\n\nHowever, it appears that it might lack test coverage.' % review_text
+        else:
+          ship = True
+      else:
+        build_tail = subprocess.check_output(['tail', '-n', str(self._tail_lines), build_output])
+        review_text = (
+          'Master (%s) is red with this patch.\n  %s\n\n%s' % (sha, self._command, build_tail))
+    except self.PatchApplyError as e:
+      review_text = str(e)
 
-def _missing_tests(server, diff):
-  # Get files that were modified by the change, flag if test coverage appears lacking.
-  diff_files = server.get_resource(diff['links']['files']['href'])['files']
-  paths = [f['source_file'] for f in diff_files]
-  return (filter(lambda f: f.startswith('src/main/'), paths) and not
-          filter(lambda f: f.startswith('src/test/'), paths))
+    review_text = ('%s\n\nI will refresh this build result if you post a review containing
"%s"'
+                   % (review_text, self.REPLY_REQUEST))
+    print('Replying to review %d:\n%s' % (review_request['id'], review_text))
+    print(self._server.get_resource(
+        review_request['links']['reviews']['href'],
+        data=urllib.urlencode({
+          'body_top': review_text,
+          'public': 'true',
+          'ship_it': 'true' if ship else 'false'
+        })))
 
 
 def main():
@@ -137,10 +223,10 @@ def main():
       default=20,
       help='Number of lines of command output to include in red build reviews.')
   parser.add_argument(
-    '--git-clean-exclude',
-    help='Patterns to pass to git-clean --exclude.',
-    nargs='*',
-    default=[])
+      '--git-clean-exclude',
+      help='Patterns to pass to git-clean --exclude.',
+      nargs='*',
+      default=[])
   args = parser.parse_args()
 
   credentials = args.reviewboard_credentials_file.readlines()
@@ -164,58 +250,16 @@ def main():
     'status': 'pending'
   })['review_requests']
   print('Found %d review requests to inspect' % len(requests))
-  for request in requests:
-    if not _needs_reply(server, request):
-      continue
-
-    diffs = server.get_resource(request['links']['diffs']['href'])['diffs']
-    if not diffs:
-      continue
 
-    latest_diff = diffs[-1]
-    print('Applying diff %d' % latest_diff['id'])
-    patch_data = server.get_resource_data(
-        latest_diff['links']['self']['href'],
-        accept='text/x-patch')
-    sha = subprocess.check_output(['git', 'rev-parse', '--short', 'HEAD']).strip()
-    ship = False
-    try:
-      _apply_patch(patch_data, args.git_clean_exclude)
-      print('Running build command.')
-      build_output = 'build_output'
-      command = args.command
-      # Pipe to a file in case output is large, also tee the output to simplify
-      # debugging.  Since we pipe the output, we must set pipefail to ensure
-      # a failing build command fails the bash pipeline.
-      result = subprocess.call([
-          'bash',
-          '-c',
-          'set -o pipefail; %s 2>&1 | tee %s' % (command, build_output)])
-      if result == 0:
-        review_text = 'Master (%s) is green with this patch.\n  %s' % (sha, command)
-        if _missing_tests(server, latest_diff):
-          review_text = '%s\n\nHowever, it appears that it might lack test coverage.' % review_text
-        else:
-          ship = True
-      else:
-        build_tail = subprocess.check_output(['tail', '-n', str(args.tail_lines), build_output])
-        review_text = (
-            'Master (%s) is red with this patch.\n  %s\n\n%s' % (sha, command, build_tail))
-    except PatchApplyError:
-      review_text = (
-          'This patch does not apply cleanly on master (%s), do you need to rebase?' % sha)
+  request_processor = RequestProcessor(
+      server,
+      args.command,
+      args.git_clean_exclude,
+      args.tail_lines)
 
-    review_text = ('%s\n\nI will refresh this build result if you post a review containing
"%s"'
-                   % (review_text, REPLY_REQUEST))
-    print('Replying to review %d:\n%s' % (request['id'], review_text))
-    print(server.get_resource(
-        request['links']['reviews']['href'],
-        data=urllib.urlencode({
-            'body_top': review_text,
-            'public': 'true',
-            'ship_it': 'true' if ship else 'false'
-        })))
+  for review_request in requests:
+    request_processor.process(review_request)
 
 
-if __name__=="__main__":
+if __name__ == "__main__":
   main()


Mime
View raw message