Commit Graph

348 Commits

Author SHA1 Message Date
Xavier Morel
4c8ab63af8 [REM] runbot_merge: leftover debug print
Found by @d-fence
2022-02-07 08:08:59 +01:00
Xavier Morel
4da0f5df69 [ADD] runbot_merge: ~~tree~~ freeze wizard
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.
2021-11-17 10:40:12 +01:00
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
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
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
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
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
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
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
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
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
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-Do
b843fee8d9 [FIX] runbot: adapt routes and licenses 2021-07-27 16:11:49 +02:00
Xavier Morel
318803d7bb [IMP] runbot_merge: tag PRs with a pseudo-branch on merge to master
If a PR got merged to master (or whatever the current development
branch is), there's no easy way to know what maintenance branch it
ended up landing in, except by asking git which branches contain the
commit (which can be rather slow).

Add a special case on merge which labels the PR with a pseudo-branch
patterned after the second-to-last branch of the project:

* if the branch ends with a number, increment the number by one
  e.g. 2.0 -> 2.1, 5 -> 5.1
* otherwise, just prefix with `post-` e.g. "maint" ->
  "post-maint" (that one doesn't sound very helpful, but I guess it's
  nice for the weirdoes who call their branches "natty narwhal" and
  shit)

Fixes #450
2021-03-02 14:28:32 +01:00
Xavier Morel
60af69b69f [FIX] runbot_merge: dashboard title should link to github
Currently links to self, which is not useful (the viewer is already
there).

Fixes #452
2021-03-02 14:28:32 +01:00
Xavier Morel
4e4e4303f6 [IMP] runbot_merge: add name_search override to PRs
Should allow filtering PRs by source or parent.

Fixes #458
2021-03-02 14:28:32 +01:00
Xavier Morel
21077c690a [FIX] runbot_merge: incorrect processing of rebased commit messages
5cf3617eef intended to create merge
messages with only the content of PR descriptions before the first
thematic break. However this processing was incorrectly applied
to all messages being processed (meaning rebased / squashed commit
messages as well).

Properly apply thematic break processing to only commit messages
created from PR descriptions.
2021-01-21 13:15:32 +01:00
Xavier Morel
08b8da08df [IMP] runbot_merge: precisely filter stagings before validating them
Before this, we would "roughly" select stagings by looking at stagings
whose heads matched a specific sha then validating them all. This
could perform extra validations on stagings once in a while but this
was assumed not to be much an issue, at least originally.

However two changes later on have contributed to this likely being the
cause of #429 (stagings never timing out):

* heads of the staging branches are uniquifier commits stored in the
  heads map, but the actual heads of the stagings are also stored
  there, some of which are no-ops (hence the uniquifiers) so assuming
  repos A and B, if a staging contains PRs touching A then the head of
  B actual will also be a head of B
* when a staging is validated, if it *contains* any pending result the
  timeout limit gets bumped back

The issue here is that if a success / failure status is lost (which
would be the most common reason for timeouts) *and* someone has forked
and is regularly rebuilding a branch-head used as-is by a staging,
each of those rebuilds will trigger a validation of the staging, which
will find that one of the statuses is still pending (because we missed
the success / failure), which will bump up the timeout limit,
continuing until the branch stops getting rebuilt.

This is probably one of the reasons why some stagings last for *way*
more than 2h, though it is far from explaining all of them: 90% of the
stagings lasting more than *3*h end up succeeding. Tho it's always
possible that this is because someone notices and resends a success
for the missing status it seems somewhat doubtful. Oh well.

Also fix the incorrect log call on `update_timeout_limit` triggering.
2021-01-20 09:22:11 +01:00
Xavier Morel
560c6e6b16 [IMP] mergebot: log repr of comments (might show oddities) 2021-01-14 07:42:10 +01:00
Xavier Morel
ba1a8ee089 [IMP] runbot_merge, forwardport: update some logging
Downgrade an error and a warning to info, and upgrade two warnings to
error.

Point is to improve logic of log levels & sentry visibility.

Fixes #258
2021-01-13 16:12:35 +01:00
Xavier Morel
95e35dde90 [IMP] runbot_merge: logging when bumping timeout
I'd forgotten that in order to better handle cases where the CI is
highly backed up (and / or slow for some reason), we actually update
the CI timeout to really take the last "pending" status as the "true
start" of the CI. This might explain why lots of stagings needed extra
time: as of right now, out of 28835 stagings

