runbot/runbot_merge/controllers/__init__.py

444 lines
17 KiB
Python
Raw Permalink Normal View History

import hashlib
import hmac
import logging
import json
from datetime import datetime, timedelta
import sentry_sdk
import werkzeug.exceptions
from odoo.http import Controller, request, route
2018-06-18 15:08:48 +07:00
from . import dashboard
from . import reviewer_provisioning
from .. import utils, github
2018-06-18 15:08:48 +07:00
_logger = logging.getLogger(__name__)
class MergebotController(Controller):
[ADD] runbot_merge: staging query endpoints `/runbot_merge/stagings` ======================== This endpoint is a reverse lookup from any number of commits to a (number of) staging(s): - it takes a list of commit hashes as either the `commits` or the `heads` keyword parameter - it then returns the stagings which have *all* these commits as respectively commits or heads, if providing all commits for a project the result should always be unique (if any) - `commits` are the merged commits, aka the stuff which ends up in the actual branches - `heads` are the staging heads, aka the commits at the tip of the `staging.$name` branches, those may be the same as the corresponding commit, or might be deduplicator commits which get discarded on success `/runbot_merge/stagings/:id` ============================ Returns a list of all PRs in the staging, grouped by batch (aka PRs which have the same label and must be merged together). For each PR, the `repository` name, `number`, and `name` in the form `$repository#$number` get returned. `/runbot_merge/stagings/:id1/:id2` ================================== Returns a list of all the *successfully merged* stagings between `id1` and `id2`, from oldest to most recent. Individual records have the form: - `staging` is the id of the staging - `prs` is the contents of the previous endpoint (a list of PRs grouped by batch) `id1` *must* be lower than `id2`. By default, this endpoint is inclusive on both ends, the `include_from` and / or `include_to` parameters can be passed with the `False` value to exclude the corresponding bound from the result. Related to #768
2023-08-11 16:13:34 +07:00
@route('/runbot_merge/stagings', auth='none', type='json')
def stagings_for_commits(self, commits=None, heads=None):
Stagings = request.env(user=1)['runbot_merge.stagings'].sudo()
if commits:
stagings = Stagings.for_commits(*commits)
elif heads:
stagings = Stagings.for_heads(*heads)
else:
raise ValueError('Must receive one of "commits" or "heads" kwarg')
return stagings.ids
@route('/runbot_merge/stagings/<int:staging>', auth='none', type='json')
def prs_for_staging(self, staging):
staging = request.env(user=1)['runbot_merge.stagings'].browse(staging)
return [
batch.prs.mapped(lambda p: {
'name': p.display_name,
'repository': p.repository.name,
'number': p.number,
})
for batch in staging.sudo().batch_ids
]
@route('/runbot_merge/stagings/<int:from_staging>/<int:to_staging>', auth='none', type='json')
def prs_for_stagings(self, from_staging, to_staging, include_from=True, include_to=True):
[ADD] runbot_merge: staging query endpoints `/runbot_merge/stagings` ======================== This endpoint is a reverse lookup from any number of commits to a (number of) staging(s): - it takes a list of commit hashes as either the `commits` or the `heads` keyword parameter - it then returns the stagings which have *all* these commits as respectively commits or heads, if providing all commits for a project the result should always be unique (if any) - `commits` are the merged commits, aka the stuff which ends up in the actual branches - `heads` are the staging heads, aka the commits at the tip of the `staging.$name` branches, those may be the same as the corresponding commit, or might be deduplicator commits which get discarded on success `/runbot_merge/stagings/:id` ============================ Returns a list of all PRs in the staging, grouped by batch (aka PRs which have the same label and must be merged together). For each PR, the `repository` name, `number`, and `name` in the form `$repository#$number` get returned. `/runbot_merge/stagings/:id1/:id2` ================================== Returns a list of all the *successfully merged* stagings between `id1` and `id2`, from oldest to most recent. Individual records have the form: - `staging` is the id of the staging - `prs` is the contents of the previous endpoint (a list of PRs grouped by batch) `id1` *must* be lower than `id2`. By default, this endpoint is inclusive on both ends, the `include_from` and / or `include_to` parameters can be passed with the `False` value to exclude the corresponding bound from the result. Related to #768
2023-08-11 16:13:34 +07:00
Stagings = request.env(user=1, context={"active_test": False})['runbot_merge.stagings']
from_staging = Stagings.browse(from_staging)
to_staging = Stagings.browse(to_staging)
if from_staging.target != to_staging.target:
raise ValueError(f"Stagings must have the same target branch, found {from_staging.target.name} and {to_staging.target.name}")
if from_staging.id >= to_staging.id:
raise ValueError("first staging must be older than second staging")
stagings = Stagings.search([
('target', '=', to_staging.target.id),
('state', '=', 'success'),
('id', '>=' if include_from else '>', from_staging.id),
('id', '<=' if include_to else '<', to_staging.id),
], order="id asc")
return [
{
'staging': staging.id,
'prs': [
batch.prs.mapped(lambda p: {
'name': p.display_name,
'repository': p.repository.name,
'number': p.number,
})
for batch in staging.batch_ids
]
}
for staging in stagings
]
@route('/runbot_merge/hooks', auth='none', type='json', csrf=False, methods=['POST'])
def index(self):
req = request.httprequest
event = req.headers['X-Github-Event']
with sentry_sdk.configure_scope() as scope:
if scope.transaction:
# only in 1.8.0 (or at least 1.7.2
if hasattr(scope, 'set_transaction_name'):
scope.set_transaction_name(f"webhook {event}")
else: # but our servers use 1.4.3
scope.transaction = f"webhook {event}"
github._gh.info(self._format(req))
data = request.get_json_data()
repo = data.get('repository', {}).get('full_name')
env = request.env(user=1)
source = repo and env['runbot_merge.events_sources'].search([('repository', '=', repo)])
if not source:
_logger.warning(
"Ignored hook %s to unknown source repository %s",
req.headers.get("X-Github-Delivery"),
repo,
)
return werkzeug.exceptions.Forbidden()
elif secret := source.secret:
signature = 'sha256=' + hmac.new(secret.strip().encode(), req.get_data(), hashlib.sha256).hexdigest()
if not hmac.compare_digest(signature, req.headers.get('X-Hub-Signature-256', '')):
_logger.warning(
"Ignored hook %s with incorrect signature on %s: got %s expected %s, in:\n%s",
req.headers.get('X-Github-Delivery'),
repo,
req.headers.get('X-Hub-Signature-256'),
signature,
req.headers,
)
return werkzeug.exceptions.Forbidden()
elif req.headers.get('X-Hub-Signature-256'):
_logger.info("No secret for %s but received a signature in:\n%s", repo, req.headers)
else:
_logger.info("No secret or signature for %s", repo)
c = EVENTS.get(event)
if not c:
_logger.warning('Unknown event %s', event)
return 'Unknown event {}'.format(event)
sentry_sdk.set_context('webhook', data)
return c(env, data)
def _format(self, request):
return """{r.method} {r.full_path}
{headers}
{body}
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv\
""".format(
r=request,
headers='\n'.join(
'\t%s: %s' % entry for entry in request.headers.items()
),
body=request.get_data(as_text=True),
)
def handle_pr(env, event):
if event['action'] in [
'assigned', 'unassigned', 'review_requested', 'review_request_removed',
'labeled', 'unlabeled'
]:
_logger.debug(
'Ignoring pull_request[%s] on %s#%s',
event['action'],
event['pull_request']['base']['repo']['full_name'],
event['pull_request']['number'],
)
2018-03-26 18:08:49 +07:00
return 'Ignoring'
pr = event['pull_request']
r = pr['base']['repo']['full_name']
b = pr['base']['ref']
repo = env['runbot_merge.repository'].search([('name', '=', r)])
if not repo:
_logger.warning("Received a PR for %s but not configured to handle that repo", r)
# sadly shit's retarded so odoo json endpoints really mean
# jsonrpc and it's LITERALLY NOT POSSIBLE TO REPLY WITH
# ACTUAL RAW HTTP RESPONSES and thus not possible to
# report actual errors to the webhooks listing thing on
# github (not that we'd be looking at them but it'd be
# useful for tests)
2018-03-26 18:08:49 +07:00
return "Not configured to handle {}".format(r)
# PRs to unmanaged branches are not necessarily abnormal and
# we don't care
branch = env['runbot_merge.branch'].with_context(active_test=False).search([
('name', '=', b),
('project_id', '=', repo.project_id.id),
])
def feedback(**info):
return env['runbot_merge.pull_requests.feedback'].create({
'repository': repo.id,
'pull_request': pr['number'],
**info,
})
def find(target):
return env['runbot_merge.pull_requests'].search([
('repository', '=', repo.id),
('number', '=', pr['number']),
# ('target', '=', target.id),
])
# edition difficulty: pr['base']['ref] is the *new* target, the old one
# is at event['change']['base']['ref'] (if the target changed), so edition
# handling must occur before the rest of the steps
if event['action'] == 'edited':
source = event['changes'].get('base', {'ref': {'from': b}})['ref']['from']
source_branch = env['runbot_merge.branch'].with_context(active_test=False).search([
('name', '=', source),
('project_id', '=', repo.project_id.id),
])
# retargeting to un-managed => delete
if not branch:
pr = find(source_branch)
pr.unlink()
2018-03-26 18:08:49 +07:00
return 'Retargeted {} to un-managed branch {}, deleted'.format(pr.id, b)
# retargeting from un-managed => create
if not source_branch:
return handle_pr(env, dict(event, action='opened'))
pr_obj = find(source_branch)
updates = {}
if source_branch != branch:
if branch != pr_obj.target:
updates['target'] = branch.id
updates['squash'] = pr['commits'] == 1
if 'title' in event['changes'] or 'body' in event['changes']:
updates['message'] = utils.make_message(pr)
_logger.info("update: %s = %s (by %s)", pr_obj.display_name, updates, event['sender']['login'])
if updates:
# copy because it updates the `updates` dict internally
pr_obj.write(dict(updates))
return 'Updated {}'.format(', '.join(updates))
return "Nothing to update ({})".format(', '.join(event['changes'].keys()))
message = None
if not branch:
message = env.ref('runbot_merge.handle.branch.unmanaged')._format(
repository=r,
branch=b,
event=event,
)
_logger.info("Ignoring event %s on PR %s#%d for un-managed branch %s",
event['action'], r, pr['number'], b)
elif not branch.active:
message = env.ref('runbot_merge.handle.branch.inactive')._format(
repository=r,
branch=b,
event=event,
)
if message and event['action'] not in ('synchronize', 'closed'):
feedback(message=message)
if not branch:
2018-03-26 18:08:49 +07:00
return "Not set up to care about {}:{}".format(r, b)
headers = request.httprequest.headers if request else {}
_logger.info(
"%s: %s#%s (%s) (by %s, delivery %s by %s)",
event['action'],
repo.name, pr['number'],
pr['title'].strip(),
event['sender']['login'],
headers.get('X-Github-Delivery'),
headers.get('User-Agent'),
)
sender = env['res.partner'].search([('github_login', '=', event['sender']['login'])], limit=1)
if not sender:
sender = env['res.partner'].create({'name': event['sender']['login'], 'github_login': event['sender']['login']})
env['runbot_merge.pull_requests']._track_set_author(sender, fallback=True)
if event['action'] == 'opened':
author_name = pr['user']['login']
author = env['res.partner'].search([('github_login', '=', author_name)], limit=1)
if not author:
env['res.partner'].create({'name': author_name, 'github_login': author_name})
pr_obj = env['runbot_merge.pull_requests']._from_gh(pr)
2018-03-26 18:08:49 +07:00
return "Tracking PR as {}".format(pr_obj.id)
pr_obj = env['runbot_merge.pull_requests']._get_or_schedule(r, pr['number'], closing=event['action'] == 'closed')
if not pr_obj:
_logger.info("webhook %s on unknown PR %s#%s, scheduled fetch", event['action'], repo.name, pr['number'])
return "Unknown PR {}:{}, scheduling fetch".format(repo.name, pr['number'])
if event['action'] == 'synchronize':
if pr_obj.head == pr['head']['sha']:
_logger.warning(
"PR %s sync %s -> %s => same head",
pr_obj.display_name,
pr_obj.head,
pr['head']['sha'],
)
2018-03-26 18:08:49 +07:00
return 'No update to pr head'
if pr_obj.state in ('closed', 'merged'):
_logger.error("PR %s sync %s -> %s => failure (closed)", pr_obj.display_name, pr_obj.head, pr['head']['sha'])
return "It's my understanding that closed/merged PRs don't get sync'd"
_logger.info(
"PR %s sync %s -> %s by %s => reset to 'open' and squash=%s",
pr_obj.display_name,
pr_obj.head,
pr['head']['sha'],
event['sender']['login'],
pr['commits'] == 1
)
if pr['base']['ref'] != pr_obj.target.name:
env['runbot_merge.fetch_job'].create({
'repository': pr_obj.repository.id,
'number': pr_obj.number,
'commits_at': datetime.now() + timedelta(minutes=5),
})
pr_obj.write({
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
'reviewed_by': False,
'error': False,
'head': pr['head']['sha'],
'squash': pr['commits'] == 1,
})
return f'Updated to {pr_obj.head}'
if event['action'] == 'ready_for_review':
pr_obj.draft = False
return f'Updated {pr_obj.display_name} to ready'
if event['action'] == 'converted_to_draft':
pr_obj.draft = True
return f'Updated {pr_obj.display_name} to draft'
# don't marked merged PRs as closed (!!!)
if event['action'] == 'closed' and pr_obj.state != 'merged':
oldstate = pr_obj.state
if pr_obj._try_closing(event['sender']['login']):
_logger.info(
'%s closed %s (state=%s)',
event['sender']['login'],
pr_obj.display_name,
oldstate,
)
return 'Closed {}'.format(pr_obj.display_name)
_logger.info(
'%s tried to close %s (state=%s) but locking failed',
event['sender']['login'],
pr_obj.display_name,
oldstate,
)
return 'Ignored: could not lock rows (probably being merged)'
if event['action'] == 'reopened' :
if pr_obj.merge_date:
if pr_obj.state == 'merged':
message = env.ref('runbot_merge.handle.pr.merged')._format(event=event)
else:
message = env.ref('runbot_merge.handle.pr.mergedbatch')._format(event=event)
feedback(close=True, message=message)
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
elif pr_obj.closed:
_logger.info('%s reopening %s', event['sender']['login'], pr_obj.display_name)
pr_obj.write({
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
'closed': False,
# updating the head triggers a revalidation, and unstages the batch
'head': pr['head']['sha'],
'squash': pr['commits'] == 1,
})
return 'Reopened {}'.format(pr_obj.display_name)
_logger.info("Ignoring event %s on PR %s", event['action'], pr['number'])
2018-03-26 18:08:49 +07:00
return "Not handling {} yet".format(event['action'])
def handle_status(env, event):
_logger.info(
'status on %(sha)s %(context)s:%(state)s (%(target_url)s) [%(description)r]',
event
)
[IMP] runbot_merge: remove unnecessary uniquifier dummy commits "Uniquifier" commits were introduced to ensure branches of a staging on which nothing had been staged would still be rebuilt properly. This means technically the branches on which something had been staged never *needed* a uniquifier, strictly speaking. And those lead to extra building, because once the actually staged PRs get pushed from staging to their final destination it's an unknown commit to the runbot, which needs to rebuild it instead of being able to just use the staging it already has. Thus only add the uniquifier where it *might* be necessary: technically the runbot should not manage this use case much better, however there are still issues like an ancillary build working with the same branch tip (e.g. the "current master") and sending a failure result which would fail the entire staging. The uniquifier guards against this issue. Also update rebase semantics to always update the *commit date* of the rebased commits: this ensures the tip commit is always "recent" in the case of a rebase-ff (which is common as that's what single-commit PRs do), as the runbot may skip commits it considers "old". Also update some of the utility methods around repos / commits to be simpler, and avoid assuming the result is JSON-decodable (sometimes it is not). Also update the handling of commit statuses using postgres' ON CONFLICT and jsonb support, hopefully this improves (or even fixes) the serialization errors. Should be compatible with 9.5 onwards which is *ancient* at this point. Fixes #509
2021-08-09 18:21:24 +07:00
status_value = json.dumps({
event['context']: {
'state': event['state'],
'target_url': event['target_url'],
'description': event['description'],
'updated_at': datetime.now().isoformat(timespec='seconds'),
}
[IMP] runbot_merge: remove unnecessary uniquifier dummy commits "Uniquifier" commits were introduced to ensure branches of a staging on which nothing had been staged would still be rebuilt properly. This means technically the branches on which something had been staged never *needed* a uniquifier, strictly speaking. And those lead to extra building, because once the actually staged PRs get pushed from staging to their final destination it's an unknown commit to the runbot, which needs to rebuild it instead of being able to just use the staging it already has. Thus only add the uniquifier where it *might* be necessary: technically the runbot should not manage this use case much better, however there are still issues like an ancillary build working with the same branch tip (e.g. the "current master") and sending a failure result which would fail the entire staging. The uniquifier guards against this issue. Also update rebase semantics to always update the *commit date* of the rebased commits: this ensures the tip commit is always "recent" in the case of a rebase-ff (which is common as that's what single-commit PRs do), as the runbot may skip commits it considers "old". Also update some of the utility methods around repos / commits to be simpler, and avoid assuming the result is JSON-decodable (sometimes it is not). Also update the handling of commit statuses using postgres' ON CONFLICT and jsonb support, hopefully this improves (or even fixes) the serialization errors. Should be compatible with 9.5 onwards which is *ancient* at this point. Fixes #509
2021-08-09 18:21:24 +07:00
})
# create status, or merge update into commit *unless* the update is already
# part of the status (dupe status)
env.cr.execute("""
INSERT INTO runbot_merge_commit AS c (sha, to_check, statuses)
VALUES (%s, true, %s)
ON CONFLICT (sha) DO UPDATE
SET to_check = true,
statuses = c.statuses::jsonb || EXCLUDED.statuses::jsonb
WHERE NOT c.statuses::jsonb @> EXCLUDED.statuses::jsonb
""", [event['sha'], status_value], log_exceptions=False)
env.ref("runbot_merge.process_updated_commits")._trigger()
return 'ok'
def handle_comment(env, event):
if 'pull_request' not in event['issue']:
return "issue comment, ignoring"
repo = event['repository']['full_name']
issue = event['issue']['number']
author = event['comment']['user']['login']
comment = event['comment']['body']
if len(comment) > 5000:
_logger.warning('comment(%s): %s %s#%s => ignored (%d characters)', event['comment']['html_url'], author, repo, issue, len(comment))
return "ignored: too big"
_logger.info('comment[%s]: %s %s#%s %r', event['action'], author, repo, issue, comment)
if event['action'] != 'created':
return "Ignored: action (%r) is not 'created'" % event['action']
return _handle_comment(env, repo, issue, event['comment'])
def handle_review(env, event):
repo = event['repository']['full_name']
pr = event['pull_request']['number']
author = event['review']['user']['login']
comment = event['review']['body'] or ''
if len(comment) > 5000:
_logger.warning('comment(%s): %s %s#%s => ignored (%d characters)', event['review']['html_url'], author, repo, pr, len(comment))
return "ignored: too big"
_logger.info('review[%s]: %s %s#%s %r', event['action'], author, repo, pr, comment)
if event['action'] != 'submitted':
return "Ignored: action (%r) is not 'submitted'" % event['action']
return _handle_comment(
env, repo, pr, event['review'],
target=event['pull_request']['base']['ref'])
def handle_ping(env, event):
_logger.info("Got ping! %s", event['zen'])
return "pong"
EVENTS = {
'pull_request': handle_pr,
'status': handle_status,
'issue_comment': handle_comment,
'pull_request_review': handle_review,
'ping': handle_ping,
}
def _handle_comment(env, repo, issue, comment, target=None):
repository = env['runbot_merge.repository'].search([('name', '=', repo)])
if not repository.project_id._find_commands(comment['body'] or ''):
return "No commands, ignoring"
pr = env['runbot_merge.pull_requests']._get_or_schedule(
repo, issue, target=target, commenter=comment['user']['login'],
)
if not pr:
return "Unknown PR, scheduling fetch"
partner = env['res.partner'].search([('github_login', '=', comment['user']['login'])])
return pr._parse_commands(partner, comment, comment['user']['login'])