Changes to allow MergeBot to post to the Commit Status API #19

jasonkuster · 2017-07-10T18:29:51Z

No description provided.

Signed-off-by: Jason Kuster <[email protected]>

markflyhigh · 2017-07-11T20:43:34Z

mergebot_backend/github_helper.py

+COMMIT_STATE_PENDING = 'pending'
+COMMIT_STATE_SUCCESS = 'success'
+COMMIT_STATE_ERROR = 'error'
+COMMIT_STATE_FAILURE = 'failure'


what's difference between error and failure? Probably some documents here or other place?

Error is a problem that happened on MergeBot's side -- something that should be retried. Failure indicates something went wrong that wasn't MergeBot's fault -- bad rebase, tests failed, merge conflict, etc.

Added a comment here to clarify.

markflyhigh · 2017-07-11T21:01:19Z

mergebot_backend/github_helper.py

@@ -155,14 +169,46 @@ 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.


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? since commit makes me think of a specific commit in a PR.

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.

markflyhigh · 2017-07-11T21:35:59Z

mergebot_backend/merge.py

@@ -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,


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?

looks like the description tell the difference.

AssertionErrors indicate something has gone preventably wrong in the PR (as mentioned in the above comment) and thus FAILURE is the appropriate level.

markflyhigh · 2017-07-11T21:45:09Z

mergebot_backend/merge.py

+            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.',


can we provide a url for health check? or some instructions? If I'm new to mergebot and got this message, I'll have no idea when mergebot is back up.

I'll update the URL to the top-level MergeBot page which indicates whether MergeBot is back up, but all of the URLs on these point to the MergeBot frontend.

Updated URL.

markflyhigh · 2017-07-11T21:46:23Z

mergebot_backend/merge.py

+        # 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 '


do we need to post similar content comparing to line 286? I guess it's another comment just right next to the above one?

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.

I previously thought the status commit is the place where people commenting on a PR. I think it's good now 👍

markflyhigh · 2017-07-11T22:00:08Z

mergebot_backend/merge.py

+                    state=github_helper.COMMIT_STATE_PENDING,
+                    url=MERGEBOT_ITEM_URL.format(
+                        name=self.config.name, number=pr_num),
+                    description='Started Work',


start merging or start running jenkins build?

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.

That's clear enough. I have better understanding after saw the real case :)

Signed-off-by: Jason Kuster <[email protected]>

markflyhigh

Thank you. Just one more question.

markflyhigh · 2017-07-11T22:52:28Z

mergebot_backend/github_helper.py

@@ -155,14 +169,46 @@ 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.


markflyhigh · 2017-07-11T22:55:36Z

mergebot_backend/merge.py

+                    state=github_helper.COMMIT_STATE_PENDING,
+                    url=MERGEBOT_ITEM_URL.format(
+                        name=self.config.name, number=pr_num),
+                    description='Started Work',


That's clear enough. I have better understanding after saw the real case :)

markflyhigh · 2017-07-11T23:00:54Z

mergebot_backend/merge.py

+        # 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 '


I previously thought the status commit is the place where people commenting on a PR. I think it's good now 👍

markflyhigh · 2017-07-11T23:01:42Z

mergebot_backend/merge.py

@@ -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,


markflyhigh · 2017-07-11T23:06:51Z

mergebot_backend/merge.py

+            pr.post_commit_status(
+                state=github_helper.COMMIT_STATE_ERROR,
+                url=job_url,
+                description='Timed out waiting for job start.',


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?

I believe this will still find queued jobs, but I'll investigate that and get back to you.

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 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.

markflyhigh · 2017-07-19T17:11:04Z

LGTM

jasonkuster added 2 commits July 10, 2017 11:11

Modify mergebot to use commit statuses as well.

411e63c

Signed-off-by: Jason Kuster <[email protected]>

Fix unit tests after commit status changes.

3b527ce

Signed-off-by: Jason Kuster <[email protected]>

markflyhigh reviewed Jul 11, 2017

View reviewed changes

Address PR comments.

d0e9a3c

Signed-off-by: Jason Kuster <[email protected]>

markflyhigh reviewed Jul 11, 2017

View reviewed changes

Bump job start timeout to 10m

cec4a15

jasonkuster merged commit 3244a16 into master Jul 19, 2017

jasonkuster deleted the commitstatus branch July 19, 2017 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to allow MergeBot to post to the Commit Status API #19

Changes to allow MergeBot to post to the Commit Status API #19

jasonkuster commented Jul 10, 2017

markflyhigh Jul 11, 2017

jasonkuster Jul 11, 2017

jasonkuster Jul 11, 2017

markflyhigh Jul 11, 2017 •

edited

Loading

jasonkuster Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

jasonkuster Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

jasonkuster Jul 11, 2017

jasonkuster Jul 11, 2017

markflyhigh Jul 11, 2017

jasonkuster Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

jasonkuster Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh left a comment •

edited

Loading

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

markflyhigh Jul 11, 2017

jasonkuster Jul 13, 2017

jasonkuster Jul 19, 2017

markflyhigh Jul 19, 2017

markflyhigh commented Jul 19, 2017

Changes to allow MergeBot to post to the Commit Status API #19

Changes to allow MergeBot to post to the Commit Status API #19

Conversation

jasonkuster commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markflyhigh Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markflyhigh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markflyhigh commented Jul 19, 2017

markflyhigh Jul 11, 2017 •

edited

Loading

markflyhigh left a comment •

edited

Loading