Commit Graph

1872 Commits

Author SHA1 Message Date
Xavier Morel
94cf3e9647 [IMP] *: convert fw=no to a genuine forward-porting policy
After seeing it be used, I foresee confusion around the current
behaviour (where it sets the limit), as one would expect the `fw=`
flags to affect one another when it looks like that would make sense
e.g. no/default/skipci/skipmerge all specify how to forward port, so
`fw=default` not doing anything after you've said `fw=no` (possibly by
mistake) would be fucking weird.

Also since the author can set limits, allow them to reset the fw
policy to default (keep skipci for reviewers), and for @d-fence add a
`fw=disabled` alias.

Fixes #902
2024-06-28 16:06:20 +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
Christophe Monniez
5740c93e38 [FIX] runbot: add missing opcode
The MAKE_CELL opcode appeared in python 3.11 and is needed in some
python steps when using closures and generators.

Like:
`(all(s > e for e in [1,2]) for s in [0,1])`
2024-06-28 15:27:39 +02:00
Xavier Morel
318e55337c [FIX] forwardport: count next to users should be the fwport
Previously it would count the number of source PRs with outstanding
forward ports, which is not the count from the home page so that was
confusing.

Also add counts next to the groups, so teams can be identified at a
glance.

And finally outline the current user in the list, so they can find
themselves faster when they're not one of the top entries.
2024-06-28 08:18:34 +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
de32824a62 [IMP] *: move the page helper fixture to the shared conftest
Use it in `test_limit` instead of direct `requests` calls.
2024-06-26 15:17:09 +02:00
Xavier Morel
1c76a675c2 [IMP] runbot_merge: cancel splits on cancel=staging
If a PR is cancel=staging, even if it's not the
urgentest (priority=alone) odds are good it's being staged to fix the
split. And even if it's not, it probably can't hurt.

So cancel splits in order to stage it. This may be slightly harmful if
the split is legit and has nothing to do with the PR being
prioritised, but that seems like the less likely scenario. And having
to update staging priorities on the fly seems like a bad idea. Though
obviously it might do nothing if the PRs are in "default" priority.#

Also simplify the unstage trigger from the PRs becoming ready:

- the user is useless as it's always the system user
- the batch id is not really helpful
2024-06-26 14:30:31 +02:00
Xavier Morel
3bc5b4e3e4 [CHG] runbot_merge: log ping instead of printing it
That's an old and completely useless leftover, but I never got around
to swapping it. It could be removed entirely or moved to debug as
well...
2024-06-25 15:54:31 +02:00
Xavier Morel
6ada35a200 [CHG] runbot_merge: ignore long comments
Comments which are too long cause `logging` itself to crash, which
kinda sucks. And long comments seem very unlikely to have anything for
the mergebot to do besides.

So just ignore them at intake. Limit is set to 5000 because there
needs to be a limit somewhere and that's about the extent of it.
2024-06-25 15:54:31 +02:00
Xavier Morel
286c1fdaee [FIX] runbot_merge: allow source author to r- forward ports
Noticed that while writing up the docs on the wiki, seems like an
unnecessary restriction, and an inconvenient one to boot: the author
could r+, then realize they forgot to do an update they needed to do
on the fw, so they should be able to cancel the staging without
needing a reviewer.
2024-06-25 15:54:31 +02:00
Xavier Morel
0a839a4857 [FIX] forwardport: don't break forward porting on huge conflicts
On forward-porting, odoo/odoo#170183 generates a conflict on pretty
much every one of the 1111 files it touches, because they are
modify/delete conflicts that generates a conflict message over 200
bytes per file, which is over 200kB of output.

For this specific scenario, the commit message was being passed
through arguments to the `git` command, resulting in a command line
exceeding `MAX_ARG_STRLEN`[^1]. The obvious way to fix this is to pass
the commit message via stdin as is done literally in the line above
where we just copy a non-generated commit message.

However I don't think hundreds of kbytes worth of stdout[^2] is of any
use, so shorten that a bit, and stderr while at it.

