Commit Graph

40 Commits

Author SHA1 Message Date
Xavier Morel
029957dbeb [IMP] *: trigger-ify task queue type crons
These are pretty simple to convert as they are straightforward: an
item is added  to a work queue (table), then a cron regularly scans
through the table executing the items and deleting them.

That means the cron trigger can just be added on `create` and things
should work out fine.

There's just two wrinkles in the port_forward cron:

- It can be requeued in the future, so needs a conditional trigger-ing
  in `write`.
- It is disabled during freeze (maybe something to change), as a
  result triggers don't enqueue at all, so we need to immediately
  trigger after freeze to force the cron re-enabling it.
2024-08-02 15:14:50 +02:00
Xavier Morel
32871c3896 [FIX] *: stray prints
Found a bunch of old leftover `print` calls which should not be in the
repo.
2024-08-02 08:59:45 +02:00
Xavier Morel
0206d5f977 [FIX] runbot_merge: followup detached PRs when disabling branches
Although the handling of forward ports on disabled branch was improved
in 94fe0329b4 in order to avoid losing
or needing to manually port such, because it goes through
`_schedule_fw_followup` some of the tests *that* performs were missed,
most notably that it only ports batches when no PRs are detached.

This is an issue if we need to force the port because of a branch
being deactivated: the forward-port could have stopped there due to a
conflict, in which case it's always going to be detached.

Thus the `force_fw` flag should also override the parenting state
check.

Also while at it make `force_fw` a regular flag, I don't understand
why I made it into a context value in the first place, it's only
passed from one location and that's directy calling the one function
which uses it...

Fixes #897
2024-06-28 16:06:20 +02:00
Xavier Morel
eb23a8c083 [REM] forwardport: useless test
`test_maintain_batch_history` was built for the original design where
PRs were removed from batches on being closed.

This decision was reverted in bbce5f8f46
as it proved an inferior and inconvenient design even in the face of
some of the edge cases, however I clearly forgot about this test.
2024-06-26 15:30:59 +02:00
Xavier Morel
2ab06ca96b [IMP] *: require explicitly specifying whether exceptions in logs are valid
Seems like a good idea to better keep track of the log of an Odoo used
to testing, and avoid silently ignoring logged errors.

- intercept odoo's stderr via a pipe, that way we can still write it
  back out and pytest is able to read & buffer it, pytest's capfd
  would not work correctly: it breaks output capturing (and printing
  on failure); and because of the way it hooks in it's unable to
  capture from subprocesses inheriting the standard stream, cf
  pytest-dev/pytest#4428
- update the env fixture to check that the odoo log doesn't have any
  exception on failure
- make that check conditional on the `expect_log_errors` marker, this
  way we can mark tests for which we expect errors to be logged, and
  assert that that does happen
2024-06-12 15:09:42 +02:00
Xavier Morel
c67325fdab [IMP] forwardport: don't ping *every* forwardport
98aaa910 updated the forwardport notifications system to notify on the
forward ports rather than the source, to try and mitigate or at least
shift some of the spam: spam the followers of the original
source (which might be many people) somewhat less, at the possible
cost of spamming the author and reviewer more because they get a
message per forgotten forward port.

This change aims to alleviate part of the latter, by only notifying on
PRs which actually need to be r+'d, and not notifying on those which
will implicitly "inherit" the reviews. This should cut down on
redundant notifications and let users focus on the important ones.
2024-06-10 18:47:49 +02:00
Xavier Morel
ceb2bd457e [IMP] forwardport: automatically approve freeze backfills
if the corresponding master PR was approved. That the backfill needs
approval can be considered a race condition: its dual is approved
and *might* have been merged before the freeze, but was not, so a
backfill was needed.

Copying over the approval is a convenience feature with pretty much no
actual risk I can think of.

Fixes #884
2024-06-07 15:02:28 +02:00
Xavier Morel
fec3d39d19 [ADD] *: per-repository webhook secret
Currently webhook secrets are configured per *project* which is an
issue both because different repositories may have different
administrators and thus creates safety concerns, and because multiple
repositories can feed into different projects (e.g. on mergebot,
odoo-dev/odoo is both an ancillary repository to the main RD project,
and the main repository to the minor / legacy master-wowl
project). This means it can be necessary to have multiple projects
share the same secret as well, this then mandates the secret for more
repositories per (1).

