Commit Graph

1833 Commits

Author SHA1 Message Date
Xavier Morel
a5c514ad6e [FIX] *bot: copy filestore (shared dir) when copying db
Previously we would copy *only* the database itself, which is mostly
not an issue, but when trying to debug *pages* using a browser, that
browser tries to load various assets, which are looked up in the
database, then in the filestore, where they are missing.

This is not an issue in and of itself, but it triggers ugly tracebacks
in the logs which is not convenient, and since
2ab06ca96b it fails the test even if the
test would otherwise work.
2024-07-23 13:00:19 +02:00
Xavier Morel
82ec48c8da [IMP] runbot_merge: track PR label
It's not modified super often, but seems important to track if it
happens to be modified.
2024-07-23 13:00:19 +02:00
Xavier Morel
4a40c0338c [ADD] runbot_merge: small wizard to split a PR off of its batch 2024-07-23 13:00:19 +02:00
Xavier Morel
e6a743bdc2 [FIX] runbot_merge: the batch target should prioritise open PRs
From the previous version of `_compute_target` this was clearly
intended otherwise the fallback makes no sense, but just as clearly I
completely missed / forgot about it halfway through (and the lack of
test didn't help).

The compute is also way overcomplicated, it's not clear why (the only
explanation I can think of is that an intermediate version had a
string target but if that ever happened it was squashed away).
2024-07-23 13:00:19 +02:00
Xavier Morel
47df8fac84 [FIX] runbot_merge: bg-unmerged color
This color was altered in 232aa271b0, it
was moved from a cyan-ish green to a yellow quite close to the warning
color.

There is no explanation why, the commit concerns itself with *PR*
dashboards but this class / color is only used on the main
dashboard. It may have been a victim of the color refactoring in that
commit and I fucked up.

This is very disagreeable as it shows up as basically a warning
between the end of staging and it actually getting merged. Rollback
this change back to a green-cyan.
2024-07-15 10:32:25 +02:00
Xavier Morel
d6bb18e358 [ADD] runbot_merge: rendering of PR descriptions
Previously PR descriptions were displayed as raw text in the PR
dashboard. While not wrong per se, this was pretty ugly and not always
convenient as e.g. links had to be copied by hand.

Push descriptions through pymarkdown for rendering them, with a few
customisations:

- Enabled footnotes & tables & fenced code blocks because GFM has
  that, this doesn't quite put pymarkdown's base behaviour on par with
  gfm (and py-gfm ultimately gave up on that effort moving to just
  wrap github's own markdown renderer instead).
- Don't allow raw html because too much of a hassle to do it
  correctly, and very few people ever do it (mostly me I think).
- Added a bespoke handler / renderer for github-style references.

  Note: uses positional captures because it started that way and named
  captures are not removed from that sequence so mixing and matching
  is not very useful, plus python does not support identically named
  groups (even exclusive) so all 4 repo captures and all 3 issue
  number captures would need different names...
- And added a second bespoke handler for our own opw/issue references
  leading to odoo.com, that's something we can't do via github[^1] so
  it's a genuine value-add.

Fixes #889

[^1]: github can do it (though possibly not with the arbitrary
    unspecified nonsense I got when I tried to list some of the
    reference styles, some folks need therapy), but it's not available
    on our plan
2024-07-15 10:28:28 +02:00
Xavier Morel
02013a53d9 [IMP] runbot_merge: message when approving a PR in error
I thought I'd removed the error message when approving an already
approved PR but apparently not?

However we can improve the message in that specific case, to make the
expected operation clearer.

Fixes #906
2024-07-09 15:18:48 +02:00
Xavier-Do
f66f768024 [FIX] runbot: patch unidiff 0.7.3
There is an issue in unidiff 0.7.3 fixed in 0.7.4
a3faffc54e

This version is not able to parse a diff with removed files.

Since the unidiff packaged version in noble is 0.7.3 patching it looks
like the easiest solution
2024-07-09 08:21:55 +02:00
Xavier Morel
b1d3278de1 [CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.

The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.

This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.

Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.

A few things have been changed by this:

- The old trace/stderr from cherrypick has disappeared as it's
  generated by cherrypick, but for a non-interactive use it's kinda
  useless anyway so I probably should have looked into removing it
  earlier (I think the main use was investigation of the inflateinit
  issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
  couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
  which makes sense, but requires updating a bunch of tests. Then
  again so does the fact that it *usually* doesn't send anything to
  stderr, so that's usually disappearing.

Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.

Fixes #847

[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-08 14:37:14 +02:00
Xavier Morel
3062f30245 [IMP] runbot_merge: pass commit-tree message via stdin
Automating via parameters is riskier as we can hit the CLI
limitations (cf 0a839a4857). Going
through stdin is a lot safer and cleaner when automating, and it's not
much of an imposition here.
2024-07-05 13:29:20 +02:00
Christophe Monniez
3a4d432de9 [FIX] runbot: add another missing opcode
The COPY_FREE_VARS code was also added in python 3.11 and is needed by
some python steps.
2024-06-28 23:08:09 +02:00
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