Commit Graph

1315 Commits

Author SHA1 Message Date
Xavier Morel
dbc001f841 [FIX] runbot_merge: odd behaviour in try_closing
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.
2021-11-16 14:11:28 +01:00
Xavier Morel
b0995adda0 [ADD] *: support for empty commits
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).
2021-11-16 14:11:28 +01:00
Xavier Morel
3cdcba2f26 [FIX] *: better cope with secondary rate limits
* 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.
2021-11-16 14:11:28 +01:00
Xavier Morel
08c4e94ca9 [FIX] forwardport: decoration of PullRequests.create
Apparently multi is the default, so the method not being decorated
blows up when creating fake PRs through the interface.
2021-11-12 16:03:28 +01:00
Xavier Morel
5336e53de2 [FIX] runbot_merge: name_search of PRs
Stray prints were forgotten in
4e4e4303f6, and the model should have a
`_rec_name` so the fallback doesn't trigger warnings.
2021-11-12 15:57:16 +01:00
Xavier Morel
f13c60a018 [IMP] runbot_merge: small reorg of main models file
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
2021-11-10 13:13:34 +01:00
Xavier Morel
9f7cfcc7ac [FIX] runbot_merge: miswritten view file 2021-11-10 08:11:09 +01:00
Xavier Morel
6d35b4e6f8 [IMP] runbot_merge: allow finding partners by github login
Also remove search on ref which we don't use
2021-10-25 10:14:01 +02:00
Xavier Morel
1cf33f9635 [IMP] runbot_merge: add misconfiguration warning to partner view
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.
2021-10-25 09:10:25 +02:00
Xavier Morel
0e087e7433 [ADD] mergebot, forwardbot: changelog
* 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.
2021-10-20 15:16:48 +02:00
Xavier Morel
bce0836aa9 [FIX] runbot_merge: avoid editing title line of commits
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
2021-10-20 15:16:48 +02:00
Xavier Morel
df8ccf8500 [ADD] runbot_merge: squash mode (partial)
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
2021-10-20 15:16:48 +02:00
Xavier Morel
6c60d59b11 [IMP] forwardport: hall of fame layout and informations
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
2021-10-20 15:16:35 +02:00
Xavier Morel
c6755a045a [FIX] forwardport: possible race in forwardport followup update
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)
2021-10-20 14:36:50 +02:00
Xavier Morel
1dcbb4a5ac [FIX] forwardport: allow delegates on fw PRs to review followups
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
2021-10-20 14:36:50 +02:00
Xavier Morel
bd041c9f4a [FIX] runbot_merge: always log staging failures
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
2021-10-20 14:36:50 +02:00
Xavier Morel
947bf3bb47 [FIX] forwardport: don't re-approve approved PRs during an fw-bot r+
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
2021-10-20 14:36:50 +02:00
Xavier Morel
a7808425e3 [IMP] runbot_merge: reject review without email
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
2021-10-20 14:36:50 +02:00
Xavier Morel
d32ca9a1b3 [IMP] Model emulator: better support method calls
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.
2021-10-20 14:36:50 +02:00
Xavier Morel
c036c7a28f [CHG] allow delegate reviewers to configure the merge method
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)
2021-10-20 14:36:50 +02:00
Xavier Morel
462d1699f6 [IMP] conftest: repo setup error checking
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.
2021-10-20 14:36:50 +02:00
Xavier Morel
1e296a4713 [IMP] conftest: reduce log spam in test results
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).
2021-10-20 14:36:50 +02:00
Xavier Morel
bf34e9aa95 [FIX] runbot_merge: ensure PR description is correct on merge
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
2021-09-24 15:18:36 +02:00
Xavier Morel
aa7200d1c8 [IMP] conftest: use conditional requests for PR data
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.
2021-09-24 15:18:21 +02:00
Xavier Morel
bca1f951ae [IMP] ngrok setup / runner
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
2021-09-24 15:18:21 +02:00
Christophe Monniez
cfac0a11ef [FIX] runbot: prevent showing old fixed build error
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.
2021-09-14 16:22:16 +02:00
Xavier-Do
2604d962e9 [FIX] runbot_build_stats
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.
2021-09-06 13:20:16 +02:00
xmo-odoo
874719870d
[FIX] runbot_message: error on PR page
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
2021-08-30 14:40:38 +02:00
xmo-odoo
e5f84dc380
[FIX] runbot_merge: add migration for draft column (#523)
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.
2021-08-25 15:59:22 +02:00
Xavier Morel
bef6a8e2d0 [FIX] runbot_merge: point to the right status on staging failure
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
2021-08-24 15:39:47 +02:00
Xavier Morel
678d2216b8 [IMP] forwardport: provide clearer picture of conflicts
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
2021-08-24 15:39:47 +02:00
Xavier Morel
82174ae66e [IMP] *: add draft support to mergebot, kinda
* Remove the forwardport creating PRs in draft, that was mostly to
  avoid codeowners triggering but we've removed the github one and
  hand-rolled it, so not a concern anymore.
* Prevent merging `draft` PRs, the mergebot rejects approval on draft
  PRs and insults people.

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

Fixes #500
2021-08-24 15:39:47 +02:00
Xavier Morel
4b12d88b3e [IMP] runbot_merge: remove unnecessary uniquifier dummy commits
"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
2021-08-24 15:39:47 +02:00
Xavier Morel
6096cc61a9 [IMP] *: tag all rebased commits with source PRev
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
2021-08-24 15:39:47 +02:00
Xavier Morel
30cfc4fe59 [IMP] runbot_merge: disable active test on batch fields
For post-mortem investigations and manipulations, they're inconvenient
as the batches are deactivated on success and failure.
2021-08-24 15:39:47 +02:00
Xavier Morel
52b6776204 [FIX] forwardport: add override to repository view
Otherwise the forwardport target can not be set, which is a bit of an
issue.
2021-08-24 15:39:47 +02:00
Xavier Morel
f6aff3e71c [FIX] *: prevent merging conflicts commits with loss of authorship
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
2021-08-24 15:39:47 +02:00
Xavier Morel
d249417ceb [FIX] forwardport: fix deduplication of authorship in multi-pr conflict
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
2021-08-24 15:39:47 +02:00
Xavier Morel
32829cf880 [FIX] forwardport: missing login in feedback message
Closes #503
2021-08-24 15:39:47 +02:00
Xavier Morel
747174f610 [FIX] runbot_merge: when fetching a PR, sync closed state
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.
2021-08-24 15:39:47 +02:00
Xavier Morel
f54c016ef9 [FIX] runbot_merge: link warning on PRs of different projects
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
2021-08-24 15:39:47 +02:00
Xavier Morel
670f56b491 [ADD] forwardport: views of outstanding forwardports
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
2021-08-24 15:39:47 +02:00
Xavier Morel
ca2742a12c [IMP] forwardport: error handling when creating PR
Don't try to parse the response as JSON in the error case(s): if the
errors are bad enough github can return complete non-parseable
garbage.

Only access the "text" property (response body, decoded, but unparsed)
in error cases, only parse in the success case.

Also avoid reusing variables for completely different values, even if
they're of the same type, especially if they can overlap.

fixes #470
2021-08-24 15:39:47 +02:00
Xavier Morel
8178b64c01 [IMP] forwardport: error message when trying to r+ via fwbot
Initial thinking was to remove the check entirely and leave it to the
mergebot, but the lack of error reporting / forwarding means while
technically correct it would probably be somewhat difficult to grok.

Instead, improve the error reporting:

* add a dedicated message when trying to r+ via fwbot on a non-fw
  PR (note: maybe the fwbot should not care? and just send it as-is
  to the mergebot in that case?)
* clarify the ACL error
* post both message as the forwardbot rather than the mergebot

Also add a missing token note for the feedback from the forwardport
limit.

fix #469
2021-08-24 15:39:47 +02:00
Xavier Morel
6b1f698c23 [IMP] forwardport: handling of updates causing conflicts on followups
If a PR is updated and has extent forward-ports, those forwardports
get updated automatically ("followup").

However there is an issue if the udpate causes a conflict in the
followup: the conflict gets silently pushed, and may fairly easily get
merged if it occurs in an area which the CI doesn't cover.

It's unclear what the policy really should be for this issue, and
there is no real way to *block* a pull request at the moment (save by
putting it in error at the mergebot level I guess?), so for now
clearly notify the user on both the modified PR and the followup,
with a comment on both.

We may want to revisit this eventually.

Fixes #467
2021-08-24 15:39:47 +02:00
Xavier Morel
f10d33ee85 [FIX] fwbot: properly prevent @up to on forward-port PRs
There was already a check, but the way the check behaved
means *detached* PRs would not be prevented from setting their
forward-port, despite that not doing anything.

Fix it by checking if the current PR has a source, not a parent.

Fixes #465
2021-08-24 15:39:47 +02:00
Xavier Morel
6a8c13b1ef [IMP] runbot_merge: show linked PRs during staging and after merging
Previously, a PR's status page would only show the linked / related
PRs when `open`.

Since the relations between PRs remains useful, also make this
information available during staging and after merging.

Fixes #463
2021-08-24 15:39:47 +02:00
Xavier Morel
c1ebe9da52 [IMP] runbot_merge: format of staging timestamps
* in the main dashboard, show the exact UTC timestamp (with a Z
  marker) on hover, not just the relative delta
* in the branch details page, show the full timestamp, zoned, in
  ISO-8601 format
2021-08-24 15:39:47 +02:00
Xavier Morel
e542dfc852 [IMP] repo creation error handling + warnings
* The repo would only be registered at the very end of the creation,
  meaning an error *during* the repo creation (e.g. while uploading the
  first blob or setting up webhooks) would leave the repository
  undeleted. Register the repository as soon as we know it was
  created, in order to correctly dispose of it afterwards.
* Migrate logging.warning call to warnings.warn on repository deletion
  failure: pytest will print warnings during its reporting, not so for
  log warnings (?)
2021-08-24 15:39:47 +02:00
Xavier-Do
b55e38ae2d [FIX] runbot: wait for chart.js to be loaded
Since the frontend_assets are loaded with `defer="defer"`,
the page sometimes fail with the message:
```
stats.js:212 Uncaught ReferenceError: Chart is not defined
    at updateChart (stats.js:212)
    at stats.js:158
    at XMLHttpRequest.xhttp.onreadystatechange (stats.js:53)
```

This commit checks that Chart is available before tring to render the graph.

Thanks to @kebeclibre for the help.
2021-08-19 15:49:41 +02:00