2021-11-10 19:13:34 +07:00
|
|
|
import logging
|
|
|
|
import re
|
[CHG] *: move forward-porting over to batches
Thank god I have a bunch of tests because once again I forgot / missed
a bunch of edge cases in doing the conversion, which the tests
caught (sadly that means I almost certainly broke a few untested edge
cases).
Important notes:
Handling of parent links
------------------------
Unlike PRs, batches don't lose their parent info ever, the link is
permanent, which is convenient to trawl through a forward port
(currently implemented very inefficiently, maybe we'll optimise that
in the future).
However this means the batch having a parent and the batch's PRs
having parents are slightly different informations, one of the edge
cases I missed is that of conflicting PRs, which are deparented and
have to be merged by hand before being forward ported further, I had
originally replaced the checks on a pr and its sibling having parents
by just the batch.
Batches & targets
-----------------
Batches were originally concepted as being fixed to a target and PRs
having that target, a PR being retargeted would move it from one batch
to an other.
As it turns out this does not work in the case where people retarget
forward-port PRs, which I know they do because #551
(2337bd85186de000d074e5a7178cfeb110d15de7). I could not think of a
good way to handle this issue as is, so scrapped the moving PRs thing,
instead one of the coherence checks of a batch being ready is that all
its PRs have the same target, and a batch only has a target if all its
PRs have the same target.
It's possible for somewhat odd effects to arise, notably if a PR is
closed (removed from batch), the other PRs are retargeted, and the new
PR is reopened, it will now be on a separate batch even if it also
gets retargeted. This is weird. I don't quite know how I should handle
it, maybe batches could merge if they have the same target and label?
however batches don't currently have a label so...
Improve limits
--------------
Keep limits on the PRs rather than lift them on the batchL if we can
add/remove PRs of batches having different limits on different PRs of
the same batch is reasonable.
Also leave limit unset by default: previously, the limit was eagerly
set to the tip (accessible) branch. That doesn't really seem
necessary, so stop doing that.
Also remove completely unnecessary `max` when trying to find a PR's
next target: `root` is either `self` or `self.source_id`, so it should
not be possible for that to have a later target.
And for now ensure the limits are consistent per batch: a PR defaults
to the limit of their batch-mate if they don't have one, and if a
limit is set via command it's set on all PRs of a batch.
This commit does not allow differential limits via commands, they are
allowed via the backend but not really tested. The issue is mostly
that it's not clear what the UX should look like to have clear and not
super error prone interactions. So punt on it for now, and hopefully
there's no hole I missed which will create inconsistent batches.
2024-05-21 20:45:53 +07:00
|
|
|
from typing import List
|
2021-11-10 19:13:34 +07:00
|
|
|
|
2023-08-22 13:03:26 +07:00
|
|
|
import requests
|
2023-06-15 13:22:38 +07:00
|
|
|
import sentry_sdk
|
|
|
|
|
2023-08-22 13:03:26 +07:00
|
|
|
from odoo import models, fields, api
|
[CHG] *: move forward-porting over to batches
Thank god I have a bunch of tests because once again I forgot / missed
a bunch of edge cases in doing the conversion, which the tests
caught (sadly that means I almost certainly broke a few untested edge
cases).
Important notes:
Handling of parent links
------------------------
Unlike PRs, batches don't lose their parent info ever, the link is
permanent, which is convenient to trawl through a forward port
(currently implemented very inefficiently, maybe we'll optimise that
in the future).
However this means the batch having a parent and the batch's PRs
having parents are slightly different informations, one of the edge
cases I missed is that of conflicting PRs, which are deparented and
have to be merged by hand before being forward ported further, I had
originally replaced the checks on a pr and its sibling having parents
by just the batch.
Batches & targets
-----------------
Batches were originally concepted as being fixed to a target and PRs
having that target, a PR being retargeted would move it from one batch
to an other.
As it turns out this does not work in the case where people retarget
forward-port PRs, which I know they do because #551
(2337bd85186de000d074e5a7178cfeb110d15de7). I could not think of a
good way to handle this issue as is, so scrapped the moving PRs thing,
instead one of the coherence checks of a batch being ready is that all
its PRs have the same target, and a batch only has a target if all its
PRs have the same target.
It's possible for somewhat odd effects to arise, notably if a PR is
closed (removed from batch), the other PRs are retargeted, and the new
PR is reopened, it will now be on a separate batch even if it also
gets retargeted. This is weird. I don't quite know how I should handle
it, maybe batches could merge if they have the same target and label?
however batches don't currently have a label so...
Improve limits
--------------
Keep limits on the PRs rather than lift them on the batchL if we can
add/remove PRs of batches having different limits on different PRs of
the same batch is reasonable.
Also leave limit unset by default: previously, the limit was eagerly
set to the tip (accessible) branch. That doesn't really seem
necessary, so stop doing that.
Also remove completely unnecessary `max` when trying to find a PR's
next target: `root` is either `self` or `self.source_id`, so it should
not be possible for that to have a later target.
And for now ensure the limits are consistent per batch: a PR defaults
to the limit of their batch-mate if they don't have one, and if a
limit is set via command it's set on all PRs of a batch.
This commit does not allow differential limits via commands, they are
allowed via the backend but not really tested. The issue is mostly
that it's not clear what the UX should look like to have clear and not
super error prone interactions. So punt on it for now, and hopefully
there's no hole I missed which will create inconsistent batches.
2024-05-21 20:45:53 +07:00
|
|
|
from odoo.osv import expression
|
|
|
|
from odoo.tools import reverse_order
|
2021-11-10 19:13:34 +07:00
|
|
|
|
|
|
|
_logger = logging.getLogger(__name__)
|
|
|
|
class Project(models.Model):
|
|
|
|
_name = _description = 'runbot_merge.project'
|
|
|
|
|
|
|
|
name = fields.Char(required=True, index=True)
|
|
|
|
repo_ids = fields.One2many(
|
|
|
|
'runbot_merge.repository', 'project_id',
|
|
|
|
help="Repos included in that project, they'll be staged together. "\
|
|
|
|
"*Not* to be used for cross-repo dependencies (that is to be handled by the CI)"
|
|
|
|
)
|
|
|
|
branch_ids = fields.One2many(
|
|
|
|
'runbot_merge.branch', 'project_id',
|
|
|
|
context={'active_test': False},
|
|
|
|
help="Branches of all project's repos which are managed by the merge bot. Also "\
|
|
|
|
"target branches of PR this project handles."
|
|
|
|
)
|
2023-11-29 19:58:40 +07:00
|
|
|
staging_enabled = fields.Boolean(default=True)
|
2023-11-30 18:27:09 +07:00
|
|
|
staging_priority = fields.Selection([
|
|
|
|
('default', "Splits over ready PRs"),
|
|
|
|
('largest', "Largest of split and ready PRs"),
|
|
|
|
('ready', "Ready PRs over split"),
|
|
|
|
], default="default", required=True)
|
2024-05-31 17:33:13 +07:00
|
|
|
staging_statuses = fields.Boolean(default=True)
|
2024-06-03 17:59:24 +07:00
|
|
|
staging_rpc = fields.Boolean(default=False)
|
2021-11-10 19:13:34 +07:00
|
|
|
|
|
|
|
ci_timeout = fields.Integer(
|
2022-12-07 21:13:55 +07:00
|
|
|
default=60, required=True, group_operator=None,
|
2021-11-10 19:13:34 +07:00
|
|
|
help="Delay (in minutes) before a staging is considered timed out and failed"
|
|
|
|
)
|
|
|
|
|
|
|
|
github_token = fields.Char("Github Token", required=True)
|
2024-11-29 15:04:06 +07:00
|
|
|
github_name = fields.Char(store=True, compute="_compute_identity", required=True, copy=True)
|
|
|
|
github_email = fields.Char(store=True, compute="_compute_identity", required=True, copy=True)
|
2021-11-10 19:13:34 +07:00
|
|
|
github_prefix = fields.Char(
|
|
|
|
required=True,
|
|
|
|
default="hanson", # mergebot du bot du bot du~
|
|
|
|
help="Prefix (~bot name) used when sending commands from PR "
|
[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
|
|
|
"comments e.g. [hanson retry] or [hanson r+ priority]",
|
2021-11-10 19:13:34 +07:00
|
|
|
)
|
2023-10-16 15:46:29 +07:00
|
|
|
fp_github_token = fields.Char()
|
|
|
|
fp_github_name = fields.Char(store=True, compute="_compute_git_identity")
|
2021-11-10 19:13:34 +07:00
|
|
|
|
|
|
|
batch_limit = fields.Integer(
|
2022-12-07 21:13:55 +07:00
|
|
|
default=8, group_operator=None, help="Maximum number of PRs staged together")
|
2021-11-10 19:13:34 +07:00
|
|
|
|
2021-11-12 22:04:34 +07:00
|
|
|
freeze_id = fields.Many2one('runbot_merge.project.freeze', compute='_compute_freeze')
|
|
|
|
freeze_reminder = fields.Text()
|
|
|
|
|
2023-10-06 20:19:36 +07:00
|
|
|
uniquifier = fields.Boolean(
|
|
|
|
default=True,
|
|
|
|
help="Whether to add a uniquifier commit on repositories without PRs"
|
|
|
|
" during staging. The lack of uniquifier can lead to CI conflicts"
|
|
|
|
" as github works off of commits, so it's possible for an"
|
|
|
|
" unrelated build to trigger a failure if somebody is a dummy and"
|
|
|
|
" includes repos they have no commit for."
|
|
|
|
)
|
|
|
|
|
2023-08-22 13:03:26 +07:00
|
|
|
@api.depends('github_token')
|
|
|
|
def _compute_identity(self):
|
|
|
|
s = requests.Session()
|
|
|
|
for project in self:
|
|
|
|
if not project.github_token or (project.github_name and project.github_email):
|
|
|
|
continue
|
|
|
|
|
2024-11-29 15:04:06 +07:00
|
|
|
headers = {'Authorization': f'token {project.github_token}'}
|
|
|
|
r0 = s.get('https://api.github.com/user', headers=headers)
|
2023-08-22 13:03:26 +07:00
|
|
|
if not r0.ok:
|
2024-11-29 15:04:06 +07:00
|
|
|
_logger.warning("Failed to fetch merge bot information for project %s: %s", project.name, r0.text or r0.content)
|
2023-08-22 13:03:26 +07:00
|
|
|
continue
|
|
|
|
|
2024-02-12 16:18:59 +07:00
|
|
|
r = r0.json()
|
|
|
|
project.github_name = r['name'] or r['login']
|
|
|
|
if email := r['email']:
|
2023-08-22 13:03:26 +07:00
|
|
|
project.github_email = email
|
|
|
|
continue
|
|
|
|
|
|
|
|
if 'user:email' not in set(re.split(r',\s*', r0.headers['x-oauth-scopes'])):
|
2024-11-29 15:04:06 +07:00
|
|
|
_logger.warning("Unable to fetch merge bot emails for project %s: scope missing from token", project.name)
|
|
|
|
r1 = s.get('https://api.github.com/user/emails', headers=headers)
|
2023-08-22 13:03:26 +07:00
|
|
|
if not r1.ok:
|
2024-11-29 15:04:06 +07:00
|
|
|
_logger.warning("Failed to fetch merge bot emails for project %s: %s", project.name, r1.text or r1.content)
|
2023-08-22 13:03:26 +07:00
|
|
|
continue
|
2024-11-29 15:04:06 +07:00
|
|
|
|
2023-08-22 13:03:26 +07:00
|
|
|
project.github_email = next((
|
|
|
|
entry['email']
|
|
|
|
for entry in r1.json()
|
|
|
|
if entry['primary']
|
|
|
|
), None)
|
|
|
|
|
2023-10-16 15:46:29 +07:00
|
|
|
# technically the email could change at any moment...
|
|
|
|
@api.depends('fp_github_token')
|
|
|
|
def _compute_git_identity(self):
|
|
|
|
s = requests.Session()
|
|
|
|
for project in self:
|
[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
|
|
|
if project.fp_github_name or not project.fp_github_token:
|
2023-10-16 15:46:29 +07:00
|
|
|
continue
|
|
|
|
|
|
|
|
r0 = s.get('https://api.github.com/user', headers={
|
|
|
|
'Authorization': 'token %s' % project.fp_github_token
|
|
|
|
})
|
|
|
|
if not r0.ok:
|
|
|
|
_logger.error("Failed to fetch forward bot information for project %s: %s", project.name, r0.text or r0.content)
|
|
|
|
continue
|
|
|
|
|
|
|
|
user = r0.json()
|
|
|
|
project.fp_github_name = user['name'] or user['login']
|
|
|
|
|
2021-11-10 19:13:34 +07:00
|
|
|
def _check_stagings(self, commit=False):
|
2023-06-08 13:19:43 +07:00
|
|
|
# check branches with an active staging
|
|
|
|
for branch in self.env['runbot_merge.branch']\
|
|
|
|
.with_context(active_test=False)\
|
|
|
|
.search([('active_staging_id', '!=', False)]):
|
2021-11-10 19:13:34 +07:00
|
|
|
staging = branch.active_staging_id
|
|
|
|
try:
|
|
|
|
with self.env.cr.savepoint():
|
|
|
|
staging.check_status()
|
|
|
|
except Exception:
|
|
|
|
_logger.exception("Failed to check staging for branch %r (staging %s)",
|
|
|
|
branch.name, staging)
|
|
|
|
else:
|
|
|
|
if commit:
|
|
|
|
self.env.cr.commit()
|
|
|
|
|
|
|
|
def _create_stagings(self, commit=False):
|
2023-08-16 20:31:42 +07:00
|
|
|
from .stagings_create import try_staging
|
|
|
|
|
2023-06-08 13:19:43 +07:00
|
|
|
# look up branches which can be staged on and have no active staging
|
|
|
|
for branch in self.env['runbot_merge.branch'].search([
|
|
|
|
('active_staging_id', '=', False),
|
|
|
|
('active', '=', True),
|
|
|
|
('staging_enabled', '=', True),
|
2023-11-29 19:58:40 +07:00
|
|
|
('project_id.staging_enabled', '=', True),
|
2023-06-08 13:19:43 +07:00
|
|
|
]):
|
2024-10-02 17:14:09 +07:00
|
|
|
try:
|
|
|
|
with self.env.cr.savepoint():
|
|
|
|
if not self.env['runbot_merge.patch']._apply_patches(branch):
|
|
|
|
self.env.ref("runbot_merge.staging_cron")._trigger()
|
|
|
|
return
|
|
|
|
|
|
|
|
except Exception:
|
|
|
|
_logger.exception("Failed to apply patches to branch %r", branch.name)
|
|
|
|
else:
|
|
|
|
if commit:
|
|
|
|
self.env.cr.commit()
|
|
|
|
|
2023-06-08 13:19:43 +07:00
|
|
|
try:
|
2023-06-15 13:22:38 +07:00
|
|
|
with self.env.cr.savepoint(), \
|
|
|
|
sentry_sdk.start_span(description=f'create staging {branch.name}') as span:
|
|
|
|
span.set_tag('branch', branch.name)
|
2023-08-16 20:31:42 +07:00
|
|
|
try_staging(branch)
|
2023-06-08 13:19:43 +07:00
|
|
|
except Exception:
|
|
|
|
_logger.exception("Failed to create staging for branch %r", branch.name)
|
|
|
|
else:
|
|
|
|
if commit:
|
|
|
|
self.env.cr.commit()
|
2021-11-10 19:13:34 +07:00
|
|
|
|
[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
|
|
|
def _find_commands(self, comment: str) -> List[str]:
|
2023-10-16 15:46:29 +07:00
|
|
|
"""Tries to find all the lines starting (ignoring leading whitespace)
|
|
|
|
with either the merge or the forward port bot identifiers.
|
|
|
|
|
|
|
|
For convenience, the identifier *can* be prefixed with an ``@`` or
|
|
|
|
``#``, and suffixed with a ``:``.
|
|
|
|
"""
|
|
|
|
# horizontal whitespace (\s - {\n, \r}), but Python doesn't have \h or \p{Blank}
|
|
|
|
h = r'[^\S\r\n]'
|
2021-11-10 19:13:34 +07:00
|
|
|
return re.findall(
|
[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
|
|
|
fr'^{h}*[@|#]?{self.github_prefix}(?:{h}+|:{h}*)(.*)$',
|
2021-11-10 19:13:34 +07:00
|
|
|
comment, re.MULTILINE | re.IGNORECASE)
|
|
|
|
|
|
|
|
def _has_branch(self, name):
|
[MERGE] bot from 16.0 to 17.0
Broken (can't run odoo at all):
- In Odoo 17.0, the `pre_init_hook` takes an env, not a cursor, update
`_check_citext`.
- Odoo 17.0 rejects `@attrs` and doesn't say where they are or how to
update them, fun, hunt down `attrs={'invisible': ...` and try to fix
them.
- Odoo 17.0 warns on non-multi creates, update them, most were very
reasonable, one very wasn't.
Test failures:
- Odoo 17.0 deprecates `name_get` and doesn't use it as a *source*
anymore, replace overrides by overrides to `_compute_display_name`.
- Multiple tracking changes:
- `_track_set_author` takes a `Partner` not an id.
- `_message_compute_author` still requires overriding in order to
handle record creation, which in standard doesn't support author
overriding.
- `mail.tracking.value.field_type` has been removed, the field type
now needs to be retrieved from the `field_id`.
- Some tracking ordering have changed and require adjusting a few
tests.
Also added a few flushes before SQL queries which are not (obviously
at least) at the start of a cron or controller, no test failure
observed but better safe than sorry (probably).
2024-08-12 18:13:03 +07:00
|
|
|
self.env['runbot_merge.branch'].flush_model(['project_id', 'name'])
|
2021-11-10 19:13:34 +07:00
|
|
|
self.env.cr.execute("""
|
|
|
|
SELECT 1 FROM runbot_merge_branch
|
|
|
|
WHERE project_id = %s AND name = %s
|
|
|
|
LIMIT 1
|
|
|
|
""", (self.id, name))
|
|
|
|
return bool(self.env.cr.rowcount)
|
2021-11-12 22:04:34 +07:00
|
|
|
|
|
|
|
def _next_freeze(self):
|
|
|
|
prev = self.branch_ids[1:2].name
|
|
|
|
if not prev:
|
|
|
|
return None
|
|
|
|
|
|
|
|
m = re.search(r'(\d+)(?:\.(\d+))?$', prev)
|
|
|
|
if m:
|
|
|
|
return "%s.%d" % (m[1], (int(m[2] or 0) + 1))
|
|
|
|
else:
|
|
|
|
return f'post-{prev}'
|
|
|
|
|
|
|
|
def _compute_freeze(self):
|
|
|
|
freezes = {
|
|
|
|
f.project_id.id: f.id
|
|
|
|
for f in self.env['runbot_merge.project.freeze'].search([('project_id', 'in', self.ids)])
|
|
|
|
}
|
|
|
|
for project in self:
|
|
|
|
project.freeze_id = freezes.get(project.id) or False
|
|
|
|
|
|
|
|
def action_prepare_freeze(self):
|
|
|
|
""" Initialises the freeze wizard and returns the corresponding action.
|
|
|
|
"""
|
|
|
|
self.check_access_rights('write')
|
|
|
|
self.check_access_rule('write')
|
|
|
|
Freeze = self.env['runbot_merge.project.freeze'].sudo()
|
|
|
|
|
|
|
|
w = Freeze.search([('project_id', '=', self.id)]) or Freeze.create({
|
|
|
|
'project_id': self.id,
|
|
|
|
'branch_name': self._next_freeze(),
|
2022-02-07 21:15:13 +07:00
|
|
|
'release_pr_ids': [
|
|
|
|
(0, 0, {'repository_id': repo.id})
|
|
|
|
for repo in self.repo_ids
|
|
|
|
if repo.freeze
|
|
|
|
]
|
2021-11-12 22:04:34 +07:00
|
|
|
})
|
2022-02-07 19:23:41 +07:00
|
|
|
return w.action_open()
|
[CHG] *: move forward-porting over to batches
Thank god I have a bunch of tests because once again I forgot / missed
a bunch of edge cases in doing the conversion, which the tests
caught (sadly that means I almost certainly broke a few untested edge
cases).
Important notes:
Handling of parent links
------------------------
Unlike PRs, batches don't lose their parent info ever, the link is
permanent, which is convenient to trawl through a forward port
(currently implemented very inefficiently, maybe we'll optimise that
in the future).
However this means the batch having a parent and the batch's PRs
having parents are slightly different informations, one of the edge
cases I missed is that of conflicting PRs, which are deparented and
have to be merged by hand before being forward ported further, I had
originally replaced the checks on a pr and its sibling having parents
by just the batch.
Batches & targets
-----------------
Batches were originally concepted as being fixed to a target and PRs
having that target, a PR being retargeted would move it from one batch
to an other.
As it turns out this does not work in the case where people retarget
forward-port PRs, which I know they do because #551
(2337bd85186de000d074e5a7178cfeb110d15de7). I could not think of a
good way to handle this issue as is, so scrapped the moving PRs thing,
instead one of the coherence checks of a batch being ready is that all
its PRs have the same target, and a batch only has a target if all its
PRs have the same target.
It's possible for somewhat odd effects to arise, notably if a PR is
closed (removed from batch), the other PRs are retargeted, and the new
PR is reopened, it will now be on a separate batch even if it also
gets retargeted. This is weird. I don't quite know how I should handle
it, maybe batches could merge if they have the same target and label?
however batches don't currently have a label so...
Improve limits
--------------
Keep limits on the PRs rather than lift them on the batchL if we can
add/remove PRs of batches having different limits on different PRs of
the same batch is reasonable.
Also leave limit unset by default: previously, the limit was eagerly
set to the tip (accessible) branch. That doesn't really seem
necessary, so stop doing that.
Also remove completely unnecessary `max` when trying to find a PR's
next target: `root` is either `self` or `self.source_id`, so it should
not be possible for that to have a later target.
And for now ensure the limits are consistent per batch: a PR defaults
to the limit of their batch-mate if they don't have one, and if a
limit is set via command it's set on all PRs of a batch.
This commit does not allow differential limits via commands, they are
allowed via the backend but not really tested. The issue is mostly
that it's not clear what the UX should look like to have clear and not
super error prone interactions. So punt on it for now, and hopefully
there's no hole I missed which will create inconsistent batches.
2024-05-21 20:45:53 +07:00
|
|
|
|
|
|
|
def _forward_port_ordered(self, domain=()):
|
|
|
|
Branches = self.env['runbot_merge.branch']
|
|
|
|
return Branches.search(expression.AND([
|
|
|
|
[('project_id', '=', self.id)],
|
|
|
|
domain or [],
|
|
|
|
]), order=reverse_order(Branches._order))
|