As for the builder, this give the ability to run the discovery of new
commits and all related logic in a separate process.
This will mainly be usefull to restart frontend without waiting for cron
or restart "leader" without stoping the frontend. This will also be
usefull for optimisation purpose.
As a custom codeowner system was successfully implemented in a python
step on our runbot instance, it's now time to have a real model for
that.
This commit adds a skeleton Codeowner model in order to be used for a
basic usage.
This should be improved in the future after some battle testing.
With the increasing usage of runbot to test various things and to take
care of random bugs in tests, the need of a team dashboard arose.
This commit adds a `runbot.team` model. Internal users can be
linked to the team. Module wildcards can be used to automatically assign
build errors to a team at 'build.error` creation.
Also, an upgrade exception can be assigned to a team in order to display
it on a dashboard.
A dashboard model is used to create custom dashboards on the team
frontend page. By default, a dashboard is meant to display a list of
failed builds. The failed builds are selected by specifying a project, a
trigger category (e.g. nightly), a config and a domain (which select
failed builds by default).
The dashboard can be customized by specifying a custom view.
Each created team has a frontend page that displays all the team
dashboards and the errors assigned to the team.
A few other improvement also come with this commit:
* The cleaned error is now in a tab on the build error form
* Known errors are displayed as "known" on the build log page
* The build form shows the config used for the build
Apparently 2.34:
* flipped around the "auto-merging" and "CONFLICT" messages on stdout,
so just match the second one with wildcards around to ignore the
location of the first
* changed the casing and content of everything after the `error` line
on stderr, so just ignore it all (none of it's actually useful
anyway)
Provides a less manual interface for creating the freeze:
* takes the name of the branch to create
* takes any number of PRs which must be part of the freeze
* takes PRs representing the HEADs of the new branches
Then essentially takes care of the test.
Implementation of the actual wizard is not trivial but fairly
straightforward and linear, biggest issue is not being able to
`project_id.branch_ids[1]` to get the new branch, not sure why but it
seems to ignore the ordering, clearing the cache doens't fix it.
When creating the branches, add a sleep after each one for secondary
rate limiting purposes. Same when deleting branches.
Also the forwardbot has been updated to disable the forwardport cron
while a freeze is ongoing, this simplifies the freezing process.
Note: after recommendation of @aab-odoo, tried using `_applyChanges`
in `_checkState` but it simply did not work: the two relational fields
got completely frozen and were impossible to update, which is less
than ideal. Oh well, hopefully it works well enough like this for now.
Could not reproduce it in a shell, but in the original version
`self.env.cr.rowcount` would always be 0 after the `modified`.
Turns out the check is really completely dumb, because if we got any
match in `select for update` we're going to find the same on in the
update, and thus the conditional is unnecessary. I've no idea why I
did that.
Anyway remove the conditional and just always try to unstage the PR.
The Commit test object now allows a tree of `None` (or an empty dict,
same diff) in which case it will create an empty commit (a commit
which uses the same tree as its parent).
* the repository apparently takes a lot more time to
propagate (randomly) now, so wait until we can *see* it
* also add sleep after modification operations on the new repository
Ideally this should be done a lot more ubiquitously, but for now this
seems to more or less suffice.
To prep for the addition of the freeze wizard:
* move projects out of `pull_requests.py`
* then realize half the methods there have no relation to projects and
move them to more relevant places in `pull_requests.py`
* update corresponding crons (and tests using those crons) as the
methods have changed model, and the cron definitions thus need to be
updated
* split update to labels out of sending feedback comments while at it:
labels are not used much during tests so their manipulation can be
avoided; and labels are not as urgent as feedback so the crons can
be quite a bit slower
* move the project view out of `mergebot.xml` as well
Following #531 reviews from reviewers without an email set are
rejected.
For delegates this isn't very helpful, however for specifically
configured reviewers we can warn the configurer that they need to set
an email for things to work out.
* Adds a changelog page, linked from the main, with content
automatically loaded from the source. To avoid conflicts, each entry
is its own file and entries are grouped by the month during which
the update will (probably) be deployed
* The last group (most likely "last update") doesn't have a title, the
rest do.
* Add changelog entries from the last update so it's not too empty.
* Also update the layout for the alerts a bit: remove bottom margin to
reduce loss of whitespace.
When a commit is lacking the purpose (?) tag e.g. `[FIX]`, `[IMP]`,
..., a normal commit message of the form `<module>: <info>` marches
the looks of a git pseudo-header.
This results in a commit rewrite rejiggering the entire thing and
breaking the message by moving the title to the pseudo-headers and
mis-promoting either the `closes` line of body content to "title",
resulting in a really crappy commit message
e.g. odoo/odoo@d4aa9ad031.
Update the commit rewriting procedure to specifically skip the title
line, and re-inject it without processing in the output.
Fixes#540
Re-introduce a "squash" mode solely for the purpose of fixing up
commit messages without having to go and edit them: for now "squash"
only works for single-commit PRs, acts as a normal
integration (`rebase-ff`) *but* replaces the message of the commit
itself by that of the PR, similar to the `merge` modes.
This means maintainers can update commit messages to standards by
editing the PR description (though this is obviously sensible to
edition races with the original author).
Fixes#539
The list of outstanding forwardports was pretty messy as the ordering
was unclear and there was little way to really drill into the thing.
* Shows outstanding forward ports sorted by merged date ascending, the
oldest-merged PRs are the ones most in need of fixing while PRs
which were only just merged can safely be ignored.
* List reviewers with outstanding forward-ports, allow filtering by
clicking on their name, allow deseleting through the subtitle of the
page.
* Don't display reviewer in list when page is already filtered by
reviewer.
Also improve PR page a bit:
* Add reviewer.
* Add direct link to backend (closes#524).
Closes#529
Normally when a forwardport is updated the forward-bot cascades the
update onto its followups (if it has any), but takes care to keep the
followups attached as they were not updated "by hand".
In the case of odoo/odoo#77677 however that did not work and the
followup PRs got detached. Looking at the logs, it becomes flagrant
that there was a race condition: either Git took a long time to
respond to the push, or there was an IO slowdown which led to the
"local update" taking a very long time. Either way this allowed the
"synchronize" webhook from github to arrive before the current
transaction was committed, rolling back said transaction and making
the forwardbot assume this was a "real" sync and detach the followup
from its parent.
Locking the PR row up-front ought fix the issue, and also move the
local update to before having pushed: the "extra" commits in the local
cache don't matter too much even if pushing to github fails, they'll
be cleaned up by a GC eventually.
Also migrate the `-f` on push to `--force-if-includes` in order to
avoid possible race conditions on the push (since we're not fetching
the current branch, use the full-syntax explicit CAS form, that's
exactly what we're looking for anyway).
Fixes#541 (hopefully)
The forward-port process currently automatically adds delegates of a
PR as delegates on its forward ports, but that only works for
the *source* pull request.
If a delegate is added to a forward-port, they were not able to
approve the followups to that initial port, which makes little sense.
Fixes#548
Because only the first staging failure is considered "hard" and will
put the PR in error, when looking at staging logs it's possible to see
the same PR get staged over and over and over again, which is quite
confusing.
To make the logs less weird, always log a staging failure even when it
doesn't put the PR in error. Sadly this can't be tested as `capsys` is
not able to intercept an stderr inherited by a child process (capfd
doesn't work either).
Fixes#527
When using the forwardport's shortcut, the bot would not skip
already-approved PRs leading to a warning from the mergebot that the
PR was already approved (out of nowhere which was weird).
During the walk to the ancestors, skip any PR which is not
approvable (either already approved or in a state where that makes no
sense e.g. closed).
Fixes#543
If a reviewer doesn't have an email set, the Signed-Off-By is an
`@users.noreply.github.com` address which just looks weird in the
final result.
Initially the thinking was that emails would be required for users to
*be* reviewers or self-reviewers, but since those are now o2ms / m2ms
it's a bit of a pain in the ass.
Instead, provide an action to easily try and fetch the public email of
a user from github.
Fixes#531
Since we have the model fields loaded up, we can just check into that
and assume anything that's not a field is a method.
That avoids having to go through `_call`, making things way less awkward.
After internal discussions it was concluded that this didn't extend
much more trust than allowing authors to accept their single-PR
commits without additional supervisions, and it would avoid some
inconveniences and PR-blocking.
Fixes#69 (nice)
Before the repo setup calls would be checked using `raise_for_status`
but that turns out to be less than great as github mostly returns
information via the body (as JSON), even if that information is often
useless.
Add a `check` utility which checks if the response is invalid and
prints the body.
Avoid logging below warning during the creation of the template db,
and don't emit `odoo.modules.loading` during tests.
That reduces log-spam a lot and makes tests results way more
readable (in case of failure, where the logs of the subprocess get
printed out).
Because sometimes github updates are missed (usually because github
never triggers it), it's possible for the mergebot's view of a PR
description to be incorrect. In that case, the PR may get merged with
the wrong merge message entirely, through no fault of the user.
Since we already fetch the PR info when staging it, there's very
little overhead to checking that the PR message we store is correct
then, and update it if it's not. This means the forward-port's
description should also be correct.
While at it, clean the forward port PR's creation a bit:
- there should always be a message since the title is required on
PRs (only the body can be missing), therefore no need to check that
- as we're adding a bunch of pseudo-headers, there always is a body,
no need for the condition
- inline the `pr_data` and `URL`: they were extracted for the support
of draft PRs, since that's been removed it's now unnecessary
Fixes#530
Currently any access to a PR's information triggers an unconditional
fetch.
Cache the result and the conditional request data so we can perform
conditional requests on accesses 1+. And this could lead to somewhat
lower API usage as according to github conditional requests should not
count towards rate limit use.
Make the startup of ngrok more reliable: in some cases where the
machine is heavily loaded a 2s sleep after Popen-ing ngrok is not
sufficient, and the following POST still fails.
Add a small loop, with a more explicit availability check (and lower
the initial check to 1s wait).
Also:
- make the comments clearer as I'd forgotten half the things
- extract the ngrok API base URL (well it should not include /api
but...) to its own variable
When a build error is archived, a linked children with an assigned fixer
may still appear on the error frontend page.
With this commit, old children are not showing up again.
Since the order was changed, the first values are actually the older ones.
This commit inverse newer_build_stats and older_build_stats
values in order to always have the new keys. Before this commit the new
keys where not displayed. A future improvement may be to combine keys
from all builds.
This commit also proposes to give a 0 value if the key did not exist in
the older build. This means that new keys will appear with a big
difference. This is maybe not a good idea and needs some testing. A
better solution would be to search for the first apparition.
The page of PRs in "error" is currently kinda broken: it does not show
any feedback aside from the PR being in error which is not very
useful.
The intent was always to show an explanation, but when adding the page
I just deref'd `staging_id` which always fails though in two different
ways:
* when the PR can not be staged at all (because of a conflict) there
is no staging at all with a reason to show, so there should be
a fallback that the PR could not even be staged
* `staging_id` is a related field which deref's to the staging_ids
of the first *active* batch, except when a staging completes
(successfully or not) both staging and batch are disabled.
Plus the first batch will be the one for the first staging so if the
PR is retried and fails again the wrong reason may be displayed.
So update the section to show what we want: the reason of the
staging of the *last* batch attached to the PR.
NOTE: there's one failure mode remaining, namely if a staging fails
then on retry the PR conflicts with the new state of the
repository (so it can't be staged at all), the "reason" will
remain that of the staging. This could be mitigated by attaching
a "nonsense" batch on failure to stage (similar to the
forwardport stuff), that batch would have no staging, therefore
no staging reason, therefore fallback.
Closes#525
Draft was added in 82174ae66e but turns
out the v13 ORM is not able to create a required column (even when
given a default value), at least for booleans.
So create it by hand.
On staging failure, the 'bot would point to the first error or failure
status it found on the commit. This turns out not to be correct as
we (now) have various statuses which are optional, and may fail
without blocking stagings (either because they're solely informational
or because they're blocking & overridable on PRs).
Fix this so the 'bot points to the first *required* failure.
Fixes#517
On conflicts in multi-commit PRs developers sometimes get confused as
to what happened why.
If a conflict occurs and the source pull request had multiple
commits, list all the source commits and show which one broke.
Related to #505
* 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
"Uniquifier" commits were introduced to ensure branches of a staging
on which nothing had been staged would still be rebuilt properly.
This means technically the branches on which something had been
staged never *needed* a uniquifier, strictly speaking. And those lead
to extra building, because once the actually staged PRs get pushed
from staging to their final destination it's an unknown commit to the
runbot, which needs to rebuild it instead of being able to just use
the staging it already has.
Thus only add the uniquifier where it *might* be necessary:
technically the runbot should not manage this use case much better,
however there are still issues like an ancillary build working with
the same branch tip (e.g. the "current master") and sending a failure
result which would fail the entire staging. The uniquifier guards
against this issue.
Also update rebase semantics to always update the *commit date* of the
rebased commits: this ensures the tip commit is always "recent" in the
case of a rebase-ff (which is common as that's what single-commit PRs
do), as the runbot may skip commits it considers "old".
Also update some of the utility methods around repos / commits to be
simpler, and avoid assuming the result is JSON-decodable (sometimes it
is not).
Also update the handling of commit statuses using postgres' ON
CONFLICT and jsonb support, hopefully this improves (or even fixes)
the serialization errors. Should be compatible with 9.5 onwards which
is *ancient* at this point.
Fixes#509
Although it's possible to find what PR a commit was part of with a bit
of `git log` magic (e.g. `--ancestry-path COMMIT.. --reverse`) it's
not the most convenient, and many people don't know about it, leading
them to various debatable decisions to try and mitigate the issue,
such as tagging every commit in a PR with the PR's identity, which
then leads github to spam the PR itself with pingbacks from its own
commits. Which is great.
Add this information to the commits when rebasing them (and *only*
when rebasing them), using a `Part-of:` pseudo-header.
Fixes#482
Proper attribution is important in general, but especially for
external contributors. Before this change, and the previous change
fixing authorship deduplication, it was rather easy for a "squashed"
conflict commit (attributed to the 'bot for lack of a really clean
option) to get merged by mistake.
This commit changes two things:
* The mergebot now refuses to stage commits without an email set, the
github API rejects those commits anyway so any integration mode
other than `merge` would fail, just with a very unclear error
* The forwardbot now creates commits with an empty author / committer
email when the pull request as a whole has multiple authors /
committers. This leverages the mergebot update.
Also clean up the staging process to provide richer error reporting
using bespoke exceptions instead of simple assertions. I'm not sure
we've ever encountered most of these errors so they're really sanity
checks but the old reporting would be... less than great.
Fixes#505
a45f7260fa had intended to use the
original authorship information for conflict commit even if there were
multiple commits, as long as there was only one author (/ committer)
for the entire sequence.
Sadly the deduplication was buggy as it took the *authorship date* in
account, which basically ensured commits would never be considered as
having the same authorship outside of tests (where it was possible for
commits to be created at the same second).
Related to #505
If a PR is closed on github and unknown by the mergebot, when fetched
it should be properly sync'd as "closed" in the backend, otherwise the
PR can get in a weird state and cause issues.
Also move the "I fetched the thing" comment before the actual creation
of the PR for workflow clarity, otherwise the reader has the
impression that the 'bot knew about the PR then fetched it anyway.
And improve savepoint management around the fetching: savepoints
should be released in all cases.
Closes#488.
If two PRs have the same label *in different projects entirely*, the
mergebot should not consider them to be linked, but it did as shown by
the warning message on odoo-dev/odoo#905 (two PRs created from the
same branch in different projects were seen as linked by the status
checker).
3b417b16a1 fixed the actual staging
selection, it's only the warning which did not properly segregate PRs.
Only group PRs which target the same branch (therefore are within the
same project).
Fixes#487
Though the forwardport posts regular reminders that an fw is outdated,
it can be easy to miss for the non-subject (and apparently the
subjects often just ignore the information entirely).
Add a few relevant links there:
* on PR pages, add a link to either the source or the
forward-ports (if applicable), as well as the merge date
* add a new page which lists all the PRs with outstanding
forwardports, as well as the forwardports in question
Fixes#474
