Commit Graph

229 Commits

Author SHA1 Message Date
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
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