diff --git a/cfg.sample.toml b/cfg.sample.toml index ca26b96..339636f 100644 --- a/cfg.sample.toml +++ b/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] diff --git a/homu/comments.py b/homu/comments.py index 276d8fa..bd2f7a6 100644 --- a/homu/comments.py +++ b/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" + \ + "" + 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"] diff --git a/homu/html/build_res.html b/homu/html/build_res.html index 0fdf7af..0e70b43 100644 --- a/homu/html/build_res.html +++ b/homu/html/build_res.html @@ -47,7 +47,13 @@

Homu build results - {{ {{loop.index}} {{builder.name}} - {{builder.result}} + + {%- if builder.url -%} + {{builder.result}} + {%- else -%} + {{ builder.result }} + {%- endif -%} + {% endfor %} diff --git a/homu/main.py b/homu/main.py index db14be9..59b8c00 100644 --- a/homu/main.py +++ b/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' # 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: diff --git a/homu/server.py b/homu/server.py index a61661e..1d90136 100644 --- a/homu/server.py +++ b/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'