Merge remote-tracking branch 'origin/master' into test-commands

Fix merge conflict in homu/main.py

cfg.sample.toml

@@ -158,6 +158,10 @@ name = "Travis CI - Branch"
 #
 # String name of the Checks run.
 #name = ""
+#
+# String name of the Checks run used for try runs.
+# If the field is omitted the same name as the auto build will be used.
+#try_name = ""
 
 # Use buildbot for running tests
 #[repo.NAME.buildbot]

homu/comments.py

@@ -18,6 +18,59 @@ def jsonify(self):
         return json.dumps(out, separators=(',', ':'))
 
 
+class Approved(Comment):
+    def __init__(self, bot=None, **args):
+        # Because homu needs to leave a comment for itself to kick off a build,
+        # we need to know the correct botname to use. However, we don't want to
+        # save that botname in our state JSON. So we need a custom constructor
+        # to grab the botname and delegate the rest of the keyword args to the
+        # Comment constructor.
+        super().__init__(**args)
+        self.bot = bot
+
+    params = ["sha", "approver"]
+
+    def render(self):
+        # The comment here is required because Homu wants a full, unambiguous,
+        # pinned commit hash to kick off the build, and this note-to-self is
+        # how it gets it. This is to safeguard against situations where Homu
+        # reloads and another commit has been pushed since the approval.
+        message = ":pushpin: Commit {sha} has been " + \
+            "approved by `{approver}`\n\n" + \
+            "<!-- @{bot} r={approver} {sha} -->"
+        return message.format(
+            sha=self.sha,
+            approver=self.approver,
+            bot=self.bot
+        )
+
+
+class ApprovalIgnoredWip(Comment):
+    def __init__(self, wip_keyword=None, **args):
+        # We want to use the wip keyword in the message, but not in the json
+        # blob.
+        super().__init__(**args)
+        self.wip_keyword = wip_keyword
+
+    params = ["sha"]
+
+    def render(self):
+        message = ':clipboard:' + \
+            ' Looks like this PR is still in progress,' + \
+            ' ignoring approval.\n\n' + \
+            'Hint: Remove **{wip_keyword}** from this PR\'s title when' + \
+            ' it is ready for review.'
+        return message.format(wip_keyword=self.wip_keyword)
+
+
+class Delegated(Comment):
+    params = ["delegator", "delegate"]
+
+    def render(self):
+        message = ':v: @{} can now approve this pull request'
+        return message.format(self.delegate)
+
+
 class BuildStarted(Comment):
     params = ["head_sha", "merge_sha"]
 

homu/html/build_res.html

@@ -47,7 +47,13 @@ <h1>Homu build results - <a href="{{repo_url}}/pull/{{pull}}" target="_blank">{{
                 <tr>
                     <td class="hide">{{loop.index}}</td>
                     <td>{{builder.name}}</td>
-                    <td class="{{builder.result}}"><a href="{{builder.url}}">{{builder.result}}</a></td>
+                    <td class="{{builder.result}}">
+                        {%- if builder.url -%}
+                            <a href="{{builder.url}}">{{builder.result}}</a>
+                        {%- else -%}
+                            {{ builder.result }}
+                        {%- endif -%}
+                    </td>
                 </tr>
                 {% endfor %}
             </tbody>

homu/main.py

@@ -408,6 +408,13 @@ def record_retry_log(self, src, body):
             [self.repo_label, self.num, src, body],
         )
 
+    @property
+    def author(self):
+        """
+        Get the GitHub login name of the author of the pull request
+        """
+        return self.get_issue().user.login
+
 
 def sha_cmp(short, full):
     return len(short) >= 4 and short == full[:len(short)]
@@ -489,13 +496,10 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
             for wip_kw in ['WIP', 'TODO', '[WIP]', '[TODO]', '[DO NOT MERGE]']:
                 if state.title.upper().startswith(wip_kw):
                     if realtime:
-                        state.add_comment((
-                            ':clipboard:'
-                            ' Looks like this PR is still in progress,'
-                            ' ignoring approval.\n\n'
-                            'Hint: Remove **{}** from this PR\'s title when'
-                            ' it is ready for review.'
-                        ).format(wip_kw))
+                        state.add_comment(comments.ApprovalIgnoredWip(
+                            sha=state.head_sha,
+                            wip_keyword=wip_kw,
+                        ))
                     is_wip = True
                     break
             if is_wip:
@@ -557,14 +561,10 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
                         .format(msg, state.head_sha)
                     )
                 else:
-                    state.add_comment(
-                        ':pushpin: Commit {} has been approved by `{}`\n\n<!-- @{} r={} {} -->'  # noqa
-                        .format(
-                            state.head_sha,
-                            approver,
-                            my_username,
-                            approver,
-                            state.head_sha,
+                    state.add_comment(comments.Approved(
+                        sha=state.head_sha,
+                        approver=approver,
+                        bot=my_username,
                     ))
                     treeclosed = state.blocked_by_closed_tree()
                     if treeclosed:
@@ -575,9 +575,18 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
                     state.change_labels(LabelEvent.APPROVED)
 
         elif command.action == 'unapprove':
-            if not verify_auth(username, repo_label, repo_cfg, state,
-                               AuthState.REVIEWER, realtime, my_username):
-                continue
+            # Allow the author of a pull request to unapprove their own PR. The
+            # author can already perform other actions that effectively
+            # unapprove the PR (change the target branch, push more commits,
+            # etc.) so allowing them to directly unapprove it is also allowed.
+
+            # Because verify_auth has side-effects (especially, it may leave a
+            # comment on the pull request if the user is not authorized), we
+            # need to do the author check BEFORE the verify_auth check.
+            if state.author != username:
+                if not verify_auth(username, repo_label, repo_cfg, state,
+                                   AuthState.REVIEWER, realtime, my_username):
+                    continue
 
             state.approved_by = ''
             state.save()
@@ -610,10 +619,10 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
             state.save()
 
             if realtime:
-                state.add_comment(
-                    ':v: @{} can now approve this pull request'
-                    .format(state.delegate)
-                )
+                state.add_comment(comments.Delegated(
+                    delegator=username,
+                    delegate=state.delegate
+                ))
 
         elif command.action == 'undelegate':
             # TODO: why is this a TRY?
@@ -630,10 +639,10 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
             state.save()
 
             if realtime:
-                state.add_comment(
-                    ':v: @{} can now approve this pull request'
-                    .format(state.delegate)
-                )
+                state.add_comment(comments.Delegated(
+                    delegator=username,
+                    delegate=state.delegate
+                ))
 
         elif command.action == 'retry' and realtime:
             if not _try_auth_verified():
@@ -662,6 +671,10 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
 
             state.save()
             if realtime and state.try_:
+                # If we've tried before, the status will be 'success', and this
+                # new try will not be picked up. Set the status back to ''
+                # so the try will be run again.
+                state.set_status('')
                 # `try-` just resets the `try` bit and doesn't correspond to
                 # any meaningful labeling events.
                 state.change_labels(LabelEvent.TRY)
@@ -1224,7 +1237,11 @@ def start_build(state, repo_cfgs, buildbot_slots, logger, db, git_cfg):
         if found_travis_context and len(builders) == 1:
             can_try_travis_exemption = True
     if 'checks' in repo_cfg:
-        builders += ['checks-' + key for key, value in repo_cfg['checks'].items() if 'name' in value]  # noqa
+        builders += [
+            'checks-' + key
+            for key, value in repo_cfg['checks'].items()
+            if 'name' in value or (state.try_ and 'try_name' in value)
+        ]
         only_status_builders = False
 
     if len(builders) == 0:

homu/server.py

@@ -82,7 +82,7 @@ def result(repo_label, pull):
     states = [state for state in g.states[repo_label].values()
               if state.num == pull]
     if len(states) == 0:
-        abort(204, 'No build results for pull request {}'.format(pull))
+        abort(404, 'No build results for pull request {}'.format(pull))
 
     state = states[0]
     builders = []
@@ -94,15 +94,15 @@ def result(repo_label, pull):
         if data['res'] is not None:
             result = "success" if data['res'] else "failed"
 
-        if not data['url']:
-            # This happens to old try builds
-            abort(204, 'No build results for pull request {}'.format(pull))
-
-        builders.append({
-            'url': data['url'],
+        builder_details = {
             'result': result,
             'name': builder,
-        })
+        }
+
+        if data['url']:
+            builder_details['url'] = data['url']
+
+        builders.append(builder_details)
 
     return g.tpls['build_res'].render(repo_label=repo_label, repo_url=repo_url,
                                       builders=builders, pull=pull)
@@ -604,7 +604,10 @@ def fail(err):
         checks_name = None
         if 'checks' in repo_cfg:
             for name, value in repo_cfg['checks'].items():
-                if 'name' in value and value['name'] == current_run_name:
+                if state.try_ and 'try_name' in value:
+                    if value['try_name'] == current_run_name:
+                        checks_name = name
+                elif 'name' in value and value['name'] == current_run_name:
                     checks_name = name
         if checks_name is None:
             return 'OK'