Commit Graph

253 Commits

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