runbot/runbot_merge/migrations/15.0.1.11/pre-migration.py
Xavier Morel d4fa1fd353 [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)
2024-05-23 07:58:46 +02:00

125 lines
4.8 KiB
Python

def move_fields(cr, *names):
cr.execute("""
UPDATE ir_model_data
SET module = 'runbot_merge'
WHERE module = 'forwardport'
AND model = 'runbot_merge_pull_requests'
AND name IN %s
""", [names])
def migrate(cr, version):
# cleanup some old crap
cr.execute("""
ALTER TABLE runbot_merge_project_freeze
DROP COLUMN IF EXISTS release_label,
DROP COLUMN IF EXISTS bump_label
""")
# fw constraint moved to mergebot, alongside all the fields it constrains
cr.execute("""
UPDATE ir_model_data
SET module = 'runbot_merge'
WHERE module = 'forwardport'
AND model = 'ir.model.constraint'
AND name = 'constraint_runbot_merge_pull_requests_fw_constraint'
""")
move_fields(
cr, 'merge_date', 'refname',
'limit_id', 'source_id', 'parent_id', 'root_id', 'forwardport_ids',
'detach_reason', 'fw_policy')
# view depends on pr.state, which prevents changing the state column's type
# we can just drop the view and it'll be recreated by the db update
cr.execute("DROP VIEW runbot_merge_freeze_labels")
# convert a few data types
cr.execute("""
CREATE TYPE runbot_merge_pull_requests_priority_type
AS ENUM ('default', 'priority', 'alone');
CREATE TYPE runbot_merge_pull_requests_state_type
AS ENUM ('opened', 'closed', 'validated', 'approved', 'ready', 'merged', 'error');
CREATE TYPE runbot_merge_pull_requests_merge_method_type
AS ENUM ('merge', 'rebase-merge', 'rebase-ff', 'squash');
CREATE TYPE runbot_merge_pull_requests_status_type
AS ENUM ('pending', 'failure', 'success');
ALTER TABLE runbot_merge_pull_requests
ALTER COLUMN priority
TYPE runbot_merge_pull_requests_priority_type
USING CASE WHEN priority = 0
THEN 'alone'
ELSE 'default'
END::runbot_merge_pull_requests_priority_type,
ALTER COLUMN state
TYPE runbot_merge_pull_requests_state_type
USING state::runbot_merge_pull_requests_state_type,
ALTER COLUMN merge_method
TYPE runbot_merge_pull_requests_merge_method_type
USING merge_method::runbot_merge_pull_requests_merge_method_type;
""")
cr.execute("""
ALTER TABLE runbot_merge_pull_requests
ADD COLUMN closed boolean not null default 'false',
ADD COLUMN error boolean not null default 'false',
ADD COLUMN skipchecks boolean not null default 'false',
ADD COLUMN cancel_staging boolean not null default 'false',
ADD COLUMN statuses text not null default '{}',
ADD COLUMN statuses_full text not null default '{}',
ADD COLUMN status runbot_merge_pull_requests_status_type not null default 'pending'
""")
# first pass: update all the new unconditional (or simple) fields
cr.execute("""
UPDATE runbot_merge_pull_requests p
SET closed = state = 'closed',
error = state = 'error',
skipchecks = priority = 'alone',
cancel_staging = priority = 'alone',
fw_policy = CASE fw_policy WHEN 'ci' THEN 'default' ELSE fw_policy END,
reviewed_by = CASE state
-- old version did not reset reviewer on PR update
WHEN 'opened' THEN NULL
WHEN 'validated' THEN NULL
-- if a PR predates the reviewed_by field, assign odoobot as reviewer
WHEN 'merged' THEN coalesce(reviewed_by, 2)
ELSE reviewed_by
END,
status = CASE state
WHEN 'validated' THEN 'success'
WHEN 'ready' THEN 'success'
WHEN 'merged' THEN 'success'
ELSE 'pending'
END::runbot_merge_pull_requests_status_type
""")
# the rest only gets updated if we have a matching commit which is not
# always the case
cr.execute("""
CREATE TEMPORARY TABLE parents ( id INTEGER not null, overrides jsonb not null );
WITH RECURSIVE parent_chain AS (
SELECT id, overrides::jsonb
FROM runbot_merge_pull_requests
WHERE parent_id IS NULL
UNION ALL
SELECT p.id, coalesce(pc.overrides || p.overrides::jsonb, pc.overrides, p.overrides::jsonb) as overrides
FROM runbot_merge_pull_requests p
JOIN parent_chain pc ON p.parent_id = pc.id
)
INSERT INTO parents SELECT * FROM parent_chain;
CREATE INDEX ON parents (id);
UPDATE runbot_merge_pull_requests p
SET statuses = jsonb_pretty(c.statuses::jsonb)::text,
statuses_full = jsonb_pretty(
c.statuses::jsonb
|| coalesce((select overrides from parents where id = p.parent_id), '{}')
|| overrides::jsonb
)::text
FROM runbot_merge_commit c
WHERE p.head = c.sha
""")