Commit Graph

1937 Commits

Author SHA1 Message Date
William Braeckman
2fec54838e [IMP] runbot: add customer is me filter
As we try to assign ourselves as customer to build errors it is useful
to add a new filter to find errors on which we are the customer more
easily.
2024-10-22 09:25:53 +02:00
Xavier Morel
640392dc20 [FIX] significantly speed up local testing
The mergebot tests have always been pretty gentle on system load which
is nice, however it's just looking at the list of longest tests that I
realised / re-membered the hook wait duration is 10 seconds for the
benefit of github, which doesn't really matter locally. This means on
interaction / cron-heavy tests the test might only be using on the
order of 10% CPU or something, that is a waste of time.

TBF this is easily compensated by increasing the concurrency of the
test suite (e.g. from 16 to 32 when I switched machine, but it seems
as if not more sensible to lower the webhook wait delay to something
more reasonable. 1s seems to be a good fit here, on my new computer at
n=16 it leads to the test suite running in 15mn at 600% CPU (which is
pretty good on a 6/12 CPU as it loads the system heavily but doesn't
completely bog it down).

Reducing it to 0.5s, the test suite takes the same duration but CPU
load increases to 770%, and errors creep up, likely a mix of
concurrency issues in the DB and dummy-central sending webhooks too
slowly as we compete with it for CPU resources (could actually make
sense to restrict the number of threads tokio can use). Reducing
concurrency could make this work better, but I think at this point
we're in a pretty good state, it's even somewhat reasonable to run the
test suite sequentially (taking about 1h10 but being functionally
invisible in terms of load).
2024-10-18 10:19:28 +02:00
Xavier Morel
5748c086e5 [FIX] runbot_merge: status of closed PRs in extant batches
If a PR is closed but part of an ongoing batch, the change in status
of the batch might be reflected on the PR still:

- if a PR is closed and the batch gets staged, the PR shows up as
  being staged
- if the PR is merged then the batch gets merged, the PR shows up as
  merged

Fixes #914

Also remove the unused `_tagstate` helper property.
2024-10-16 12:17:30 +02:00
Christophe Monniez
dda0170a89 [FIX] runbot: avoid trying to push unexisting image
When an image is build for the first time and the build fails, an
ImageNotfound Exception is raised when trying to push the image.

Whith this commit, a warning is emmited instead of crashing in such a
case.
2024-10-10 09:16:56 +02:00
Christophe Monniez
a8b3d6e205 [FIX] runbot: avoid copying always_pull field
This commit will prevent people acting like cowboys to break the whole
system by duplicating a Dockerfile without disabling the always_pull
field.
2024-10-10 07:41:04 +02:00
Xavier-Do
ad87096436 [IMP] runbot: allow to skip requirements from trigger 2024-10-08 10:47:48 +02:00
Xavier Morel
20a4e97b05 [CHG] runbot_merge: make merge method non-blocking
Because of the false negatives due to github's reordering of events on
retargeting, blocking merge methods can be rather frustrating or the
user as what's happening and how to solve it isn't clear in that case.

Keep the warnings and all, but remove the blocking factor: if a PR
doesn't have a merge method and is not single-commit, just skip it on
staging. This way, PRs which are actually single-commit will stage
fine even if the mergebot thinks they shouldn't be.

Fixes #957
2024-10-07 08:07:59 +02:00
Xavier Morel
4215be770d [IMP] runbot_merge: move read_tracking_value to utils
And use it in test_trivial_flow instead of the kinda half-assed manual
version.
2024-10-07 08:06:03 +02:00
Xavier Morel
a45db1e089 [IMP] conftest: support for a more generic current_date 2024-10-07 08:04:25 +02:00
Xavier Morel
3a8b4684da [IMP] conftest: support for mapped(fn) 2024-10-07 08:03:36 +02:00
Xavier Morel
6a1b77b92c [ADD] runbot_merge: support for unstaged patches
Unstaged changes can be useful or necessary for some tasks
e.g. absolute emergency (where even faking the state of a staging is
not really desirable, if that's even possible anymore), or changes
which are so broad they're difficult to stage (e.g. t10s updates).

Add a new object which serves as a queue for patch to direct-apply,
with support for either text patches (udiff style out of git show or
format-patch) or commits to cherry-pick. In the former case, the part
of the show / format-patch before the diff itself is used for the
commit metadata (author, committer, dates, message) whereas for the
commit version the commit itself is reused as-is.

Applied patches are simply disabled for traceability.

Fixes #926
2024-10-03 12:06:00 +02:00
Xavier Morel
aac987f2bb [FIX] runbot_merge: dashboard display nits
- fix staging reasons containing escaped quotes (would render as
  ` ` to the end user)
- remove extra spacing around PR title @title/popovers
- simplify a few view conditionals through judicious use of `t-elif`
  and nesting
- make `staging_end` non-computed as it's not computed anymore, just
  set if and when the staging gets disabled
  (146564a90a)
2024-09-27 14:13:43 +02:00
Xavier Morel
430ccab2cb [IMP] runbot_merge: suppress view validation warning
This is a dumb false positive, kill it.
2024-09-27 12:53:51 +02:00
Xavier Morel
8f27773f8d [IMP] forwardport: surfacing of modify/delete conflicts
Given branch A, and branch B forked from it. If B removes a file which
a PR to A later modifies, on forward port Git generates a
modify/delete conflict (as in one side modifies a file which the
other deleted).

So far so good, except while it does notify the caller of the issue
the modified file is just dumped as-is into the working copy (or
commit), which essentially resurrects it.

This is an issue, *especially* as the file is already part of a
commit (rather tan just a U local file), if the developer fixes the
conflict "in place" rather than re-doing the forward-port from scratch
it's easy to miss the reincarnated file (and possibly the changes that
were part of it too), which at best leaves parasitic dead code in the
working copy. There is also no easy way for the runbot to check it as
adding unimported standalone files while rare is not unknown
e.g. utility scripts (to say nothing of JS where we can't really track
the usages / imports at all).

To resolve this issue, during conflict generation post-process
modify/delete to insert artificial conflict markers, the file should
be syntactically invalid so linters / checkers should complain, and
the minimal check has a step looking for conflict markers which should
fire and prevent merging the PR.

Fixes #896
2024-09-27 12:37:49 +02:00
Xavier Morel
ef22529620 [ADD] support for recursive tree to the test GH proxy 2024-09-27 12:36:50 +02:00
Xavier Morel
26882c42aa [FIX] warning in test logs 2024-09-27 12:36:02 +02:00
Xavier-Do
f8f435d468 [FIX] runbot: remove record iteration in onchange
A recent task introduced a record loop using self inside.

It doesn't respect an api multi classic loop but it isn't an issue
since onchange are always called on a single record.

Removing the loop on all onchange to clean it up.
2024-09-25 10:37:09 +02:00
Xavier Morel
98868b5200 [IMP] runbot_merge: add notifications on inactive branch interactions
Add warnings when trying to send comments / commands to PRs targeting
inactive branches.

This was missing leading to confusion, as one warning is clearly not
enough.

Fixes #941
2024-09-24 10:22:07 +02:00
Xavier Morel
e309e1a3a2 [FIX] runbot_merge: don't over-bump timeout
By updating the staging timeout every time we run `_compute_state` and
still have a `pending` status, we'd actually bump the timeout *on
every success CI* except for the last one. Which was never the
intention and can add an hour or two to the mergebot-side timeout.

Instead, add an `updated_at` attribute to statuses (name taken from
the webhook payload even though we don't use that), read it when we
see `pending` required statuses, and update the staging timeout based
on that if necessary.

That way as long as we don't get *new* pending statuses, the timeout
doesn't get updated.

Fixes #952
2024-09-20 12:17:17 +02:00
Xavier Morel
154e610bbc [IMP] *: modernize TestPRUpdate
- Switch to just `default` ci.
- Autouse fixture to init the master branch.
- Switch to `make_commits`.
- Merge `test_reopen_update` and `test_update_closed_revalidate` into
  `test_update_closed`: the former did basically nothing useful and
  the latter could easily be folded into `test_update_closed` by just
  validating the new commit.
2024-09-19 12:17:59 +02:00
odoo
0b5d7d0566 [IMP] add customer on runbot build tasks
Idea is to help runbot/QA team to monitor other peoples tasks
easier. This way we will have a responsible person for each task
that someone else has assigned.
The goal of pr is two-fold:
1) We need to be able to better monitor the progress of tasks that
are not our own.
2) It introduces another metric with which we can measure individual
progress, and this can become main measuring metric for future non-technical
employees whose main goal will be making sure that none of the tasks become
rotten(not have any visible progress for months).
2024-09-19 08:20:14 +02:00
Xavier Morel
c8a06601a7 [FIX] *: unstage on status going from success to failure
And unconditionally unstage when the HEAD of a PR is synchronised.