- 20086 had their timeout bumped by more than 15mn
- 6967 had their timeout bumped by more than 30mn
- 264 had their timeout bumped by more than 1h
- 30 had their timeout bumped by more than 2h

Add some logging every time the CI is bumped this way, so we have
better visibility into that event.

Closes #429
2021-01-13 16:12:35 +01:00
Xavier Morel
3cc87051dd [FIX] runbot_merge: avoid repeatedly warning about the same failures
The mergebot has a feature to ping users when an approved PR or
forward-port suffers from a CI failure, as those PRs might be somewhat
unattended (so the author needs to be warned explicitly).

Because the runbot can send the same failure information multiple
times, the mergebot also has a *deduplication* feature, however this
deduplication feature was too weak to handle the case where the PR has
2+ failures e.g. ci and linting as it only stores the last-seen
failure, and there would be two different failures here.

Worse, because the validation step looks at all required statuses, in
that case it would send a failure ping message for each failed
status *on each inbound status*: first it'd notify about the ci
failure and store that, then it'd see the linting failure, check
against the previous (ci), consider it a new failure, notify, and
store that. Rinse and repeat every time runbot sends a ci *or* lint
failure, leading to a lot of dumb and useless spam.

Fix by storing the entire current failure state (a map of context:
status) instead of just the last-seen status data.

Note: includes a backwards-compatibility shim where we just convert a
stored status into a full `{context: status}` map. This uses the
"current context" because we don't have the original, but if it was a
different context it's not going to match anyway (the target_url
should be different) and if it was the same context then there's a
chance we skip sending a redundant notification.

Fixes #435
2021-01-13 16:12:35 +01:00
Xavier Morel
e175609950 [IMP] forwardport: unmodified fw automatically inherit overrides
Before this change, a CI override would have to be replicated on most
/ all forward-ports of the base PR. This was intentional to see how it
would shake out, the answer being that it's rather annoying.

Also add a `statuses_full` computed field on PRs for the aggregate
status: the existing `statuses` field is just a copy of the commit
statuses which I didn't remember I kept free of the overrides so the
commit statuses could be displayed "as-is" in the backend (the
overrides are displayed separately). And while at it fix the PR
dashboard to use that new field: that was basically the intention but
then I went on to use the "wrong" field hence #433.

Mebbe the UI part should be displayed using a computed M2M (?)
as a table or as tags instead? This m2m could indicate whether the
status is an override or an "intrinsic" status.

Also removed some dead code:

* leftover from the removed tagging feature (removed the tag
  manipulation but forgot some of the setup / computations)
* unused local variables
* an empty skipped test case

Fixes #439.

Fixes #433.
2021-01-13 16:11:14 +01:00
Xavier Morel
5cf3617eef [IMP] runbot_merge: split out content after separator when creating commit
Currently when creating *merges* the commit message is the
concatenation of the PR title and the PR body.

However it can be convenient to include description test which would
not be included in the commit message e.g. PR template
stuff. "Thematic breaks" seem like a good way to do this: the commit
message will only include the content preceding the first thematic
break, everything else is ignored (except headings which are not
ignored, double negations FTW).

Might be that that's a bit excessive and we should only ignore what
follows the *last* thematic break. Or that there needs to be a more
specific marker. We'll see.

Fixes #443.
2021-01-12 12:24:34 +01:00
Xavier Morel
842b1e4b96 [FIX] mergebot: handle multiples of each command
Because the commands were collected in a `dict[Command, Params]` a
command could only be present once *in the mergebot* (the forwardbot
already collected commands in a list).

* Update mergebot commands parser to collect commands in a list.
* Improve override to allow a comma-separated list of CIs to override.
* Simplify the parsing of delegate to generate one delegate command
  per target (slightly less efficient if multiple logins are provided
  but that is likely extremely rare).

Fixes #445
2021-01-12 09:54:45 +01:00
Martin Trigaux
8c625278c3 [FIX] runbot_merge: correct Linked PRs URLs
It was not evaluated and rendered to something like
  https://mergebot.odoo.com/odoo/odoo/pull/linked.url

