Commit Graph

211 Commits

Author SHA1 Message Date
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
Xavier Morel
1b9bd67776 [FIX] runbot_merge: incorrectly logged PRs
A while back I implemented name_get/display_name to print PRs using
the canonical github format (owner/repo#number), however looks like
some of the logging calls were still using bespoke formatting.
2020-01-29 15:59:43 +01:00
Xavier Morel
ecd75b1822 [FIX] runbot_merge: statuses migration script should be for 13.0
Moving statuses from project to repo was originally developed on 11,
but since the PR was only merged after the 13.0 update, the script
migration script should be moved to match.
2020-01-29 13:29:21 +01:00
Xavier Morel
dd22f687bf [IMP] runbot_merge: allow filtering out branches from repositories 2020-01-24 13:39:14 +01:00
Xavier Morel
6b3c81a177 [FIX] make pytest cross-module-runnable
The pytest suite had been partially unified between mergebot and
forwardport but because of session-scoped modules it could not run
across those.

Make the db cache lazy and able to cache multiple databases, and move
the "current required module" to function scoped, this way things
should (and seem to) work properly on runs involving mergebot & fwbot.

Next step: xdist! (need to randomise repo names for that, probably).
2020-01-24 13:39:14 +01:00
Xavier Morel
7dfa973b57 [IMP] runbot_merge, forwardport: move required statuses to repository
Allows more flexibility in project composition as different
repositories can each have their own CI passes.
2020-01-24 13:39:14 +01:00
Denis Ledoux
d0138712bd [ADD] runbot_merge: automatic reviewer de-provisioning
When an employee sadly leaves Odoo,
the Odoo production database (odoo.com) will call these routes
in order to remove the reviewer rights automatically.

So a user who no longer works for Odoo can't "r+" Github PRs.

This is related to odoo/internal#617
2020-01-24 13:28:18 +01:00
Xavier Morel
5dccb141b7 [FIX] runbot_merge: disable homepage added enabled by website
Pages take over from redirections which really is a pain in the ass
when trying to find out why the bloody redirection seemingly refuses
to work.

Note: can't use the record tag because homepage_page is marked as
noupdate, so we have to bypass the flag checking.
2020-01-23 13:57:33 +01:00
Xavier Morel
b1ce1e82e0 [REM] runbot_merge: str override on pull_request
Interaction of CacheMiss and BaseModel is fucked, leading to an
infinite loop when trying to provide useful __str__ on a model (by
accessing model fields).
2020-01-23 10:08:55 +01:00
Xavier Morel
3f44bb5c9e [FIX] runbot_merge: styling crap
bs4 yields complete vomit on the template as-is (see:
https://imgur.com/a/XIMn7MX).

Add a bunch of color and styling overrides to get something closer to
the original, and move the existing styles to a "proper" scss file
while at it.
2020-01-21 11:21:14 +01:00
Xavier Morel
63271cd82e [IMP] runbot_merge: better notification of stored field update
Using `modified` seems safer than just blowing the cache with respect
to stored computed fields depending on PR state (not sure there are
any but it's likely).
2020-01-13 08:47:58 +01:00
Xavier Morel
f91629f693 [FIX] runbot_merge: v13 computed field compatibility
In v13, computed fields *must* have their value set on all records.
2020-01-13 08:47:45 +01:00
Xavier Morel
08f73ea3d3 [FIX] runbot_merge: convert priority field to a regular integer
In 13.0, the value of a selection field can't be an integer anymore.
2020-01-13 08:43:57 +01:00
Xavier Morel
27e9a4f9ee [FIX] runbot_merge: provide explicit labels on fields
Two fields can't have the same label, because of the field names,
there were field label conflicts.
2020-01-13 08:43:10 +01:00
Xavier Morel
0bdc824c2e [FIX] runbot_merge: incorrect name_get implementation
It's supposed to return a list of pairs, not a dict.
2020-01-13 08:42:25 +01:00
Xavier Morel
99d2d426eb [FIX] runbot_merge: api.multi decorator removed 2020-01-13 08:42:08 +01:00
Xavier Morel
a30a1c08e7 [FIX] runbot_merge, forwardport: missing model descriptions
Really can't be arsed to care, just want to remove the warning.
2020-01-13 08:41:54 +01:00
Xavier Morel
f23fb2a35b [FIX] runbot_merge: notification-via-deployment
Previous version incorrectly browsed the PR *number* (rather than ID)
so at best it would do nothing and at worst it might go and notify the
wrong PR entirely.
2019-11-25 11:29:45 +01:00
Xavier Morel
629e00a117 [IMP] runbot_merge: add related PRs to top comment
Discussing #238 with @odony, the main concern was the difficulty of
understanding if things merged in one repo were related to things
merged in an other repo: currently, knowing this requires going to the
merged PR, getting its label, and checking the PRs with the same HEAD
in the other repository to see if there's a correlation (e.g. PRs
merged around the same time).

The current structure of the mergebot makes it reasonably easy to add
the other PRs of the batch in the pseudo-headers, such that we get
links to all "related" PRs in the head commit (and links back from the
commits which is probably less useful but...)

Fixes #238
2019-11-22 09:21:40 +01:00
Xavier Morel
1b5a05e40c [FIX] mergebot: improve handling of having missed PR updates
1. if we try to stage a PR and realize we'd stored / checked the wrong
   head, cancel the staging and notify the PR
2. provide a command to forcefully update pr heads (or at least check
   that a PR's head is up to date)

Closes #241
2019-11-21 08:10:39 +01:00
Xavier Morel
8e67aed792 [IMP] runbot_merge: limit spamming on PR close
When closing a PR, github completely separates the events "close the
PR" and "comment on the PR" (even when using "comment and close" in
the UI, a feature which isn't even available in the API). It doesn't
aggregate the notifications either, so users following the PR for
one reason or another get 2 notifications / mails every time a PR
gets merged, which is a lot of traffic, even more so with
forward-ported PRs multiplying the amount of PRs users are involved
in.

The comment on top of the closure itself is useful though: it allows
tracking exactly where and how the PR was merged from the PR, this
information should not be lost.

While more involved than a simple comment, *deployments* seem like
a suitable solution: they allow providing links as permanent
information / metadata on the PRs, and apparently don't trigger
notifications to users.

Therefore, modify the "close" method so it doesn't do
"comment-and-close", and provide a way to close PRs with non-comment
feedback: when the feedback's message is structured (parsable as
json) assume it's intended as deployment-bound notifications.

TODO: maybe add more keys to the feedback event payload, though in my
      tests (odoo/runbot#222) none of the deployment metadata
      outside of "environment" and "target_url" is listed on the PR
      UI

Fixes #224
2019-11-21 08:10:39 +01:00
Xavier Morel
b853e5ba96 [IMP] runbot_merge: further validation of squash update on retargeting
It should have already been working, added an additional check for
update-then-retarget just in case but that worked out of the box. So
not sure why odoo/odoo#40106 failed.

Closes #256
2019-11-21 08:10:39 +01:00
Xavier Morel
8a47c57c3d [ADD] runbot_merge: logging of every GH request / response
If odoo is configured with a logfile, log to a separate file in the
same directory.

* log request / response when querying github
* log *received* requests for webhooks

Either way log the entire request metadata, though only the first 400
bytes/chars of the entity bodies.

This is intended to help mostly with post-mortem debugging: timestamps
from the main log can be correlated with the timestamps from the
github log in order to have more relevant information, both for
internal use and to send to gh support.

Closes #257
2019-11-21 08:10:39 +01:00
Xavier Morel
6f68db15d6 [IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.

This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).

The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.

The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:

1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
   base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:

   - the metadata of the original
   - the tree of the merge commit
   - the "current head" as parent

   then update the "current head" to that commit's ref'

If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).

As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.

As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.

This can probably be removed if / when odoo/runbot#247 happens.
2019-11-12 07:44:01 +01:00
Xavier Morel
7598d45283 [IMP] runbot_merge: use exponential backoff on head check
It's a waste to lose the entire staging if it's only a short blip /
delay thing, so retry multiple times. Add utility function to make
backoff functions easier (though the UI is not great ATM).

Also log the "left" parent of a merge commit (which should be the
"base") when creating it, for additional post-mortem information.
2019-11-07 10:03:54 +01:00
Xavier Morel
10ef6b82f0 [IMP] runbot_merge: sanity check PATCH git/ref/heads
Turns out not only can that operation fail, that operation can succeed
but have its effect delayed. To try and guard against that,
immediately check that we get the correct ref' after having reset it.

This is the cause of the November 6 mess: when preparing a staging,
the mergebot does the following,

1. get the head of <branch>
2. hard-reset tmp.<branch> to that
3. start merging PRs, which requires getting the current state of
   tmp.<branch> back

On the 6ths, these steps looked like this

```text
2019-11-06 10:03:21,588 head(odoo/odoo, master) -> ab6d0c38512e4944458b0b6f80f38d6c26b6b597
2019-11-06 10:03:22,375 set_ref(update, odoo/odoo, tmp.master, ab6d0c38512e4944458b0b6f80f38d6c26b6b597 -> 200 (OK)
2019-11-06 10:03:28,674 head(odoo/odoo, tmp.master) -> de2a852e7cc1f390e50190cfc497bc253687fba8
2019-11-06 10:03:30,292 head(odoo/odoo, tmp.master) -> de2a852e7cc1f390e50190cfc497bc253687fba8
```
So the 'bot fetched the commit at the head of master (ab6d0c), reset
tmp.master to that... and then got a different commit when it fetched
the tmp head to stage a PR on it.

That different head being of course a previous rejected staging. When
the new staging succeeded, it brought the entire thing in and made a
mess.

This was compounded by an issue I still have to investigate: the
staging of the new PR took the wrong base commit *but the right base
tree*, as a result the first thing it did was *reverse the entire
previous commit* (without that we could probably have left it as-is
rather than need to force-push master -- twice).
2019-11-07 07:52:12 +01:00
Xavier Morel
8f3f773eef [IMP] *: testing helpers
* add a sorted method on fake models
* fix recordset equality to ignore ids order
* when creating commits on a ref, add a param to only *update* the ref
  (forcefully): when simulating a force-push we don't want to *create*
  a ref as that might silently be done in the wrong repository entirely
* fix pytest.skip call at the module level, not sure where it came
  from and why I missed it until now
2019-10-14 10:09:48 +02:00
Xavier Morel
cc029c2891 [IMP] runbot_merge: add some logging to controllers
The closing or reopening of PRs was not logged at all, which can be
inconvenient when trying to find out why PRs are closed (or not) in
the backend.

Also leverage PR display_name improvements from
3ce3dd9569 for more regular PR names in
logs.
2019-10-14 10:09:48 +02:00
Xavier Morel
2971d042a4 [FIX] forwardport: only send notifications to the PR we're processing 2019-10-11 09:37:03 +02:00
Xavier Morel
3ce3dd9569 [IMP] forwardbot: show FP PRs in reminder message
When posting a reminder that there are open / waiting forward ports on
a source PR, also post *which* PRs those are.

While at it, move the cron code in a proper python file (so we can use
stuff from odoo.tools), and fix display_name so we can straight use
display_name as a github ref' ({owner}/{repo}#{number}). This impacts
log-grepping but it seems like an improvement nonetheless.

Closes odoo/runbot#228
2019-10-11 09:13:55 +02:00
Xavier Morel
036ae3a8ee [IMP] forwardbot: reduce length of fw branch name
* shorten the postfix, forwardbot is now a bigram!
* shorten the uniquifier: go from 5 to 3 bytes, and use urlsafe base64
  that way we only have a 4-char uniquifier instead of 8
* while at it, fix deprecated calls to logging.warn (should be
  logging.warning)

Fixes #226
2019-10-10 11:37:27 +02:00
Xavier Morel
d453943252 [IMP] *: unify gh test API between runbot and fw-bot
The fw-bot testing API should improve the perfs of mergebot tests
somewhat (less waiting around for instance).

The code has been updated to the bare minimum (context-managing repos,
change to PRs and replacing rolenames by explicit token provisions)
but extra facilities were used to avoid changing *everything*
e.g. make_commit (singular), automatic generation of PR refs, ...

The tests should eventually be updated to remove these.

Also remove the local fake / mock. Being so much faster is a huge
draw, but I don't really want to spend more time updating it,
especially when fwbot doesn't get to take advantage. A local /
lightweight fake github (as an external service over http) might
eventually be a good idea though, and more applicable (including to
third-parties).
2019-10-10 10:11:48 +02:00
Xavier Morel
fafa7ef437 [IMP] *: attempt to avoid some of the FP spam
Attempt to avoid some of the comment spam by dedup-ing input (only
signaling when the status actually changes and ignoring identity
transformations) and in case of failing CI keeping the last failed
status and not signaling on the next update if it's the same failure.

Closes #225
2019-10-07 16:38:14 +02:00
Xavier Morel
85035ad2c0 [IMP] runbot_merge: separate splits from other awaiting PRs
Should make the (eventual) wait and the extent of the splitting clearer.

Fixes #217
2019-10-07 09:26:27 +02:00
Xavier Morel
60c8f0f498 [FIX] runbot_merge: behaviour when no CI are required
If the required_statuses are empty, PRs should always be
validated (and just require a review) rather than never be merge-able.

Fixes #216
2019-10-04 10:00:14 +02:00
Xavier Morel
0152271fb8 [IMP] runbot_merge: layout responsivity
* only show 2 stagings on cellphones as 4 is way too much, moving to a
  vertical layout would probably be a bad idea as stagings can already
  be very tall and then we have multiple branches stacked on one
  another, unless we also make branches foldable

  the more complete list of stagings (per branch) is available on the
  branch's page anyway so providing a not-completely-broken home looks
  more useful, and at a fundamental level the current / last staging
  is really the one we care about
* remove the size bounds on stagings to avoid smushing all the cells
  together and overlapping text, sadly can't overflow scroll the
  stagings element because you can't have an overflow-x: scroll and an
  overflow-y: visible (that becomes auto)
2019-10-03 11:19:00 +02:00
Xavier Morel
0ae01c6ddb [FIX] runbot_merge: dropdown in branch staging history
Had apparently been fixed "live" but not committed in code, so every
update of the module would re-break it.

Fixes #221
2019-10-02 17:49:54 +02:00
Xavier Morel
e49b112447 [FIX] runbot_merge: only update pending staging state
The staging validation routine would ignore stagings which were
cancelled or ff_failed, but it should also have ignored failed and
successful aka all terminal state.

Simplify the condition for that: just ignore a staging's validation if
the staging is not pending.

Closes #211
2019-10-02 17:49:54 +02:00