While a rebuild on a PR which was previously staged can be a false
positive (e.g. because it hit a non-derministic test the second time
around) it can also be legitimate e.g. auto-rebase of an old PR to
check it out. In that case the PR should be unstaged.

Furthermore, as soon as the PR gets rebuilt it goes back into
`approved` (because the status goes to pending and currently there's
no great way to suppress that in the rebuild case without also fucking
it up for the sync case). This would cause a sync on the PR to be
missed (in that it would not unstage the PR), which is broken. Fix
that by not checking the state of the PR beforehand, it seems to be an
unnecessary remnant of older code, and not really an optimisation (or
at least one likely not worth bothering with, especially as we then
proceed to perform a full PR validation anyway).

Fixes #950
2024-09-18 15:19:13 +02:00
Xavier Morel
a046cf2f7c [FIX] *: UX around fw=no
The UX around the split of limit and forward port policy (and
especially moving "don't forward port" to the policy) was not really
considered and caused confusion for both me and devs: after having
disabled forward porting, updating the limit would not restore it, but
there would be no indication of such an issue.

This caused odoo/enterprise#68916 to not be forward ported at merge
(despite looking for all the world like it should be), and while
updating the limit post-merge did force a forward-port that
inconsistency was just as jarring (also not helped by being unable to
create an fw batch via the backend UI because reasons, see
10ca096d86).