Courtesy of Xavier Dollé (xdo)
2020-12-03 11:01:36 +01:00
Xavier Morel
5c19342bf6 [CHG] runbot_merge, forwardport: remove labelling
Because github materialises every labels change in the
timeline (interspersed with comments), the increasing labels churn
contributes to PRs being difficult to read and review.

This change removes the update of labels on PRs, instead the mergebot
will automatically send a comment to created PRs serving as a
notification that the PR was noticed & providing a link to the
mergebot's dashboard for that PR where users should be able to see the
PR state in detail in case they wonder what's what.

Lots of tests had to be edited to:

- remove any check on the labels of the PR
- add checks on the PR dashboard (to ensure that they're at least on
  the correct "view")
- add a helper to handle the comment now added to every PR by the 'bot
- since that helper is needed by both mergebot and forwardbot, the
  utils modules were unified and moved out of the odoo modules

Probably relevant note: no test was added for the dashboard
ACL, though since I had to explicitly unset the group on the repo used
for tests for things to work it looks to me like it at least excludes
people just fine.

Fixes #419
2020-11-20 07:41:54 +01:00
Xavier Morel
c80d8048f7 [ADD] runbot_merge: PR dashboard
Provides a view of the "ongoing" state of a PR in order to more easily
know what might be blocking / problematic.
2020-11-20 07:41:54 +01:00
Xavier Morel
47e8b5b014 [IMP] runbot_merge: overridable CI
Convert overridable CI to an m2m from partners, it's significantly
more convenient to manipulate as multiple users can (and likely will)
have access to the same overrides, add a name_search so the override
is easy to find from a partner, and provide a view for the
overrides (with partners as tags).

Also make the repository optional on CI overrides.

Fixes #420
2020-11-20 07:41:54 +01:00
Xavier Morel
8abf1be278 [FIX] runbot_merge: cancel approval (r+) when fetching PRs
When retrieving unknown PRs, the process would apply all comments,
thereby applying eventual r+ without taking in account their
relationship to a force push. This means it was possible for a
mergebot-unknown PR to be r+'d, updated, retargeted, and the mergetbot
would consider it good to go.

The possible damage would be somewhat limited but still, not great.

Sadly Github simply doesn't provide access to the entire event stream
of the PR, so there is no way to even know whether the PR was updated,
let alone when in relation to comments. Therefore just resync the PR
after having applied comments: we still want to apply the merge method
& al, we just want to reset back to un-approved.

An other minor fix (for something we never actually hit but could):
reviews are treated more or less as comments, but separate at github's
level. The job would apply all comments then all reviews, so the
relative order of comments and reviews would be wrong.

Combine and order comments and reviews so they are applied
in (hopefully) the correct order of their creation / submission.

Closes #416
2020-11-20 07:41:54 +01:00
Xavier Morel
963ba9e911 [FIX] runbot_merge: name_get using the wrong field on override rights 2020-11-20 07:41:54 +01:00
Xavier Morel
dd0905f8d3 [IMP] runbot_merge: handling of PRs on disabled branches
Historically PRs to disabled branches were treated like PRs to
un-managed branches: ignored.

However because they cay *already exist* when the branch is disabled,
the effects can be subtly different, and problematically so
e.g. ignoring all PR events on PRs targeting disabled branches means
we can't close them anymore, which is less than great.

So don't ignore events on PRs to disabled branches (creation, sync,
closing, and reopening) but also send feedback on PRs to disabled or
un-managed branches to indicate that they're not merge-able.

Fixes #410
2020-10-06 14:45:49 +02:00
Xavier Morel
c61c1c74a0 [FIX] runbot_merge: remove widget on branch_filter
Apparently:

* this requires a `model` option (makes sense)
* this blows up in a list view (makes less sense)
2020-10-05 08:02:14 +02:00
Xavier Morel
3b28d7801d [IMP] runbot_merge: be more optimistic about PR mergeability
If we can't stage a PR, rather than immediately put them in error wait
until they were the first we tried staging, otherwise they might have
been conflicting with the previous batch which ultimately won't be
merged for other reason and they would have worked as part of the next
batch.

Note: this commit will lead to false negatives because it's
batch-based rather than repo-based, so if the first batch only affects
repo A and the second batch only affects repo B, if the second batch
triggers a merge error it should be rejected immediately (as it's
applied on a "pristine" repo B) but this change will just bump it to
the next staging.