Don't touch the commit message size for now, possibly forever, but
note that some log diving reveals a commit with a legit 18kB message
(odoo/odoo@42a3b704f7) so if we want to
restrict that the limit should be at least 32k, and possibly 64. But
it might be a good idea to make that limit part of the ready / merge
checks too, rather than cut things off or trigger errors during
staging.

Fixes #900

[^1]: Most resources on "Argument list too long" reference `ARG_MAX`,
    but on both my machine and the server it is 2097152 (25% of the
    default stack), which is ~10x larger than the commit message we
    tried to generate. The actual limit is `MAX_ARG_STRLEN` which
    can't be queried directly but is essentially hard-coded to
    PAGE_SIZE * 32 = 128kiB, which tracks.

[^2]: Somewhat unexpectedly, that's where `git-cherry-pick` sends the
    conflict info.
2024-06-25 15:54:23 +02:00
Xavier Morel
dc90a207d6 [ADD] runbot_merge: help command, and help on error
Fixes #898
2024-06-24 22:16:43 +02:00
Xavier Morel
f3a0a5c27c [FIX] runbot_merge: tracking message author on PullRequest events
d4fa1fd353 added tracking to changes
from *comments* (as well as a few hacks around authorship transfer),
however it missed two things:

First, it set the `change-author` during comments handling only, so
changes from the `PullRequest` hook e.g. open, synchronise, close,
edit, don't get attributed to their actual source, and instead just
fall back to uid(1). This is easy enough to fix as the `sender` is
always provided, that can be resolved to a partner which is then set
as the author of whatever changes happen.

Second, I actually missed one of the message hooks: there's both
`_message_log` and `_message_log_batch` and they don't call one
another, so both have to be overridden in order for tracking to be
consistent. In this case, specifically, the *creation* of a tracked
object goes through `_message_log_batch` (since that's a very generic
message and so works on every tracked object created during the
transaction... even though batch has a message per record anyway...)
while *updates* go through `_message_log`.

Fixes #895
2024-06-21 16:33:44 +02:00
Xavier Morel
b109225f44 [IMP] runbot_merge: quality of feedback on errorneous commands
- When a redundant approval is sent to a PR, notify but don't ignore
  the entire command set, there's no actual risk.
- Indicate that the entire comment was ignored when finding something
  which does not parse.

Fixes #892, fixes #893
2024-06-21 15:38:54 +02:00
Xavier Morel
7cd9afe7f2 [IMP] runbot_merge: trigger commits cron
The commit cron needs to be triggered any time we:

- create a new commit
- update a commit to set its `to_check`

So do that in create and write as well as the SQL query in the
webhook handler.

This should mean we don't need the periodic cron anymore, but for
safety's sake run it on 30mn for now.

TBF even if we miss triggers, the next `status` webhook hitting will
check all the relevant commits anyway...
2024-06-21 11:02:50 +02:00
Xavier Morel
92e8eecbb5 [FIX] runbot_merge: ability to create PRs via the UI
This is useful to repro issues.

60c4b5141d added `inverse=readonly`
hooks to various newly computed fields to ensure they can not be *written*
to, either overwriting the content (stored) or silently being
dropped (non-stored).

However because they're `inverse` hooks this had the effect of making
them writeable from the backend UI since the ORM uses `inverse` as a
signal to make the field writeable. This then caused the web client to
send stuff for those fields, which are not necessarily even visible in
the form, leading to write errors when trying to save a PR creation.

By marking the fields as `readonly` explicitly we make sure that
doesn't happen, and we can create PRs from the backend UI (kinda, I
think the label is still an issue).
2024-06-21 10:42:37 +02:00
Xavier Morel
906505ed15 [IMP] runbot_merge: filter on the base attribute not computed
Should not actually do anything relevant, but seems like a good idea.
2024-06-21 10:42:08 +02:00
Xavier Morel
3410f50248 [FIX] runbot_merge: Commit.create
The method was not marked as a create, following which it did not
allow creating commits via the UI (annoying for testing / reproducing
issues involving statuses).
2024-06-21 10:41:01 +02:00
Xavier Morel
737cbd5de2 [IMP] *: merge fw overrides into their parent
Not actually useful in any way, but it does remove a few lines, avoids
a few dupe writes, and furthers the cause of #789
2024-06-21 10:40:06 +02:00
Xavier Morel
f303674434 [FIX] *: re-enable notification on status failure
If a PR gets approved *then* fails CI, there should be a notification
warning the author & reviewer since
48e08b657b, it even has a test, which
passes (in fact it has *two*, one of which is redundant, so merge
`test_ci_failure_after_review` into the later `test_ci_approved`).