Fix this along most axis:

- Notify the user and reset the fw policy if the limit is updated
  while `fw=no`.
- Trigger a forward port if the fw policy is updated (from `no`) on a
  merged PR, currently only sources.
- Add check & warning to limit update process so it does *not* force a
  port (though maybe it should under the assumption that we're
  updating the limit anyway? We'll see).

Fixes #953
2024-09-17 11:31:20 +02:00
Xavier Morel
851656bec0 [IMP] runbot_merge: set status on skipchecks & use that
- rather than enumerate states, forward-porting should just check if
  the statuses are successful on a PR
- for the same consistency reasons explained in
  f97502e503, `skipchecks` should force
  the status of a PR to `success`: it very odd that a PR would be
  ready without being validated...
2024-09-16 12:49:23 +02:00
Xavier Morel
fe7cd8e1f0 [IMP] runbot_merge: name PR correctly on staging success
Logging PRs by id is unusual, unreadable, and inconvenient.
2024-09-16 12:48:42 +02:00
Xavier Morel
10ca096d86 [FIX] forwardport: cron create prototype
Apparently when I added these to trigger the corresponding cron the
lack of decorator caused them to not work at all, at least when
invoked from the interface, consequently I notably wasn't able to
force a forward port via creating one such task when trying to work
around #953
2024-09-16 12:46:44 +02:00
Christophe Monniez
3ae3f6dff3 [IMP] runbot: pull docker image only when needed
When a lot of Docker images are updated at the same time, all runbot
hosts will try to pull them at the same moment.

With this commit, only the images marked as `always_pull` will be pulled
and if one of them takes too much time to be pulled, we let the host
make another loop turn before continuing the images pull.

Finally, if a host uses the Docker registry, it will pull the remote
image when running a step, that way the image will be pulled
automatically if needed.
2024-09-13 11:57:07 +02:00
Xavier-Do
b60d54d4ea [FIX] runbot: fix image build failure 2024-09-12 13:08:00 +02:00
Xavier-Do
b96b32e8b9 [IMP] runbot: better docker logging
The docker operation are called often and cannot be logged each time.
If a docker operation is slow, we log it at the end but waiting for this
output we have no idea what the process is doing.

This pr proposed to go a lower level and get the stream from the docker
operation to be able to log earlier if we started a potentially slow
operation.
2024-09-11 13:16:13 +02:00
Xavier-Do
14bac37438 [IMP] runbot: small change of docker_build api
returns now a dict instead of a tuple for easier extension
2024-09-11 13:16:13 +02:00
Christophe Monniez
e4b04a42d1 [FIX] runbot: remove leftover tuple in markdown log 2024-09-09 08:07:42 +02:00
Xavier Morel
60188063f8 [FIX] *: ensure I don't get bollocked up again by tags
Today (or really a month ago) I learned: when giving git a symbolic
ref (e.g. a ref name), if it's ambiguous then

1. If `$GIT_DIR/$name` exists, that is what you mean (this is usually
   useful only for `HEAD`, `FETCH_HEAD`, `ORIG_HEAD`, `MERGE_HEAD`,
   `REBASE_HEAD`, `REVERT_HEAD`, `CHERRY_PICK_HEAD`, `BISECT_HEAD` and
   `AUTO_MERGE`)
2. otherwise, `refs/$name` if it exists
3. otherwise, `refs/tags/$name` if it exists
4. otherwise, `refs/heads/$name` if it exists
5. otherwise, `refs/remotes/$name` if it exists
6. otherwise, `refs/remotes/$name/HEAD` if it exists

This means if a tag and a branch have the same name and only the name
is provided (not the full ref), git will select the tag, which gets
very confusing for the mergebot as it now tries to rebase onto the tag
(which because that's not fun otherwise was not even on the branch of
the same name).

Fix by providing full refs to `rev-parse` when trying to retrieve the
head of the target branches. And as a defense in depth opportunity,
also exclude tags when fetching refs by spec: apparently fetching a
specific commit does not trigger the retrieval of tags, but any sort
of spec will see the tags come along for the ride even if the tags are
not in any way under the fetched ref e.g. `refs/heads/*` will very
much retrieve the tags despite them being located at `refs/tags/*`.

Fixes #922
2024-09-06 15:09:08 +02:00
Xavier Morel
d0723499a2 [IMP] runbot_merge: stage by first ready
This is an approximation under the assumption that stored computes
update the `write_date`, and that there's not much else that will be
computed on a batch.

Eventually it might be a good idea for this to be a proper field,
computed alongside the unblocking of the batch.

Fixes #932
2024-09-06 13:51:55 +02:00
Xavier Morel
146564a90a [FIX] runbot_merge: set staging_end on all terminations
Rather than only setting `staging_end` on status change, set it when
the staging gets deactivated. This way cancelling a staging (whether
explicitely or via a PR update) will also end it, so will a staging
timeout, etc..., rather than keep the counter running.

Fixes #931
2024-09-06 13:16:37 +02:00
Xavier Morel
64f9dcbc22 [FIX] *: unnecessary warning on r- of forward port
Reminding users that `r-` on a forward port only unreviews *that*
forwardport is useful, as `r+;r-` is not a no-op (all preceding
siblings are still reviewed).

However it is useless if all siblings are not approved or already
merged. So avoid sending the warning in that case.

Fixes #934
2024-09-06 13:04:13 +02:00
Xavier Morel
017b8d4bb0 [FIX] forwardport: don't post reminder on PRs waiting to be staged
The FW reminder is useful to remind people of the outstanding forward
ports they need to process, as taking too long can be an issue.

They are, however, not useful if the developer has already done
everything, the PR is ready and unblocked, but the mergebot has fallen
behind and has a hard time catching up. In that case there is nothing
for the developer to do, so pinging them is not productive, it's only
frustrating.

Fixes #930
2024-09-06 12:33:51 +02:00
Christophe Monniez
33452d27e7 [IMP] runbot: add the possibility to disable a test per version
When disabling tests on runbot by using the test_tags on a build error,
the tests are disabled on every tested version. This can be annoying
when a test only fails from one version up to another.

With this commit, a min and max version can be specified on a
build_error when the test_tags field is used.
If the min and max are not used, the test will be disabled cross
versions.
2024-09-06 11:09:40 +02:00
Xavier-Do
dff50b49c4 [IMP] runbot: Improve stats listing display
Sort trigger and group them by category, better selection of categories
to display on stats page
2024-09-06 09:57:45 +02:00
Christophe Monniez
bc5e7fe69c [IMP] runbot: bump chrome version to 126.0.6478.182 2024-09-05 15:34:31 +02:00
Xavier-Do
21d6c84b26 [FIX] runbot: fix markdown adding escape
A common error on runbot is to generate link containing a __init__.py
file

[/some/path/to/__init__.py](/some/path/to/__init__.py)

This would be rendered as

<a href="/some/path/to/<ins>init<ins>.py">/some/path/to/<ins>init<ins>.py</a>

Breaking the link, and the display of the name

By default markdown will not render links avoiding this issue, but it
will remain for the content of the a, needing to manage some kind of
escaping.

The way to escape markdown is to add a \ before any special character
This must be done upront before formating, adding the method
markdown_escape

Our implementation of markdown is not meant to meet the exact
specification of markdown but better suit our needs. One of the
requirements is to be able to use it to format message easily but adding
dynamic countent comming from the outside. One of the error than can
occur is also

'Some code `%s`' % code can also cause problem if code contains `

This issue could be solved using indented code block, but this would
need to complexify the generated string, have a dedicated method to
escape the code blocs, ...

Since we have the controll on the input, we can easily sanitize all
ynamic content to avoid such issues. For code block we introduce a way
to escape backtick (\`). It is non standard but will be easier to use.

Combine with that, the build._log method now allows to add args with the
values to format the string (similar to logging) but will escape
params by default. (cr.execute spirit)

name = '__init__.py'
url = 'path/to/__init__.py'
code = '# comment `for` something'
build._log('f', 'Some message [%s](%s) \n `%s`', name, url, code)

name, url and code will be escaped
2024-09-05 15:33:41 +02:00
Xavier-Do
2fd7f47e49 [IMP] runbot: make stats when build timeout
The stats are only kept when the build finishes, this makes sence to
avoid collecting stats of a manually killed build (or because of newer
build found), but the stats mays be interresting when the build timeouts.

Adding a manual collection of stats in this case.
2024-09-05 14:24:41 +02:00
Xavier-Do
bc8c2a4853 [IMP] runbot: faster fetch
Use 2 jobs to fetch multiple origin at the same time.
2024-09-05 14:20:06 +02:00
Xavier Morel
1d106f552d [FIX] runbot_merge: missing feedback on fw r+
In some cases, feedback to the PR author that an r+ is redundant went
missing.

This turns out to be due to the convolution of the handling of
approval on forward-port, and the fact that the target PR is treated
exactly like its ancestors: if the PR is already approved the approval
is not even attempted (and so no feedback if it's incorrect).

Straighten up this bit and add a special case for the PR being
commented on, it should have the usual feedback if in error or already
commented on.

Furthermore, update `PullRequests._pr_acl` to kinda work out of the
box for forward-port: if the current PR is a forward port,
`is_reviewer` should check delegation on all ancestors, there doesn't
seem to be any reason to split "source_reviewer", "parent_reviewer",
and "is_reviewer".

Fixes #939
2024-09-05 13:25:19 +02:00
Xavier Morel
3b8d392548 [FIX] pytest warnings
Backport of d7a78f89d0

- `choice` is not a proper type
- markers should be declared
2024-09-05 13:24:10 +02:00
Xavier-Do
7411bd5755 [FIX] runbot: allow - in database name
The underscore was previously used in database name, but this is not a
valid character for certificates (still need some investigation about
the limitations)

If it worked to access it it was causing issues for IOT andother.
_ is forbidden on saas actually.

The design_theme database was changed to design-theme, but this is not
accessible through the nginx condig. This should solve the issue.
2024-09-03 10:54:54 +02:00
Xavier-Do
9660cf6517 [IMP] runbot: bundle with all pr closed 2024-08-29 15:03:45 +02:00
Xavier-Do
faef017a25 [FIX] runbot: local cleanup 2024-08-29 08:44:57 +02:00
Christophe Monniez
d722160247 [IMP] drunbot: docker registry
The current runbot infrastructure has 100+ machine that will each build
all docker images.

This is unpractical for multiple reasons:
- impotant load on all machine when the build could be done once
- possible differences for the same image depending on the moment the
base was pulled on the host. An extreme example was the version of
python (3.11 or 3.12) when building the noble docker image before it was
stabilized.
- increase the chance to have a docker build failure in case of network
problem. (random)

A centralized registry will help with that, and allow future devlopment
to generate more docker images. All docker images will be exactly the
same, the pull time is faster than build time, only one build per docker
image, ...
2024-08-29 08:44:57 +02:00
Christophe Monniez
4a2c9d7491 [FIX] runbot: remove useless fields in test 2024-08-28 10:23:29 +02:00
Christophe Monniez
8ac21fb60b [IMP] runbot: allow to archive dockerfile 2024-08-28 10:23:08 +02:00