fixes #209
2020-10-02 15:28:36 +02:00
Xavier Morel
c78ffb9e3f [CHG] runbot_merge: branch_ids -> branch_filter
On per-repo status configurations, convert the "branch_ids" filter to
a domain on branches. Since the selection is generally
binary (statuses either apply to the master branch or apply to
non-master branch) this avoids error-prone missed updates where we
forget to enable statuses pretty much every time we fork off a new
branch.

Fixes #404
2020-10-02 15:28:36 +02:00
Xavier Morel
2fafd5419a [FIX] runbot_merge: attempt to stage PRs on disabled branches
Normally opening a PR against a disabled branch is like opening a PR
against a branch which is not configured at all: the PR id ignored
entirely.

However if the PR already exists then the state of the branch isn't
normally checked when interacting with the branch, and it is possible
to trigger its staging, at which point the staging itself will crash:
on a project the branches are `active_test=False` so they're all
visible in the form, but when repos go search()-ing for the branch
they won't find it and will blow up.

Solution: only try staging on branches which are active. Fixes
odoo/runbot#408. Also do the same for checking stagings.

And while at it, fix #409 by wrapping each checking or staging into a
try/except and a savepoint. This way if a staging blows up it should
move on to the next branch instead of getting stuck.
2020-10-02 15:28:36 +02:00
Xavier Morel
c93ce7c2c2 [FIX] runbot_merge: blocked flag false positive
The "blocked" computation would not take branch targets in account, so
PRs with the same label targeting *different branches* (possible if
somewhat rare due to our naming conventions) could block one another,
despite really being unrelated.

Also fix up some messages:

* if a PR is blocked due to having no merge method, it should say
  that, not "has no merge" (no merge what?)
* format un-managed branches as `$repo:$branch` in logging messages,
  `$repo#$thing` is for issues / PRs and `$branch` alone can be very
  unhelpful

Closes #407
2020-10-02 15:28:36 +02:00
Xavier Morel
32384548ab [FIX] runbot_merge: stuck feedback cron
When using @fw-bot close, a feedback would be created without a
message (rather than e.g. with an empty one). As a result, the
feedback-sending cron would crash, but not before having closed the
corresponding PR.

This would lead to closing the PR in a loop & spamming the logs with
tracebacks.
2020-09-14 14:04:40 +02:00
Xavier Morel
9980bfd843 [FIX] runbot_merge: make Sentry middleware delegate to wrapped app
In Odoo 13, the cache middleware was modified to straight hit
`http.root` assuming it's the Odoo root object. When `http.root` is
replaced by a wrapping middleware, the entire thing blows up and shits
the bed.

Patch up by automatically delegating attribute accesses to the wrapped
application (which is probably Root), although why this is not just
folded into Root is getting less and less clear.
2020-07-30 09:49:26 +02:00
Xavier Morel
07918cddd6 [FIX] runbot_merge: incorrect attribute access
Seems to be a pretty long-standing issue but I'd not noticed it before
as it's rather rarely taken & our sentry remains rather blown to hell,
I only happened to stumble upon the issue in the logs.

There's no ``number`` attribute on the repository object (to which
``_load_pr`` belongs). We obviously want to use the number of the PR
we're currently loading.
2020-07-30 09:14:15 +02:00
Xavier Morel
b8408546a0 [FIX] runbot_merge: stagings dashboard
Mistake in the statuses handling: the context is not sufficient to
uniquely identify a staging status as different repositories can get
the same status context (e.g. ci/runbot is present on all our
repositories).

This is only a visual problem, but the status dropdown on
stagings (both the dashboard and the branchwise listing) would reuse
one of the status with the context for all of them, leading to
incorrect links and misleading displays.

Fix by keying on (repo, context) instead, that's exactly why the
repository name was part of the status in the first place.
2020-07-29 08:42:56 +02:00
Xavier Morel
36026f46e4 [FIX] runbot_merge: don't warn on unrecognized commands
This is a regression due to the implementation details of
odoo/runbot#376: previously _parse_command would only yield the
commands it had specifically recognised (from a whitelist).

22e18e752b simplified the implementation
and (for convenience when adding new commands) now passes through any
command to the executor instead of skipping the unknown one.

