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.sir...@gmail.com> Authored: Thu Dec 24 10:25:51 2015 -0600 Committer: Joshua Cohen <jco...@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()