This is a pain in the ass, so just detach secrets from projects and
link them *only* to repositories, it's cleaner and easier to manage
and set up progressively.

This requires a lot of changes to the tests, as they all need to
correctly configure the signaling.

For `runbot_merge` there was *some* setup sharing already via the
module-level `repo` fixtures`, those were merged into a conftest-level
fixture which could handle the signaling setup. A few tests which
unnecessarily set up repositories ad-hoc were also moved to the
fixture. But for most of the ad-hoc setup in `runbot_merge`, as well
as `forwardport` where it's all ad-hoc, events sources setup was just
appended as is. This should probably be cleaned up at one point, with
the various requirements collected and organised into a small set of
fixtures doing the job more uniformly.

Fixes #887
2024-06-06 11:07:57 +02:00
Xavier Morel
bbce5f8f46 [IMP] *: don't remove PRs from batches on close
Initially wanted to skip this only for FW PRs, but after some thinking
I feel this info could still be valuable even for non-fw PRs which
were never merged in the first place.

Requires a few adjustments to not break *everything*: `batch.prs`
excludes closed PRs by default as most processes only expect to be
faced by a closed PR inside a batch, and we *especially* want to avoid
that before the batch is merged (as we'd risk staging a closed PR).

However since PRs don't get removed from batches anymore (and batches
don't get deleted when they have no PRs) we now may have a bunch of
batches whose PRs (usually a single one) are all closed, this has two
major side-effects:

- a new PR may get attached to an old batch full of closed PRs (as
  batches are filtered out on being *merged*), which is weird
- the eventual list of batches gets polluted with a bunch of
  irrelevant batches which are hard to filter out

The solution is to reintroduce an `active` field, as a stored compute
field based on the state of batch PRs. This way if all PRs of a batch
are closed it switches to inactive, and is automatically filtered out
by search which solves both issues.
2024-05-29 07:55:07 +02:00
Xavier Morel
6972749fee [IMP] forwardport: remove duplicate-ish make_basic in test_weird
The one from the utils can easily be adapted for the job, and
documented while at it, no need for an extra 50 lines or so of
complete duplicate.
2024-05-24 09:08:56 +02:00
Xavier Morel
94fe0329b4 [FIX] *: behaviour around branch deactivation & fw maintenance
Test and refine the handling of batch forward ports around branch
deactivation, especially with differential. Notably, fix an error in
the conversion of the FW process to batches: individual PR limit was
not correctly taken in account during forward port unless *all* PRs
were done, even though that is a primary motivation for the
change.

Partial forward porting should now work correctly, and the detection
and handling of differential next target should be better handled to
boot.

Significantly rework the interplay between batches and PRs being
closed in order to maintain sequencing / consistency of forward port
sequences: previously a batch would get deleted if all its PRs are
closed, but that is an issue when it is part of a forward port
sequence as we now lose information.

Instead, detach the PRs from the batch as before but have the batch
skip unlinking if it has historical value (parent or child
batch). Currently the batch's state is a bit weird as it doesn't get
merged, but...

While at it, significantly simplify `_try_closing` as it turns out to
have a ton of incidental / historical complexity from old attempts at
fixing concurrency issues, which should not be necessary anymore and
in fact actively interfere with the new and more compute-heavy state
of things.
2024-05-24 09:08:56 +02:00
Xavier Morel
9ddf017768 [CHG] *: move fw_policy from PR to batch 2024-05-23 07:58:58 +02:00
Xavier Morel
473f89f87d [CHG] *: persistent batches
This probably has latent bugs, and is only the start of the road to v2
(#789): PR batches are now created up-front (alongside the PR), with
PRs attached and detached as needed, hopefully such that things are
not broken (tests pass but...), this required a fair number of
ajustments to code not taking batches into account, or creating
batches on the fly.

`PullRequests.blocked` has also been updated to rely on the batch to
get its batch-mates, such that it can now be a stored field with the
right dependencies.

The next step is to better leverage this change:

- move cross-PR state up to the batch (e.g. skipchecks, priority, ...)
- add fw info to the batch, perform forward-ports batchwise in order
  to avoid redundant batch-selection work, and allow altering batches
  during fw (e.g. adding or removing PRs)
- use batches to select stagings
- maybe expose staging history of a batch?
2024-05-23 07:58:58 +02:00
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
Xavier Morel
955a61a1e8 [CHG] runbot_merge, forwardbot: merge commands parser
- move all commands parsing to runbot_merge as part of the long-term
  unification effort (#789)
- set up an actual parser-ish structure to parse the commands to
  something approaching a sum type (fixes #507)
- this is mostly prep for reworking the commands set (#673), although
  *strict command parsing* has been implemented (cf update to
  `test_unknown_commands`)
2024-05-16 10:37:50 +02:00
Xavier Morel
270dfdd495 [REF] *: move most feedback messages to pseudo-templates
Current system makes it hard to iterate feedback messages and make
them clearer, this should improve things a touch.

Use a bespoke model to avoid concerns with qweb rendering
complexity (we just want GFM output and should not need logic).

Also update fwbot test setup to always configure an fwbot name, in
order to avoid ping messages closing the PRs they're talking
about, that took a while to debug, and given the old message I assume
I'd already hit it and just been too lazy to fix. This requires
updating a bunch of tests as fwbot ping are sent *to*
`fp_github_name`, but sent *from* the reference user (because that's
the key we set).

Note: noupdate on CSV files doesn't seem to work anymore, which isn't
great. But instead set tracking on the template's templates, it's not
quite as good but should be sufficient.

Fixes #769
2023-06-14 16:01:45 +02:00
Xavier Morel
2009177ada [IMP] *: allow disabling staging on branch, remove fp target flag
- currently disabling staging only works globally, allow disabling on
  a single branch

  - use a toggle
  - remove a pair of tests which work specifically with `fp_target`,
    can't work with `active` (probably)
  - cleanup search of possible and active stagings, add relevant
    indexes and use direct search of relevant branches instead of
    looking up from the project

- also use toggle button for `active` on branches
- shitty workaround for upgrading DB: apparently mail really wants to
  have a `user_id` to do some weird thing, so need to re-add it after
  resetting everything

Fixes #727
2023-06-14 16:01:42 +02:00
Xavier Morel
8d7d6302d3 [FIX] runbot_merge: make freeze wizard labels lookup not shit
I DECLARE BANKRUPTCY!!!

The previous implementation of labels lookup was really not
intuitive (it was just a char field, and matched labels by equality
including the owner tag), and was also full of broken edge
cases (e.g. traceback if a label matched multiple PRs in the same repo
because people reuse branch names).

Tried messing about with contextual `display_name` and `name_search`
on PRs but the client goes wonky in that case, and there is no clean
autocomplete for non-relational fields.

So created a view which reifies labels, and that can be used as the
basis for our search. It doesn't have to be maintained by hand, can be
searched somewhat flexibly, we can add new view fields in the future
if desirable, and it seems to work fine providing a nice
understandable UX, with the reliability of using a normal Odoo model
the normal way.

Also fixed the handling of bump PRs, clearly clearing the entire field
before trying to update existing records (even with a link_to
inbetween) is not the web client's fancy, re-selecting the current
label would just empty the thing entirely.

So use a two-step process slightly closer to the release PRs instead:

- first update or delete the existing bump PRs
- then add the new ones

The second part is because bump PRs are somewhat less critical than
release, so it can be a bit more DWIM compared to the more deliberate
process of release PRs where first the list of repositories involved
has to be set up just so, then the PRs can be filled in each of them.

Fixes #697
2023-01-25 12:25:45 +01:00
Xavier Morel
1cea247e6c [FIX] runbot_merge: sync PR target and message on check
Previously the mergebot would only sync the head commit, but synching
more is useful.

Also update the final sanity check on staging:

- as with check, update the message & target branch
- reset PR state and post a message when updating message instead of
  doing so silently

Note: maybe only fail the staging if the message is updated *and*
relevant to staging (aka there's a merge method and it's not
`rebase`)?

Fixes #680
2023-01-25 12:25:45 +01:00
Xavier Morel
2742fe18fd [FIX] forwardport: correct batching of intermediate PRs
When inserting a new branch, if there are extant forward ports
sequences spanning over the insertion, new forward ports must be
created for the new branch.

This was the case, *however* one of the cases was handled incorrectly:
PR batches (PRs to different repositories staged together) must be
forward-ported as batches. However when creating the "insert" batches,
these PRs would be split apart, leading to either inconsistent states
or an inability to merge the new forward ports (if they are
co-dependent which is a common case). This is because the handler
would look for all the relevant PRs to forward ports, but it would
fail to group them by label thereafter.

Fix that, create the `insert` batches correctly, and augment the
relevant test with an additional repository to test this case.

No gh issue was created, but this problem was reported on
odoo/odoo#110029 and odoo/enterprise#35838 (they were re-linked by
hand in the runbot and mergebot, but on github the labels remain
visibly different, one having the slug ZCwE while the other
hasO7Pr).

It's possible that the problem had occurred previously and not been
noticed (or reported), though it's also possible this issue had never
occurred before: for the latest freeze (16.1) there were 18 forward
ports spanning the insertion, but of them only 2 were the same batch.
2023-01-19 10:57:09 +01:00
Xavier Morel
a563fcf907 [REM] forwardport: fp_sequence field
It's almost certainly not useful, save as a minor convenience for
tests: decorrelating the branch sequence and the fp sequence seems
like it would only be extremely confusing, and on the mergebot all the
fp_sequence values are set to the default while the sequence values
are set to something useful and sensible (kinda).

Fixes #584
2022-12-08 10:46:22 +01:00
Xavier Morel
7948e59c51 [FIX] *: fix forward port inserter if last branch is disabled
In case where the last branch (before the branch being frozen) is
disabled, the forwardport inserter screws up, and fails to correctly
create the intermediate forwardports from the new branch.

Also when disabling a branch, if there are FW PRs which target that
branch and have not been forward-ported further, automatically
forward-port them as if the branch had been disabled when they were
created, this should limit data loss and confusion.

Also change the message set on PRs when disabling a branch: because of
user conflicts in test setup, the message about a branch being
disabled would close the PRs, which would then orphan the followup,
leading to unexpected / inconsistent behaviour.

Fixes #665
2022-12-01 10:57:32 +01:00
Xavier Morel
b45ecf08f9 [IMP] forwardport: handling of missing magic refs
Github can fail to create the magic refs on PRs
(`pull/refs/?/head`). Since forwardport relies on these refs to fetch
PR content this is an issue when it occurs, as the forward ports fail
in a loop.

After discussion with Github support, it turns out Github enabled
`allowReachableSHA1InWant` a while back, meaning it's possible to
fetch content by commit (rather than ref) as long as the content is
"in network".

Use this property as fallback when checking if we can see the PR head
before forward porting.

Also:

- remove explicit configuration of GC during fetch, it doesn't disable
  the autogc (yet?) but that's likely going to happen anyway
- update logging and logger hierarchy during forward port to make
  things clearer and easier to extract, although based on PR id rather
  than number
- rate limit failing forward ports to avoid running them on every cron
  (~ every minute), run them every ~30mn instead, this provides higher
  odds of recovery with less log garbage in case of transient github
  failure, and if the PR is stuck it limits the log pollution

Fixes #658
2022-12-01 10:57:32 +01:00
Xavier Morel
e9278c021d [IMP] forwardport: commenting of test_author_can_close_via_fwbot 2022-08-05 15:35:51 +02:00
Xavier Morel
b86092de83 [IMP] *: freeze wizard v3, freezer and wizarder
Stop *staging* release PRs: they are normally fairly simple and should
not fail their staging outside of unreliable tests (or possibly a few
edge cases e.g. forgot one version change thing), however staging them
creates the possibility of a "version hole" on the release branch
which is undesirable.

Instead, immediately and unconditionally push the release commits onto
the newly created branches, if there are things which don't work they
can be fixed afterwards (and the process refined, maybe).

Also add the same feature for *bump* PRs, with the difference that the
bump PRs are not created / requested by default (they have to be opted
in individually).

For convenience, add a feature which automatically finds the PRs via
inputting the label (not really tested yet).

Closes #603
2022-08-05 15:35:51 +02:00
Xavier Morel
f430c014c1 [IMP] *: review mergebot & forwardbot messages for pinging
Old messages were quite inconsistent in their pinging of the PR author
and reviewer.

Reviewed messages (probably missed some but...) and try to more
consistently ping when the feedback requires some sort of action in
order to proceed.

Fixes #592
2022-06-30 15:07:49 +02:00
Xavier Morel
2337bd8518 [FIX] forwardport: chain crash on insert in forward-port cron
On two of the freezes, thereafter the logs showed serial crashes in
the forwardport cron when trying to find the insertion point for a new
forward-port.

The first time was not really diagnosed, the second time the cause was
found to be a retargeted PR which led to a failure of the "insertion"
forward port, which did not take that possibility in account (it
assumed -- sensibly I believed -- that an intermediate FP following a
branch insertion would always succeed, sadly the malevolent universe
had other plans).

So only insert the new forward port inside its sequence (if necessary)
if the forward port actually succeeded, otherwise ignore it.

Fixes #551
2022-02-10 13:51:33 +01:00
Xavier Morel
e05cc77a57 [FIX] forwardport: don't forwardport freeze PRs
The freeze wizard has support for merging freeze / release PRs on each
of the newly created branches. But since this would be done by, well,
merging, those PRs would get forward-ported to master, and would have
to be closed there.

This creates additional work for the freeze master, and noise /
parasitic PRs.

Obviously it's possible for the freeze master to set some nonsense `up
to` (nonsense because the "real" limit doesn't exist yet at that
point), but really it never makes any sense to forward port release
PRs, so the wizard should do it.
2022-02-08 10:11:57 +01:00
Xavier Morel
82174ae66e [IMP] *: add draft support to mergebot, kinda
* Remove the forwardport creating PRs in draft, that was mostly to
  avoid codeowners triggering but we've removed the github one and
  hand-rolled it, so not a concern anymore.
* Prevent merging `draft` PRs, the mergebot rejects approval on draft
  PRs and insults people.

TBD (maybe): try to create *conflicting* forward-port PRs in draft so
it's clearer they need to be *fixed*? Issue of not being able to do
that on all private repositories remains so~~

Fixes #500
2021-08-24 15:39:47 +02:00
Xavier Morel
30cfc4fe59 [IMP] runbot_merge: disable active test on batch fields
For post-mortem investigations and manipulations, they're inconvenient
as the batches are deactivated on success and failure.
2021-08-24 15:39:47 +02:00
Xavier Morel
e175609950 [IMP] forwardport: unmodified fw automatically inherit overrides
Before this change, a CI override would have to be replicated on most
/ all forward-ports of the base PR. This was intentional to see how it
would shake out, the answer being that it's rather annoying.

Also add a `statuses_full` computed field on PRs for the aggregate
status: the existing `statuses` field is just a copy of the commit
statuses which I didn't remember I kept free of the overrides so the
commit statuses could be displayed "as-is" in the backend (the
overrides are displayed separately). And while at it fix the PR
dashboard to use that new field: that was basically the intention but
then I went on to use the "wrong" field hence #433.

Mebbe the UI part should be displayed using a computed M2M (?)
as a table or as tags instead? This m2m could indicate whether the
status is an override or an "intrinsic" status.

Also removed some dead code:

* leftover from the removed tagging feature (removed the tag
  manipulation but forgot some of the setup / computations)
* unused local variables
* an empty skipped test case

Fixes #439.

Fixes #433.
2021-01-13 16:11:14 +01:00
Xavier Morel
5c19342bf6 [CHG] runbot_merge, forwardport: remove labelling
Because github materialises every labels change in the
timeline (interspersed with comments), the increasing labels churn
contributes to PRs being difficult to read and review.

This change removes the update of labels on PRs, instead the mergebot
will automatically send a comment to created PRs serving as a
notification that the PR was noticed & providing a link to the
mergebot's dashboard for that PR where users should be able to see the
PR state in detail in case they wonder what's what.

Lots of tests had to be edited to:

- remove any check on the labels of the PR
- add checks on the PR dashboard (to ensure that they're at least on
  the correct "view")
- add a helper to handle the comment now added to every PR by the 'bot
- since that helper is needed by both mergebot and forwardbot, the
  utils modules were unified and moved out of the odoo modules

Probably relevant note: no test was added for the dashboard
ACL, though since I had to explicitly unset the group on the repo used
for tests for things to work it looks to me like it at least excludes
people just fine.

Fixes #419
2020-11-20 07:41:54 +01:00
Xavier Morel
d99d1c2ad6 [ADD] forwardport: ability to skip CI when forward porting
Provides a `skipci` command to PR reviewers. This makes it so the
followup PRs (after the first one) get created immediately, without
waiting for CI to succeed on a given forward-port PR.

This can be useful if for some reason a change *must* be merged in
branch N+1 before it can be merged in branch N.

Fixes #363
2020-04-17 08:09:01 +02:00
Xavier Morel
aa2248e379 [IMP] forwardport: close command
This is useful as the author of the original PR doesn't necessarily
have (write) access to the repository where the forward-port PR was
created. As a result, while they can r+ the PR they're unable to close
it (via github's interface).

Since the forwardport bot created the PR, it can also close it, which
seems like a useful feature.

Closes #341
2020-03-04 11:56:01 +01:00
Xavier Morel
742e3219a6 [IMP] runbot_merge: make review rights repo-dependent
As the odds of having more projects or more repos with different
requirements in the same project, the need to have different sets of
reviewers for different repositories increases.

As a result, rather than be trivial boolean flags the review info
should probably depend on the user / partner and the repo. Turns out
the permission checks had already been extracted into their own
function so most of the mess comes from testing utilities which went
and configured their review rights as needed.

Incidentally it might be that the test suite could just use something
like a sequence of commoditized accounts which get configured as
needed and not even looked at unless they're used.
2020-02-11 08:07:57 +01:00
Xavier Morel
9aac1b4a3e [ADD] forwardport: special handling of adding branches to projects
If a new branch is added to a project, there's an issue with *ongoing*
forward ports (forward ports which were not merged before the branch
was forked from an existing one): the new branch gets "skipped" and
might be missing some fixes until those are noticed and backported.

This commit hooks into updating projects to try and see if the update
consists of adding a branch inside the sequence, in which case it
tries to find the FP sequences to update and queues up new
"intermediate" forward ports to insert into the existing sequences.

Note: had to increase the cron socket limit to 2mn as 1mn blew up the
big staging cron in the test (after all the forward-port PRs are
approved).

Fixes #262

[FIX]
2020-01-29 15:59:43 +01:00
Xavier Morel
60574d6fe2 [ADD] forwardport: support for branch filtering
Add handling of branch filtering to the forwardport module:

* don't forward port (and trigger an error) when trying to port
  PRs to different next targets
* otherwise port normally

e.g. given a project with repos A and B and branches a, b and c, with
branch b being excluded from repo B:

* a PR merged into A.a will be forward-ported to A.b and A.c
* a PR merged into B.a will be forward-ported to B.c (skipping the
  excluded B.b)
* a PR set merged into (A.a, B.a) will *not* be forward-ported, and a
  message will be posted to each PR denoting the incompatibility
2020-01-24 13:39:14 +01:00
Xavier Morel
7dfa973b57 [IMP] runbot_merge, forwardport: move required statuses to repository
Allows more flexibility in project composition as different
repositories can each have their own CI passes.
2020-01-24 13:39:14 +01:00
Xavier Morel
37cf6accb7 [FIX] forwardport: FP PR getting CI'd after initial FP check
Test is probably more complex than necessary (thinking about it, the
failed staging is probably unnecessary) but that triggers the issue
and matches the original scenario.

The problem was really with new CI events being received on the last
forward-port PR of a sequence: previous PRs would have a child PR so
the check would abort, but for the last PR it would go through, fail
to find an active batch, then blow up as it tries to create a
forwardport.batch without an actual batch.

Change this to use the existence of an inactive batch not linked to a
staging as a flag that the PR has been processed and forward-ported.

Closes #218
2019-10-01 20:51:31 +02:00
Xavier Morel
f671dcc828 [ADD] forwardbot
* Cherrypicking is handrolled because there seems to be no easy way to
  programmatically edit commit messages during the cherrypicking
  sequence: `-n` basically squashes all commits and `-e` invokes a
  subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it
  interrupts the process before each commit), however there doesn't
  seem to be clean status codes so it's difficult to know if the
  cherrypick failed or if it's just waiting for a commit of this step.

  Instead, cherrypick commits individually then edit / rewrite their
  commit messages:

  * add a reference to the original commit
  * convert signed-off-by to something else as the original commit was
    signed off but not necessarily this one

* Can't assign users when creating PRs: only repository collaborators
  or people who commented on the issue / PR (which we're in the
  process of creating) can be assigned.

  PR authors are as likely to be collaborators as not, and we can have
  non-collaborator reviewers. So pinging via a regular comment seems
  less fraught as a way to notify users.
2019-09-05 10:00:07 +02:00