Changes to allow MergeBot to post to the Commit Status API #19
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,8 @@ | |
| 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}' | ||
|
|
||
|
|
||
| class Terminate(Exception): | ||
|
|
@@ -142,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 = 600 | ||
| WAIT_INTERVAL = 10 | ||
|
|
||
| def __init__(self, config, work_queue, pipe): | ||
|
|
@@ -190,6 +193,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 +206,26 @@ def run(self): | |
| info="Requested by {user} at {time}.".format( | ||
| user=pr.metadata['asf_id'], time=pr.metadata['created'])) | ||
| try: | ||
| 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', | ||
|
There was a problem hiding this comment. start merging or start running jenkins build? There was a problem hiding this comment. This indicates that MergeBot has begun work on this PR. I could add additional commit status updates which show when the status changes in intermediary ways, if that's something you think would be useful. There was a problem hiding this comment. That's clear enough. I have better understanding after saw the real case :) |
||
| 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) | ||
|
|
@@ -222,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: | ||
|
|
@@ -254,12 +274,22 @@ 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. | ||
| """ | ||
| 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_MAIN_URL, | ||
| 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 ' | ||
|
There was a problem hiding this comment. do we need to post similar content comparing to line 286? I guess it's another comment just right next to the above one? There was a problem hiding this comment. I'm not sure what you mean by this -- in this section we're both updating the commit status (line ~286), and posting a comment on the PR indicating the same message. There was a problem hiding this comment. I previously thought the status commit is the place where people commenting on a PR. I think it's good now 👍 |
||
| '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, | ||
|
There was a problem hiding this comment. why "failure" not "error"? Also i'm thinking if someone that first use mergebot, how can they tell difference between failure and error status? Do I miss anything that contains more instructions? There was a problem hiding this comment. looks like the description tell the difference. There was a problem hiding this comment. AssertionErrors indicate something has gone preventably wrong in the PR (as mentioned in the above comment) and thus FAILURE is the appropriate level. |
||
| 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_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 | ||
|
|
||
| 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.', | ||
|
There was a problem hiding this comment. Sometimes Jenkins doesn't have enough executors so job will be queued. Is it possible that job is queued and timed out on mergebot side? There was a problem hiding this comment. I believe this will still find queued jobs, but I'll investigate that and get back to you. There was a problem hiding this comment. It doesn't appear to find queued jobs. I've bumped the timeout to 10 minutes to compensate -- if jobs are waiting longer than 10 minutes we probably need new executors. There was a problem hiding this comment. There will be resource waste if timeout here since Jenkins will run the queued job anyway. Another issue is created and we can address it in another PR. |
||
| 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_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 | ||
|
|
||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I'm wrong, it looks like a function that posts/updates merge status to the particular PR. If that's true, how about something like
post/update_merge_status? sincecommitmakes me think of a specific commit in a PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually attached to a particular commit, and is using the "Commit Status API" from GitHub.
https://developer.github.com/v3/repos/statuses/
Statuses need to be attached to specific commits since they are what represent the HEAD of a particular pull request. GitHub doesn't have any other way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.