*However* this is in runbot_merge, turns out in
fafa7ef437 some refactoring was done in
order to override the notification and customise it for *forward
ports* with a failed status... except that override never called its
`super()`, so as soon as forwardport is installed the base
notification stops working, and that's been that since October
2019 (had been added in March that year, ignoring deployment lag).

This can be revealed by adding the corresponding check in the
*forwardport* tests, revealing the failure.

This was a pain to track down, thankfully it reproduced relatively
easily locally.

While this could be resolved in the override, might as well fold it
into the base method in furtherance of #789: the mergebot is only
used by odoo, and only with both modules combined, so splitting them
is not useful. And furthermore it things should work fine with the
forwardport installed but unused.

Fixes #894
2024-06-21 10:27:01 +02:00
Xavier Morel
4a521e1251 [IMP] runbot_merge: hide backend links from group_user
The backend links in the PR dashboard were gated behind the
`group_user` (internal user) group, however turns out while internal
users have read access to PRs they don't have access to ancillary
objects (e.g. batches, stagings, the link between stagings and
batches), and I think the only way to fix the issue would be to move
it to an optional inheritance (inheritance + group), because `groups`
on view nodes only hides the content without removing it.

I believe in more recent Odoo versions this actually works correctly,
so that might actually be more of an incentive to upgrade...
2024-06-20 14:21:40 +02:00
Xavier Morel
20d259aa77 [IMP] runbot_merge: always display PR title
Previous version would always hide the title if the PR was
blocked (e.g. blocked or failed), turns out there are people who
actually use the PR title on the main dashboard, so suppressing that
is inconvenient for them.

Try to show the PR title if available, and add the blocked message if
present.
2024-06-20 13:49:17 +02:00
Xavier-Do
c562dac84d [FIX] higher sleep values 2024-06-17 12:52:58 +02:00
Xavier Morel
728524db12 [IMP] runbot_merge: send merge method warning faster, and on review
- Instead of warning about the merge method on ready PRs, also warn on
  *approved* (but exclude staged just cuz), as that's really when the
  user wants to know that they forgot to set the merge method
