From 411e63cdca2b5f5620309ca4eaddfb274f022c4d Mon Sep 17 00:00:00 2001 From: Jason Kuster Date: Mon, 10 Jul 2017 11:11:08 -0700 Subject: [PATCH 1/4] Modify mergebot to use commit statuses as well. Signed-off-by: Jason Kuster --- mergebot_backend/github_helper.py | 54 +++++++++++++-- mergebot_backend/merge.py | 100 +++++++++++++++++++++++++--- mergebot_backend/mergebot_poller.py | 9 ++- 3 files changed, 147 insertions(+), 16 deletions(-) diff --git a/mergebot_backend/github_helper.py b/mergebot_backend/github_helper.py index e5c8bd4..c654e58 100644 --- a/mergebot_backend/github_helper.py +++ b/mergebot_backend/github_helper.py @@ -13,6 +13,20 @@ BOT_NAME = 'asfgit' GITHUB_SECRET = 'mergebot.secret' +COMMENT_PAYLOAD = '{{"body": "{body}"}}' +COMMIT_STATUS_PAYLOAD = """ +{{ + "state": "{state}", + "target_url": "{url}", + "description": "{description}", + "context": "{context}" +}} +""" +COMMIT_STATE_PENDING = 'pending' +COMMIT_STATE_SUCCESS = 'success' +COMMIT_STATE_ERROR = 'error' +COMMIT_STATE_FAILURE = 'failure' + class GithubHelper(object): """GithubHelper is a lightweight authenticated wrapper for the Github API. @@ -60,17 +74,16 @@ def get(self, endpoint): return resp.json() resp.raise_for_status() - def post(self, content, endpoint): + def post(self, payload, endpoint): """post makes a POST request against a specified endpoint. Args: - content: data to send in POST body. + payload: data to send in POST body. endpoint: URL to which to make the POST request. URL is relative to https://api.github.com/repos/{self.org}/{self.repo}/ Returns: None if request returned successfully, error otherwise. """ - payload = '{{"body": "{}"}}'.format(content) url = urlparse.urljoin(self.repo_url, endpoint) try: resp = requests.post(url, data=payload, auth=(BOT_NAME, @@ -97,6 +110,7 @@ def __init__(self, helper, pr_object): self.helper = helper self.pr_num = pr_object['number'] self.head_sha = pr_object['head']['sha'] + self.statuses_url = pr_object['statuses_url'] self.comments_url = pr_object['comments_url'] self.updated = dateutil.parser.parse(pr_object['updated_at']) self.metadata = {} @@ -155,6 +169,37 @@ def post_info(self, content, logger): """ self.post_pr_comment('Info: {}'.format(content), logger) + def post_commit_status(self, state, url, description, context, logger): + """ Posts a commit status update to this PR. + + Args: + state: State for status; can be 'pending', 'success', 'error', or + 'failure'. + url: URL at which more information can be found. + description: Description to show on commit status. + context: Unique name for this particular status line. + logger: Logger to use for reporting failure. + """ + payload = COMMIT_STATUS_PAYLOAD.format( + state=state, + url=url, + description=description, + context=context + ) + err = self.helper.post(payload, self.statuses_url) + if err: + logger.error( + "Couldn't set commit status for pr {pr_num}: {err}. Context: " + "{context}, State: {state}, Description: {description}, URL: " + "{url}. Falling back to posting a comment.".format( + pr_num=self.pr_num, err=err, context=context, state=state, + description=description, url=url)) + self.post_pr_comment( + '{context}: PR {pr_num} in state {state} with description ' + '{description}. URL: {url}.'.format( + context=context, pr_num=self.pr_num, state=state, + description=description, url=url), logger) + def post_pr_comment(self, content, logger): """Posts a PR comment to Github. @@ -162,7 +207,8 @@ def post_pr_comment(self, content, logger): content: the content to post. logger: logger to send error to if post fails. """ - err = self.helper.post(content, self.comments_url) + payload = COMMENT_PAYLOAD.format(body=content) + err = self.helper.post(payload, self.comments_url) if err is not None: logger.error( "Couldn't post comment '{cmt}' to Github: {err}.".format( diff --git a/mergebot_backend/merge.py b/mergebot_backend/merge.py index 2265a9d..1241ac0 100644 --- a/mergebot_backend/merge.py +++ b/mergebot_backend/merge.py @@ -16,6 +16,7 @@ from threading import Thread from mergebot_backend import db_publisher +from mergebot_backend import github_helper from mergebot_backend.log_helper import get_logger TMP_DIR_FMT = '/tmp/{dir}' @@ -24,6 +25,7 @@ JENKINS_STARTED_MSG = ('Job verification started. Verification job is [here]' '({build_url}) (may still be pending; if page 404s, ' 'check job status page [here]({job_url})).') +MERGEBOT_ITEM_URL = 'http://mergebot-vm.apache.org/{name}/{number}' class Terminate(Exception): @@ -190,6 +192,9 @@ def run(self): return time.sleep(1) + if not isinstance(pr, github_helper.GithubPR): + raise AttributeError + pr_num = pr.get_num() self.merge_logger.info( 'Starting work on PR#{pr_num}.'.format(pr_num=pr_num)) @@ -200,12 +205,26 @@ def run(self): info="Requested by {user} at {time}.".format( user=pr.metadata['asf_id'], time=pr.metadata['created'])) try: - pr.post_info( - 'MergeBot starting work on PR#{pr_num}.'.format( - pr_num=pr_num), - self.merge_logger) + pr.post_commit_status( + state=github_helper.COMMIT_STATE_PENDING, + url=MERGEBOT_ITEM_URL.format( + name=self.config.name, number=pr_num), + description='Started Work', + context='MergeBot: Merge', + logger=self.merge_logger) self.merge_git_pr(pr) except BaseException as exc: + pr.post_commit_status( + state=github_helper.COMMIT_STATE_ERROR, + url=MERGEBOT_ITEM_URL.format( + name=self.config.name, number=pr_num), + description='Unexpected Error: {exc}.'.format(exc=exc), + context='MergeBot: Merge', + logger=self.merge_logger) + # In the case of failure, we still post back to the pull request + # because otherwise if mergebot is stopped and restarted we will + # try this pull request again. If mergebot is modified to work + # with webhooks this can be removed. pr.post_error( 'MergeBot encountered an unexpected error while processing ' 'this PR: {exc}.'.format(exc=exc), self.merge_logger) @@ -260,6 +279,17 @@ def flush_queue(self): """ pr = self.work_queue.get_nowait() self.publisher.publish_dequeue(item_id=pr.get_num()) + pr.post_commit_status( + state=github_helper.COMMIT_STATE_ERROR, + url=MERGEBOT_ITEM_URL.format( + name=self.config.name, number=pr.get_num()), + description='MergeBot shutdown; resubmit when mergebot is back up.', + context='MergeBot: Merge', + logger=self.merge_logger) + # In the case of shutdown, we still post back to the pull request + # because otherwise if mergebot is stopped and restarted we will try + # this pull request again. If mergebot is modified to work with webhooks + # this can be removed. pr.post_info('MergeBot shutting down; please resubmit when MergeBot is ' 'back up.', self.merge_logger) @@ -289,6 +319,17 @@ def merge_git_pr(self, pr): _set_up(tmp_dir) self.execute_merge_lifecycle(pr, tmp_dir, pr_logger) except AssertionError as exc: + pr.post_commit_status( + state=github_helper.COMMIT_STATE_FAILURE, + url=MERGEBOT_ITEM_URL.format( + name=self.config.name, number=pr.get_num()), + description=exc.message, + context='MergeBot: Merge', + logger=self.merge_logger) + # In the case of failure, we still post back to the pull request + # because otherwise if mergebot is stopped and restarted we will + # try this pull request again. If mergebot is modified to work with + # webhooks this can be removed. pr.post_error(exc, pr_logger) self.publisher.publish_item_status( item_id=pr_num, status='ERROR', @@ -340,8 +381,19 @@ def execute_merge_lifecycle(self, pr, tmp_dir, pr_logger): self.publisher.publish_item_status(item_id=pr_num, status='PREPARE') run_cmds(self.PREPARE_CMDS, pr_vars, tmp_dir, pr_logger) self.publisher.publish_item_status(item_id=pr_num, status='MERGE') + # Success commit status needs to be posted before actual success because + # we lose the ability to update a commit status after the PR is closed. + # If FINAL_CMDS are not successful, we'll update the commit status with + # an error as part of the exception handling process, so the user won't + # see success turn into failure. + pr.post_commit_status( + state=github_helper.COMMIT_STATE_SUCCESS, + url=MERGEBOT_ITEM_URL.format( + name=self.config.name, number=pr_num), + description='Merge Succeeded!', + context='MergeBot: Merge', + logger=self.merge_logger) run_cmds(self.FINAL_CMDS, pr_vars, tmp_dir, pr_logger) - pr.post_info('PR merge succeeded!', pr_logger) pr_logger.info( 'Merge for {pr_num} completed successfully.'.format(pr_num=pr_num)) @@ -363,6 +415,15 @@ def verify_pr_via_jenkins(self, job, build_num, pr, pr_logger): pr_num = pr.get_num() self.publisher.publish_item_status(item_id=pr_num, status='WAIT_ON_JOB_START') + job_url = urlparse.urljoin( + self.config.jenkins_location, '/job/{job_name}/'.format( + job_name=self.config.verification_job_name)) + pr.post_commit_status( + state=github_helper.COMMIT_STATE_PENDING, + url=job_url, + description='Starting verification job.', + context='MergeBot: Verification Job', + logger=self.merge_logger) while not build and wait_secs < self.JOB_START_TIMEOUT: try: build = job.get_build(build_num) @@ -372,18 +433,25 @@ def verify_pr_via_jenkins(self, job, build_num, pr, pr_logger): time.sleep(self.WAIT_INTERVAL) wait_secs += self.WAIT_INTERVAL - job_url = urlparse.urljoin( - self.config.jenkins_location, '/job/{job_name}/'.format( - job_name=self.config.verification_job_name)) if not build: pr_logger.error('Timed out trying to find the verification job.') + pr.post_commit_status( + state=github_helper.COMMIT_STATE_ERROR, + url=job_url, + description='Timed out waiting for job start.', + context='MergeBot: Verification Job', + logger=self.merge_logger) raise AssertionError(JENKINS_TIMEOUT_ERR.format(url=job_url)) pr_logger.info("Build #{build_num} found.".format(build_num=build_num)) build_url = urlparse.urljoin(job_url, str(build_num)) - pr.post_info(JENKINS_STARTED_MSG.format(build_url=build_url, - job_url=job_url), pr_logger) + pr.post_commit_status( + state=github_helper.COMMIT_STATE_PENDING, + url=build_url, + description='Verification job running.', + context='MergeBot: Verification Job', + logger=self.merge_logger) self.publisher.publish_item_status( item_id=pr_num, status='JOB_FOUND', info='Build URL: {build_url}'.format(build_url=build_url)) @@ -403,7 +471,19 @@ def verify_pr_via_jenkins(self, job, build_num, pr, pr_logger): # find job status anywhere on the object. build = job.get_build(build_num) if build.get_status() == "SUCCESS": + pr.post_commit_status( + state=github_helper.COMMIT_STATE_SUCCESS, + url=build_url, + description='Verification job succeeded!', + context='MergeBot: Verification Job', + logger=self.merge_logger) return True + pr.post_commit_status( + state=github_helper.COMMIT_STATE_FAILURE, + url=build_url, + description='Verification job failed.', + context='MergeBot: Verification Job', + logger=self.merge_logger) return False diff --git a/mergebot_backend/mergebot_poller.py b/mergebot_backend/mergebot_poller.py index 1a02400..0275051 100644 --- a/mergebot_backend/mergebot_poller.py +++ b/mergebot_backend/mergebot_poller.py @@ -15,6 +15,7 @@ from mergebot_backend.log_helper import get_logger BOT_NAME = 'asfgit' +MERGEBOT_PROJ_URL = 'http://mergebot-vm.apache.org/{name}' def create_poller(config, comm_pipe): @@ -213,8 +214,12 @@ def merge_git(self, pull): contract with search_github_pr since other commands could fail. """ self.l.info('Command was merge, adding to merge queue.') - pull.post_info('Adding PR to work queue; current position: ' - '{pos}.'.format(pos=self.work_queue.qsize() + 1), self.l) + pull.post_commit_status( + state=github_helper.COMMIT_STATE_PENDING, + url=MERGEBOT_PROJ_URL.format(name=self.config.name), + description='In Queue', + context='MergeBot: Merge', + logger=self.l) self.publisher.publish_enqueue(item_id=pull.get_num()) self.work_queue.put(pull) return True From 3b527ce6a7a71cfa0b01855a9dcb9d65454f1370 Mon Sep 17 00:00:00 2001 From: Jason Kuster Date: Mon, 10 Jul 2017 11:27:58 -0700 Subject: [PATCH 2/4] Fix unit tests after commit status changes. Signed-off-by: Jason Kuster --- mergebot_backend/tests/merge_test.py | 9 +++--- .../tests/mergebot_poller_test.py | 28 ++++++++++--------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/mergebot_backend/tests/merge_test.py b/mergebot_backend/tests/merge_test.py index 22fa94a..aaac614 100644 --- a/mergebot_backend/tests/merge_test.py +++ b/mergebot_backend/tests/merge_test.py @@ -14,9 +14,10 @@ class MergeTest(unittest.TestCase): def setUp(self): self.config = MergeBotConfig( - name='test', github_org='test', repository='test', + name='test', proj_name='test', github_org='test', repository='test', merge_branch='test', verification_branch='test', scm_type='github', - jenkins_location='http://test.test', verification_job_name='test') + jenkins_location='http://test.test', prepare_command='test', + verification_job_name='test') @patch('mergebot_backend.db_publisher.DBPublisher') @patch('mergebot_backend.merge.get_logger') @@ -135,8 +136,8 @@ def test_execute_merge_lifecycle_validation_success( mock_jenkins.return_value = {'test': mock_job} mock_verify.return_value = True gm.execute_merge_lifecycle(mock_pr, '/tmp', mock_logger) - self.assertEqual(gm.publisher.publish_item_status.call_count, 3) - self.assertEqual(mock_run_cmds.call_count, 3) + self.assertEqual(gm.publisher.publish_item_status.call_count, 4) + self.assertEqual(mock_run_cmds.call_count, 4) @patch('logging.Logger') @patch('mergebot_backend.github_helper.GithubPR') diff --git a/mergebot_backend/tests/mergebot_poller_test.py b/mergebot_backend/tests/mergebot_poller_test.py index 4edf472..6fb35d5 100644 --- a/mergebot_backend/tests/mergebot_poller_test.py +++ b/mergebot_backend/tests/mergebot_poller_test.py @@ -12,9 +12,10 @@ class GithubPollerTest(unittest.TestCase): def setUp(self): self.config = MergeBotConfig( - name='test', github_org='test', repository='test', + name='test', proj_name='test', github_org='test', repository='test', merge_branch='test', verification_branch='test', scm_type='github', - jenkins_location='http://test.test', verification_job_name='test') + jenkins_location='http://test.test', prepare_command='test', + verification_job_name='test') @patch('mergebot_backend.db_publisher.DBPublisher') @patch('mergebot_backend.github_helper.GithubHelper') @@ -29,7 +30,7 @@ def create_poller_for_testing(self, mock_logger, mock_queue, mock_pipe, mock_publisher): pipe = MagicMock() mock_pipe.return_value = (MagicMock(), MagicMock()) - mock_authorized.return_value = ['authorized'] + mock_authorized.return_value = {'authorized_gh': 'authorized_asf'} return mergebot_poller.GithubPoller(config=self.config, comm_pipe=pipe) @patch('mergebot_backend.mergebot_poller.GithubPoller') @@ -142,15 +143,16 @@ def testSearchUnauthorizedUser(self, mock_merge_git, mock_comment, mock_pr): # Tests that a command by an unauthorized user is caught successfully. ghp = self.create_poller_for_testing() ghp.merge_git = mock_merge_git - mock_comment.get_body.return_value = '@apache-merge-bot command' + mock_comment.get_body.return_value = '@asfgit command' mock_comment.get_user.return_value = 'unauthorized' mock_pr.fetch_comments.return_value = [mock_comment] mock_pr.post_error.return_value = True search = ghp.search_github_pr(mock_pr) self.assertTrue(search) - mock_pr.post_error.assert_called_with(content="User unauthorized not a " - "committer; access denied.", - logger=ghp.l) + mock_pr.post_error.assert_called_with( + content="User unauthorized not a committer on test or not " + "registered in gitbox.apache.org; access denied.", + logger=ghp.l) self.assertEqual(mock_merge_git.call_count, 0) @patch('mergebot_backend.github_helper.GithubPR') @@ -160,9 +162,9 @@ def testSearchInvalidCommand(self, mock_merge_git, mock_comment, mock_pr): # Tests that an invalid comment on a valid PR is caught successfully. ghp = self.create_poller_for_testing() ghp.merge_git = mock_merge_git - mock_comment.get_body.return_value = '@apache-merge-bot command' - mock_comment.get_user.return_value = 'authorized' - mergebot_poller.AUTHORIZED_USERS = ['authorized'] + mock_comment.get_body.return_value = '@asfgit command' + mock_comment.get_user.return_value = 'authorized_gh' + ghp.authorized_users = {'authorized_gh': 'authorized_asf'} mock_pr.fetch_comments.return_value = [mock_comment] mock_pr.post_error.return_value = None search = ghp.search_github_pr(mock_pr) @@ -180,9 +182,9 @@ def testSearchValidCommand(self, mock_merge_git, mock_comment, mock_pr): # Tests that valid commands are successfully piped through. ghp = self.create_poller_for_testing() ghp.COMMANDS['merge'] = mock_merge_git - mock_comment.get_body.return_value = '@apache-merge-bot merge' - mock_comment.get_user.return_value = 'authorized' - ghp.authorized_users = ['authorized'] + mock_comment.get_body.return_value = '@asfgit merge' + mock_comment.get_user.return_value = 'authorized_gh' + ghp.authorized_users = {'authorized_gh': 'authorized_asf'} mock_pr.fetch_comments.return_value = [mock_comment] search = ghp.search_github_pr(mock_pr) self.assertTrue(search) From d0e9a3cb81b518345647e94fc1d28f8e184df74a Mon Sep 17 00:00:00 2001 From: Jason Kuster Date: Tue, 11 Jul 2017 15:34:32 -0700 Subject: [PATCH 3/4] Address PR comments. Signed-off-by: Jason Kuster --- mergebot_backend/github_helper.py | 4 ++-- mergebot_backend/merge.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mergebot_backend/github_helper.py b/mergebot_backend/github_helper.py index c654e58..35ddf74 100644 --- a/mergebot_backend/github_helper.py +++ b/mergebot_backend/github_helper.py @@ -24,8 +24,8 @@ """ COMMIT_STATE_PENDING = 'pending' COMMIT_STATE_SUCCESS = 'success' -COMMIT_STATE_ERROR = 'error' -COMMIT_STATE_FAILURE = 'failure' +COMMIT_STATE_ERROR = 'error' # error represents a problem with MergeBot. +COMMIT_STATE_FAILURE = 'failure' # failure represents a problem with the PR. class GithubHelper(object): diff --git a/mergebot_backend/merge.py b/mergebot_backend/merge.py index 1241ac0..7e9d68a 100644 --- a/mergebot_backend/merge.py +++ b/mergebot_backend/merge.py @@ -25,6 +25,7 @@ JENKINS_STARTED_MSG = ('Job verification started. Verification job is [here]' '({build_url}) (may still be pending; if page 404s, ' 'check job status page [here]({job_url})).') +MERGEBOT_MAIN_URL = 'http://mergebot-vm.apache.org/' MERGEBOT_ITEM_URL = 'http://mergebot-vm.apache.org/{name}/{number}' @@ -241,7 +242,7 @@ def run(self): def fetch_from_queue(self): """fetch_from_queue tries to pull a PR off of the queue. - + Raises: merge.Terminate if we receive the termination signal. Returns: @@ -273,7 +274,7 @@ def fetch_from_queue(self): def flush_queue(self): """flush_queue posts a shutdown message to a PR in the queue. - + Raises: Queue.Empty if there is nothing in the queue. """ @@ -281,8 +282,7 @@ def flush_queue(self): self.publisher.publish_dequeue(item_id=pr.get_num()) pr.post_commit_status( state=github_helper.COMMIT_STATE_ERROR, - url=MERGEBOT_ITEM_URL.format( - name=self.config.name, number=pr.get_num()), + url=MERGEBOT_MAIN_URL, description='MergeBot shutdown; resubmit when mergebot is back up.', context='MergeBot: Merge', logger=self.merge_logger) From cec4a158a22a3abe1d0572c97260508afa41a240 Mon Sep 17 00:00:00 2001 From: jasonkuster Date: Wed, 19 Jul 2017 09:44:36 -0700 Subject: [PATCH 4/4] Bump job start timeout to 10m --- mergebot_backend/merge.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mergebot_backend/merge.py b/mergebot_backend/merge.py index 7e9d68a..c90291c 100644 --- a/mergebot_backend/merge.py +++ b/mergebot_backend/merge.py @@ -145,7 +145,7 @@ class GitMerger(Merger): APACHE_GIT = 'https://gitbox.apache.org/repos/asf/{repo}.git' GITHUB_REPO_URL = 'https://github.com/{org}/{repo}.git' - JOB_START_TIMEOUT = 300 + JOB_START_TIMEOUT = 600 WAIT_INTERVAL = 10 def __init__(self, config, work_queue, pipe):