But I forgot to update the executor to ignore unknown commands, so it
treats them as *failed* (since the success flag doesn't get set) and
assumes it's an ACL issue, so notifies the user that they can't do the
thing they never really asked for.

Add an end-case which skips the feedback bit for unrecognized
commands, which restores the old behavior.

Fixes #390
2020-07-27 12:56:41 +02:00
Xavier Morel
a564723fd8 [ADD] runbot_merge: indication of status states in the mergebot
Currently it can be difficult to know why the mergebot refuses to
merge a PR (not that people care, they generally just keep sending new
commands without checking what the 'bot is telling them, oh well...).

Anyway knowing the CI state is the most complicated bit because the CI
tag only provides a global pass/fail for statuses but not a view of
specific statuses, and sometimes either the runbot or github fails to
notify the mergebot, leading to inconsistent internal states & shit.

By adding a tag per status context per PR, we can more clearly
indicate what's what.

Fixes #389
2020-07-27 12:56:41 +02:00
Xavier Morel
8364dded3e [ADD] runbot_merge: untested case, failed non-required status
Ensure that a non-required status failing does not trigger a
notification message.

Closes #388
2020-07-27 12:56:41 +02:00
Xavier Morel
7781d8b09c [IMP] runbot_merge: only show required statuses in the dashboard
Apparently a long-running issue but not really a concern before the
new mergebot started sending a lot more statuses: stagings would show
a list of all statuses they received, including optional / irrelevant
statuses.

Get a list of required statuses and only show that on the staging
dropdowns.

Closes #387
2020-07-27 12:56:41 +02:00
Xavier Morel
22e18e752b [ADD] runbot_merge: allow overriding statuses
Adds an `override` mergebot command. The ability to override is set on
an individual per-context per-repository basis, similar to but
independent from review rights. That is, a given individual may be
able to override the status X on repository A and unable to do so on
repository B.

Overrides are stored in the same format as regular statuses, but
independent from them in order to persist them across builds.

Only PR statuses can be overridden, statuses which are overridable on
PRs would simply not be required on stagings.

An alternative to implementing this feature in the mergebot would be
to add it to individual status-generating tools on a per-need
basis.

Pros of that alternative:

* display the correct status on PRs, currently the PR will be failing
  status-wise (on github) but correct as far as the mergebot is
  concerned
* remove complexity from the mergebot

Cons of that alternative:

* each status-generating tool would have to implement some sort of ACL
  system
* each status-generating tool would have to receive & parse PR
  comments
* each status-generating tool would have to maintain per-pr state in
  order to track overrides

Some sort of helper library / framework ought make that rather easy
though. It could also be linked into the central provisioning system
thing.

Closes #376
2020-07-14 13:34:05 +02:00
Xavier Morel
b42c7b15c7 [ADD] runbot_merge: migration for repository status split 2020-07-14 13:34:05 +02:00
Xavier Morel
5c1caa1a18 [IMP] runbot_merge: provide for statuses being only on PRs or stagings
Requirement for odoo/runbot#376: one can't expect there being someone
to override CI checks on stagings, so it only makes sense for checks
on PRs, which in turns requires that there could be checks only
required on PRs.

Could also be useful for features like incremental linting /
formatting, we may want to apply checks on PRs which filter on the
lines modified, but not require the entire software be reformatted at
once.
2020-07-14 13:34:05 +02:00
Xavier Morel
be228a5681 [IMP] runbot_merge: split status configuration into own object
Having the required statuses be a mere list of contexts has become a
bit too limiting for our needs as it doesn't allow e.g. adding new
required statuses on only some branches of a repository (e.g. only
master), nor does it allow putting checks on only branches, or only
stagings, which would be useful for overridable checks and the like,
or for checks which only make sense linked to a specific revision
range (e.g. "incremental" linting which would only check whatever's
been modified in a PR).

Split the required statuses into a separate set of objects, any of
which can be separately marked as applying only to some branches (no
branch = all branches).

Fixes #382
2020-07-14 13:34:05 +02:00
Xavier Morel
53b7c05701 [IMP] runbot_merge: drop merge method on squash
When moving from !squash to squash to !squash, it's a bit odd / risky
to keep the merge method as-is. So drop it.
2020-05-28 13:33:56 +02:00
Xavier Morel
b2a4da4739 [FIX] runbot_merge: update squash flag on reopen
While the head gets updated (properly), the squash flag did not which
could lead to odd results. Since a PR can only be reopened if it was
regular-pushed to (not after a force push) there are two scenarios:

* the PR updated to have 0 commits, closed, pushed to with one commit
  then reopened, after reopening the PR would be marked as !squash and
  would ask for a merge method (that's what happened with
  odoo/odoo#51763)
* the PR has a single commit, is closed, pushed to then reopened,
  after reopening the PR would still be marked a squash and potentially
  straight rebased without asking for a merge method

Nothing would break per-se but both scenarios are undesirable.

Close #373
2020-05-28 13:33:56 +02:00
Xavier Morel
c005561546 [FIX] runbot_merge: mistake in logging parameter
When it updated tagging e82de3136b also
incorrectly replaced a `pr` by `pr.display_name`, probably leftover
from an attempt to update a callsite from `str(pr)` to
`pr.display_name` which I missed when reverting that.

Anyway at that section, `pr` is an integer (as it comes from an SQL
query) not an object.
2020-04-17 08:09:01 +02:00
Xavier Morel
c26ed2ecab [FIX] runbot_merge: typo in field parameter name 2020-03-17 08:17:37 +01:00
Xavier Morel
5f8ad6a626 [IMP] runbot_merge: support for merging partners w/ github_login
The logic of the partner merge wizard is to collect all relevant data
from source partners, write them to a destination partner, then remove
the sources.

This... doesn't work when the field in question has a UNIQUE
constraint (like github_login), because it's going to copy the value
from a source onto a dest which will blow the constraint, and so the
copy fails. In that case the user first has to *move over* the unique
field's value then they can use the wizard.

Just fix for the github login: take all sources, remove (and store)
their github logins, then write the login onto the dst.

An alternative would have been to *defer* the constraint, however:

* it only works on unique constraints, not unique indexes
* it requires the constraint to be declared DEFERRABLE

Closes #301
2020-03-16 15:03:11 +01:00
Xavier Morel
736e618110 [IMP] runbot_merge: non-empty fallback on fast-forward failure
Up till now if an FF failed with an exception having neither cause nor
context the cancel reason would be an empty string.

Fallback on stringifying the exception itself as a last resort.
2020-03-16 15:03:11 +01:00
Xavier Morel
974bab40ba [IMP] runbot_merge: add style for successful but unmerged staging
Currently the PR becomes successful-green as soon as CI fully passes
but before it's merged, which can be an issue as e.g. merging might be
delayed (there's no visible difference between "CI success" and
"staging merged") or it might ultimately failed (FF error).

Create an intermediate color for "successful" stagings which are still
pending merge.

Also add a fallback message for fast-forward errors instead of en
empty string.

Closes #308
2020-03-16 15:03:11 +01:00
Xavier Morel
f60bc1d067 [IMP] forwardport: on conflict note previous FP can be r+'d
Closes #294
2020-03-16 15:03:11 +01:00
Xavier Morel
e82de3136b [IMP] runbot_merge, forwardport: unify tagging
Genericise runbot_merge's tagging (move states to the "UI" but only
store / manage actual tags), and remove forwardport.tagging as it's
now redundant.

Closes #232
2020-03-16 10:53:19 +01:00
Xavier Morel
0b73e5c640 [IMP] fwbot: warning on r+ for failed PR
approving a PR which failed CI should trigger a feedback message since
6cb58a322d (#158), the code has not been
removed and the tests still pass.

However fwbot r+ would go through its own process for r+ which would
explain why that feedback is sometimes gone / lost (cf #327 and #336).

* make fwbot r+ delegate to mergebot r+
* add dedicated logging for this operation to better analyze
  post-mortem
* automatically ping the reviewer to specifically tell them they're idiots
* move the feedback item out of the state change bit, send it even if
  it's a useless r+ (because it's already r+'d)
* add a test for forward-ports

Closes #327, closes #336
2020-03-10 07:58:09 +01:00
Xavier Morel
a3599f34cc [IMP] runbot_merge: don't inject Related if related PR already referenced
Closes #325
2020-03-04 11:56:01 +01:00
Xavier Morel
dde59b9fe6 [IMP] runbot_merge, forwardport: better signed-off-by handling
Remove original-signed-off-by, doesn't actually seem useful given the
semantics of signed-off-by according to the kernel doc'. Plus it
didn't actually work as the intent was to keep the signoff of the
original PR in the forward-port, but that signoff is not part of the
commit we're cherrypicking (it gets added on the fly when the commit
is merged).

Therefore explicitly get the ack-chain into the PR: when merging an FP
PR, try to integrate the signoff of the original PR, that of the final
FP pr, and while at it that of the last explicit update in the commit
chain (e.g. in case there's been a conflict or something).

Fixes #284
2020-03-04 11:56:01 +01:00
Xavier Morel
745f385dd3 [FIX] runbot_merge: error in test 2020-02-26 16:21:24 +01:00
Xavier Morel
9dbd0ac623 [IMP] runbot_merge: note on behalf of which commit a staging was created
Closes #329
2020-02-26 16:21:24 +01:00
Xavier Morel
48ba61d872 [IMP] runbot_merge: display & filtering of partners list
* only provide fields which make sense for the mergebot
* provide formatting & searchability for review rights records so
  they're visible from the list directly
2020-02-12 15:35:18 +01:00
Xavier Morel
7bd136b9d2 [FIX] runbot_merge: leftover debug prints 2020-02-11 14:37:03 +01:00
Xavier Morel
c235e9f6cc [ADD] runbot_merge: substitution filter on PR labels 2020-02-11 14:20:32 +01:00
Xavier Morel
314622e326 [IMP] runbot_merge: handle on repo_ids field
So they can be resequenced by drag&drop
2020-02-11 10:05:00 +01:00
Xavier Morel
3b417b16a1 [FIX] runbot_merge: when looking for stageable PRs group by target
This is more of a sanity check as it normally should not be a factor:
labels generally contain the target name, and staging checks are
performed per-target so we're not mixing multiple targets anyway.

But let's say a third-party creates a fix-foo branch for A and a
fix-foo branch for B, we want to ensure they're not considered batched
together.
2020-02-11 09:49:39 +01:00
Xavier Morel
b96bc9a58c [FIX] runbot_merge: make github_login case insensitive
Rather than try to fix up various bits where we search & all and
wonder what index we should be using, make the column a CIText.

For mergebot the main use case would be properly handling
delegate=XXX: currently if XXX is not a case-sensitive match we're
going to create a new partner with the new github login and
give *them* delegation, and the intended target of the delegation
isn't going to work correctly.

Also try to install the citext extension if it's not in the database,
and run the database-creation process with `check=True` so if that
fails we properly bubble up the error and don't try to run tests on a
corrupted / broken DB.

Fixes #318
2020-02-11 09:17:52 +01:00
Xavier Morel
742e3219a6 [IMP] runbot_merge: make review rights repo-dependent
As the odds of having more projects or more repos with different
requirements in the same project, the need to have different sets of
reviewers for different repositories increases.

As a result, rather than be trivial boolean flags the review info
should probably depend on the user / partner and the repo. Turns out
the permission checks had already been extracted into their own
function so most of the mess comes from testing utilities which went
and configured their review rights as needed.

Incidentally it might be that the test suite could just use something
like a sequence of commoditized accounts which get configured as
needed and not even looked at unless they're used.
2020-02-11 08:07:57 +01:00
Xavier Morel
05444aaf3f [IMP] runbot_merge: downgrade PRs to priority 1 on r-
Before this change, `r-` on a pr[p=0] does essentially nothing. At
most it will unstage if the PR had been (somewhat unnecessarily) r+'d
in the past but then the PR will get re-staged immediately.

To avoid this odd behaviour, if r- is sent to a p=0 PR not only is
the PR unreviewed (if it was reviewed) it always gets unstaged, and
its priority gets reset to 1 (high priority but doesn't bypass CI and
review). Also send a comment on that subject so followers of the pr
are notified.

Fixes #313
2020-02-11 08:07:57 +01:00
Xavier Morel
d9661064d6 [FIX runbot_merge: prevent reopening a merged PR
Fixes #305
2020-02-11 08:07:57 +01:00
Xavier Morel
18736d282f [FIX] runbot_merge: incorrect deref'
in controller, pr is the dict of the PR coming from github, pr_obj is
the odoo-level PR object (if any)
2020-02-11 08:07:57 +01:00
Xavier Morel
289c3deee8 [ADD] runbot_merge: tests to check behaviour of github in case of updating closed PRs 2020-02-11 08:07:57 +01:00
Xavier Morel
53e46001ba [IMP] runbot_merge, forwardport: notify when main crons are off
During freezes it can be useful to notify viewers that nothing is
going to forward port or merge for a while, and that this is
intentional (not something that's broken).

Fixes #307
2020-02-11 08:07:57 +01:00
Xavier Morel
9d661480fc [IMP] runbot_merge: split staging cron in two
The staging cron was already essentially split between "check if one
of the stagings is successful (and merge it)" and "check if we should
create a staging" as these were two separate loops in the cron.

But it might be useful to disable these two operations separately
e.g. we might want to stop the creation of new staging but let the
existing stagings complete.

The actual splitting is easy but it turns out a bunch of tests were
"optimised" to only run the merge cron. Most of them didn't blow up
but it seems more prudent to fix them all.

fixes odoo/runbot#310
2020-02-11 08:07:57 +01:00
Xavier Morel
c702fecda1 [FIX] runbot_merge: updating PRs in repos without required statuses
The PR creation had been fixed to always validate even without a
commit found (in case there was no need for a commit), but the update
of a PR in such a situation was not tested, and thus naturally did not
work because why would it work if it wasn't tested?

Also remove the conditional skip on updating a PR to a new head.
2020-02-07 16:11:12 +01:00
Xavier Morel
8f09eacfb5 [FIX] runbot_merge: handling of empty required_statuses for computed pr status 2020-02-07 15:54:14 +01:00
Xavier Morel
0831c899e8 [FIX] runbot_merge: the fix
The test was checking things would work properly with
required_statuses being an empty string, because I'd also forgotten an
empty field becomes stored as `False` in the database, so trying
things out live neither the PRs nor the staging would work as their
assumption that they could straight split the required_statuses would
always fail.

Update the test to better match expectations, and hopefully this is
the end of that saga.
2020-02-07 09:55:22 +01:00
Xavier Morel
4bdf7e5eda [FIX] runbot_merge: stagings involving repos w/o required statuses
PRs transitioning to 'ready' had been checked and tested but turns out
I had completely forgotten to test that stagings would validate
properly therefore of course they didn't.

The issue here was I'd forgotten `''.split(',')` returns `['']` rather
than `[]`, so on an empty required_statuses the staging validator
would keep looking for a status matching the context `''` and would
never find it, keeping the staging pending until timeout. So most
likely the problem could have been resolved by just adding a condition
to

    [r.strip() for r in repomap[c.sha].required_statuses.split(',')]

but I'd already done all the rest of the reorganisation by that point,
test pass and I think it's a somewhat better logic. Therefore I'll go
with that for now.

* properly handle empty required_statuses during staging validation
* remove the final postcondition, if we're missing commits which don't
  require any statuse we should not care
* expand test to include up to merging PRs
* automatically create dummy commits when creating stagings, that way
  the relevant commits are in the database (can't hurt)

PS: an other alternative would have been to filter out or skip ahead
on commits which don't require any statuses aka cmap &
required_statuse / cmap would not even have that entry
2020-02-07 08:29:55 +01:00
Xavier Morel
f4fd17a884 [ADD] runbot_merge: sequence on repositories 2020-02-04 07:45:52 +01:00
Xavier Morel
c7393e2da9 [IMP] runbot_merge: explain why a PR is blocked on the dashboard
Refactor the selection thingie, hopefully in a way which doesn't
absolutely crater performances, so that it's possible to explain the
reason why a PR is considered blocked.
2020-01-31 15:19:32 +01:00
Xavier Morel
35dbfd2c12 [FIX] runbot_merge: status deduplication (maybe)
Despite the existing dedup' sometimes the "xxx failed on this
forward-port PR" would still get multiplicated due to split builds
e.g. in odoo/odoo#43935 4 such messages appear within ~5 minutes, then
one more 10mn later.

This is despite all of them having the same "build" (target_url) and
status (failure). Since the description is the only thing that's not
logged I assume that's the field which varies and makes the dedup'
fail. Therefore:

* add the description to the logging (when getting a status ping)
* exclude the description when checking if a new status should be
  taken in account or ignored: the build (and thus url) should change
  on rebuild

Hopefully fixes #281
2020-01-31 10:40:59 +01:00