- The cron only triggers hourly, but *if* a user approves a PR *and*
  the merge method is not set yet, chances are good they'll need a
  reminder (if they `r+ rebase-merge` or w/e the cron will just ignore
  the PR and it's no skin off our back), so `_trigger` the cron for
  validation.
- Also do the same when skipchecks is set as it's very similar.

In reality we might want to hook off of the state transitioning to
reviewed but I'm not sure there's good ways to do that (triggering a
cron inside a compute doesn't seem like a good idea).

Update a pair of tests which would approve a multi-commit PR without
setting a merge method, just because the helper they use to build the
PR happens to create multiple commits.

Fix #891
2024-06-13 13:36:34 +02:00
Xavier Morel
9d9cae1d57 [FIX] runbot_merge: access to self in loop
This is a low issue as the prs of a commit are only listed from the
form so the compute is pretty much always called with a single record,
but still an unforced error which can easily be fixed.
2024-06-13 09:35:29 +02:00
Xavier Morel
2662411b96 [FIX] runbot_merge: _schedule_fp_followup not handling multiple batches
`_schedule_fp_followup` correctly iterates on `self`, however some of
the per-iteration work did not handle that correctly, and would try to
access fields on `self`.

Thankfully in most cases it only works on one batch at a time
anyway, *however* if multiple PRs share a HEAD (which is weird but...)
then `_validate` is called on multiple PRs, which through the
forwardport override leads to `_schedule_fp_followup` being called on
multiple batches, and failing when trying to access the `fw_policy`.

Fix by avoiding the misuse of `self` in the two locations where it's
doing something other than accessing `env`.
2024-06-13 08:04:12 +02:00
Xavier Morel
7711d09854 [IMP] *: add fw=no, deprecate ignore
Without fw-bot being its bearer, "ignore" is a lot less clear than it
used to as it looks to be asking to ignore the PR entirely (as if it
was targeted to an unmanaged branch).

Deprecate this command, and tack on the shortcut to the fw
subcommand. It is slightly sub-par as technically it does not quite
fit with the other subcommands, and furthermore can't be disabled via
fw=default... although maybe it could be? Maybe instead of setting the
limit fw=no could set that value to the forwardport mode, and the
fw_policy users could check that? It would require some more finessing
tho:

- `DEFAULT` would need to be accessible to the author as well as the
  reviewers so the author could toggle between `NO` and `DEFAULT`.
- There should probably be a warning of some sort when setting a limit
  to an unportable PR.
- The dashboards would need to take `NO` in account (though I guess
  that's just defaulting the limit to the target).
2024-06-12 16:08:25 +02:00
Xavier Morel
413027ad5b [IMP] runbot_merge: formatting & langage of PR attributes
Replace the unclear "unchecked" and "unreviewed" by "missing statuses"
and "missing r+", which are hopefully clearer as they better match
other lingo.

Also increase font for attributes, as size 10 was a bit small.

And finally add staging state to caching key, to differentiate "ready"
from "staged" pictures in gh's cache. "ready" should not be necessary
as it ought be implied by the label.
2024-06-12 15:51:17 +02:00
Xavier Morel
a2d7180216 [IMP] runbot_merge: move limit to fwport tab
And filter it to only consider branches in the same project as the PR,
and a lower sequence than its target. That way it's harder to fuck up
when trying to set limits from the backend.
2024-06-12 15:34:39 +02:00
Xavier Morel
d010f0374a [FIX] *: dashboard when PRs have different limits
The code selecting the lower and upper bounds for the PR dashboard did
not deal correctly with getting multiple limits in the same genealogy.
2024-06-12 15:09:47 +02:00
Xavier Morel
81f133de15 [IMP] test utils: don't ACL-protect repos by default
This is quite frustrating when trying to access the frontend from a
test especially combined with the lack of in-log feedback (before the
previous commit).
2024-06-12 15:09:42 +02:00
Xavier Morel
d2e730c39b [IMP] runbot_merge: log ACL error in PR controller
Currently this just silently returns a 404. Since repos are gated by
default (only accessible to internal users) this can get very
confusing when trying to setup a new repo or when forgetting this
information when writing tests.
2024-06-12 15:09:42 +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
60c4b5141d [FIX] runbot_merge: leftover direct setting of PR state
Setting the PR state directly really doesn't work as it doesn't
correctly save (and can get overwritten by any dependency of which
there are many).

This caused setting odoo/odoo#165777 in error to fail, leading to it
being re-staged (and failing) repeatedly, and the PR being spammed
with comments.

- create a more formal helper for preventing directly setting computed
  functions (without an actual inverse)
- replace direct state setting by setting the corresponding dependency
  e.g. `error` for error and `skipchecks` to force a PR to ready
- add a `skipchecks` inverse to the PR so it can also set itself as
  reviewed, and is convenient, might be worth also adding stuff to
  `Batch.write`
2024-06-11 15:41:20 +02:00
Xavier Morel
e320de0439 [FIX] runbot_merge: handle gh comments ending with newlines
Regex `$` apparently does not quite strip that out.
2024-06-11 15:24:09 +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
187f7f6429 [CHG] runbot_merge: allow pr author to approve all fw
- trigger FW section on all forward ports, not just attached ones
- allow author of original PR to approve any fwport
2024-06-10 15:21:24 +02:00
Xavier Morel
e403593799 [FIX] runbot_merge: incorrect computation dependencies
`Batch.staging_ids` is a computed field, it can't be used as a
dependency for an other compute (at least not in 15.0).
2024-06-10 14:31:02 +02:00
Xavier Morel
14a2b0068d [FIX] runbot_merge: type error in conflict handling 2024-06-10 14:29:55 +02:00
Xavier Morel
4af515b20d [IMP] runbot_merge: stagings button wrapping
Because one of the previous commits adds the duration of the staging
to the staging dropdown toggles, it's now much longer, and by default
the text does not wrap so it looks like shit and goes completely out
the column "CSS is awesome" style.

Update the style of the dropdown toggles specifically to allow text
wrapping. Also align them left instead of centering, because the text
makes a centered layout super ugly.
2024-06-07 17:07:10 +02:00
Xavier Morel
2fb85c515e [ADD] runbot_merge: missing staging migration
44084e303c changed the interpretation
and schema of the `statuses_cache` field on stagings, but I forgot to
add a migration, so it would just blow up on opening the home
dashboard or the staging lists.
2024-06-07 17:05:58 +02:00
Xavier Morel
f4035932e3 [IMP] runbot_merge: add staging status to dashboard
The dashboard can be a bit unclear as to the state of a PR when
everything's gone well. Make it more clear / explicit that it's ready
or staged.

Fixes #888
2024-06-07 15:51:26 +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
cc9f239261 [IMP] forwardport: use ORT and zdiff3 for cherrypicks
- zdiff3 should provide better conflict annotations than diff3
- ORT might also provide less conflicts period
- always perform cherrypicks without rename limit, since we're
  re-trying every failure without rename limit, it seems like
  unnecessary work, assuming git only ever looks as far as it needs
- also enable copy support maybe...

Fixes #827
2024-06-04 14:21:15 +02:00
Xavier Morel
c1e2e5a2e0 [REF] forwardport: update re_matches to not use a regex
Using a regex as the pattern is quite frustrating due to all the
escaping necessary, which in this refactoring I found out I'd missed,
multiple times.

Convert the pattern to something bespoke but not too complicated, we
may want to add anchoring support and a bit more finesse and the
future but for now straightforward "holes" seem to work well. I've
added support for capturing and even named groups even if this as yet
unnecessary and unused.

Fixes #861

[^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 14:18:04 +02:00
Xavier-Do
6fca88afa8 [IMP] allow to add single version in foreign trigger dependencies 2024-06-04 13:31:06 +02:00
Xavier Morel
98aaa9107f [CHG] forwardport: notify the outstanding forwardports rather than source
I have been convinced that this might be an improvement to the affairs
of the people: originally the message was sent to the source PR so we
wouldn't have to ping the author & reviewer and to limit the amount of
spam, *however*:

- we ended up adding pings anyway
- it also pings the followers of the source PR
- it increases the size of the original discussion (especially if was
- originally long)
- it adds steps to fixing the issue as you need to bounce from the
  source to the forward ports

Note that this might still notify a lot of people as they might be
made followers of the forward ports automatically, and it increases
the messaging load of the forwardbot significantly. But we'll see how
things go. Worst case scenario, we can revert it back.

Fixes #836
2024-06-04 08:56:51 +02:00
Xavier Morel
9c51f87aed [ADD] runbot_merge: support for non-webhook staging validation
Add support for the ability to validate *stagings* over RPC rather
than via webhook. This may later be expanded to PRs as well.

The core motivation for this is to avoid bouncing through github which
sometimes drops the ball on statuses, and it's frustrating to have a
staging time out because GH fucked up.

Implemented via RPC, requiring both the staging itself (by id) and the
head commit being affected, as that is necessary to know what CIs are
required for that head and correctly report cross branch on the
various PRs.

Fix #881 (kinda)
2024-06-04 08:56:51 +02:00
Christophe Monniez
eded56a4ef [FIX] runbot: adapt docker template for debian control
The Debian control file was changed in odoo/odoo@55849aca in order to
work with Ubuntu Noble. Because of that, it was needed to have a more
robust parsing of the Debian Control file format.
2024-06-04 08:53:18 +02:00