Commit Graph

264 Commits

Author SHA1 Message Date
Xavier Morel
8a1b3466a7 [IMP] runbot_merge: send integratin failure comment via feedback queue
If a transient github failure makes the integration fail but also
makes the following reset fail the entire staging process would be
cancelled (and operations so far rollbacked) except for the failure
comment which would be effected, as in odoo/odoo#26380.

By pushing the comment to the feedback queue, if the reset fails the
comment is rollbacked and "unqueued".
2018-11-27 11:53:10 +01:00
xmo-odoo
e468d7116e
[FIX] runbot_merge: ignore comment edition & deletion
As well as review edition & dismissal.

Closes #53
2018-11-26 10:28:27 +01:00
xmo-odoo
6655e0ea5b
[ADD] runbot_merge: better integration controls
Closes #48
2018-11-26 10:28:13 +01:00
Xavier Morel
c814ce8f34 [FIX] runbot_merge: tagging on closed PR
Closed tagging was broken since the raw-sql alterations of the close
hook: because it's raw SQL, the write() method doesn't get invoked
anymore and as a result the tagging feedback record is not created,
and never executed.

Add a test to check for the PR's proper tagging, and fix this issue by
explicitly creating a tagging record.

Closes #49
2018-11-22 16:06:15 +01:00
Xavier Morel
46c460fd9b [IMP] runbot_merge: send comments asynchronously via feedback table
Hopefully this finally fixes the double commenting
issue (e.g. odoo/odoo#28160). This seems (according to reading the
logs and also logic) like a failure (concurrency of some sort) leading
to a transaction rollback *after* the GH API call, so the cron's DB
actions are rollbacked (then performed again on the next run) *but*
the GH API interactions are not rollbacked, and are thus performed
twice.

Since the entire purpose of the feedback table is to send comments,
send both "staging failed" and "Linked pull requests" comments via
that.
2018-10-29 09:42:26 +01:00
Xavier Morel
3c633cb70d [IMP] runbot_merge: concurrency issue with GH closing PRs being merged
Once more unto the breach, with the issue of pushing stagings (with
"closes" annotations) to the target branch making GH close the PR &
send the hook, which makes runbot_merge consider the PR closed and the
staging cancelled.

This probably still doesn't fix the issue, but it reduces the
problematic window: before this, the process first updates the
branches, then marks the PRs, then comments & closes the PRs, and
finally commits the PR update.

This means as runbot_merge is sending a comment & a status update to
each PR in a staging, GH has some time to send the "closed" webhook
behind its back, making the controller immediately cancel the current
staging, especially if the v3 endpoint is a bit slow.

By moving the commenting & closing out of the critical path (to the
feedback queue), this window should be significantly shortened.
2018-10-24 16:14:31 +02:00
Xavier Morel
e885db8a50 [FIX] runbot_merge: make local tests run in v11 as well
Runbot is supposed to run on v11, yet because the runbot_merge test
suite was built on v12 due to an API change in
Registry.enter_test_mode the (local) test suite would only run on v12.

Add a conditional pick such that the test suite can run transparently
on both v11 and v12.
2018-10-24 15:11:51 +02:00
Xavier Morel
6565a0f9a1 [FIX] runbot_merge: don't batch patch-X PRs across repos
Normally, two PRs from the same owner with the same branch
name (source) are batched together, and that's represented by batching
them by label e.g. two PRs labelled
`odoo-dev:12.0-snailmail-address-format-anp` means they probably
should be merged together somehow.

*However* this is an issue when editing via the web interface: if the
editor doesn't have write access to the repository, github
automatically createa a branch called `patch-<n>` under their
ownership, where `<n>` is a sequence (of sorts?) *within the user's
fork*.

This means it's possible (and indeed easy) to create <foo>:patch-1 on
different (non-forks) but related repositories without them actually
being co-dependent, at which point they get blocked on one
another, which can lead to them being blocked (period) due to runbot
not currently handling co-dependencies between PRs.
2018-10-23 17:39:31 +02:00
Xavier Morel
13f843a165 [IMP] runbot_merge: adds direct links to CI details to the dashboard
Currently, if a staging is ongoing or failed one has to hunt for the
staging branches on the runbot dashboard in order to find out what
happens.

This adds a dropdown to the staging box/block providing direct status
and access to all the CI information whether the CI is ongoing or done,
successful or not.
2018-10-23 17:39:30 +02:00
Xavier Morel
02dd03fca3 [IMP] runbot_merge: warning on re-reviewing a reviewed PR 2018-10-23 17:39:30 +02:00
Xavier Morel
be4c8bd491 [FIX] runbot_merge: forcefully rollback ref after a failed rebase()
rebase() can fail after merge(), during set_ref(), having already
updated the target.

Under the pre-rebase model, stage() assumed on a staging failure on a
given repo it only had to rollback stagings having succeeded. This
assumption fails in a post-rebase model as even a failed staging can
have modified the target, leading to the next staging (if multiple
batches are ready) containing the failed one.

Things can get really strange if the set_ref failure was (as it
probably is) some sort of transient failure, as the following staging
will likely succeed (under the assumption that most PRs/batches pass
staging) as PR1's content gets merged as part of PR2, and PR2 is
merged empty of content later on.
2018-10-17 14:18:49 +02:00
Xavier Morel
313d405a26 [ADD] runbot_merge: various feedback messages
Send reponse comments when users mis-interact with robodoo e.g.
send comments they don't have the right to, or commands which don't make
sense in the PR's state, or tentative interactions with robodoo from
unmanaged PRs.
2018-10-16 12:40:45 +02:00
Xavier Morel
0b629a32bc [ADD] runbot_merge: warning that a ready PR is linked to a non-ready one
People get surprised/worried that their ready PR never gets picked up,
but it's because there is a non-ready (either unreviewed or failing CI)
pull request linked to it. This aims to at least warn them of the issue.
2018-10-15 16:19:29 +02:00
Xavier Morel
63b7484873 [IMP] runbot_merge: helper for creating branches in multirepo tests
Taking 3 statements to create a branch before working with it is a bit
much, a simple helper reduces that to a single function call with 4
params (from 3 function calls with 1/4/1 params).
2018-10-15 12:52:36 +02:00
Xavier Morel
5ec2c12454 [IMP] runbot_merge: split tagging cron out of main progress cron
They're completely independent (or should be), and there's no reason
for the tagging cron to extend the "lifetime" of the main cron's
transaction (and thus extend the odds of racey behaviour).
2018-10-12 15:54:14 +02:00
Xavier Morel
af8c62e4ad [FIX] runbot_merge: better handle targets being branch-protected
If a staging covers multiple repositories and there's a fast-forward
issue on any but the first repo/target, runbot_merge attempted to
revert the commits it had fast-forwarded on the previous repos.

This doesn't work when branch-protection is active, unless runbot_merge
is a repository administrator (and branch protection is not configured
to apply to those): reverting is done by push-forcing the original head
back onto the ref, which branch-protection unconditionally precludes.

This commit does not entirely fix the race condition (it does not look
like github provides any way to do that), but it should significantly
reduce the race-condition window as it performs a semi-wet run of the
fast-forward process on the tmp branches before actually updating the
targets. That way the only remaining breakage should be when somebody
pushes on repositories 1.. between the test-FF on tmp branches and the
actual fast forward.

While at it, updated the github API thing to *always* dump the JSON body
on an error response, if the content-type is json.
2018-10-10 10:50:21 +02:00
Xavier Morel
a1384b3868 [FIX] runbot_merge: iterate commits in topological order
Previously, runbot_merge assumed github would return commits in
topological order (from base to head of PR). However as in the UI
github sorts commits using the author's date field, so depending on
rebasing/cherrypick/... it's possible to have the head of the commit
be "younger" than the rest. In that case robodoo will try to merge
it *first*, then attempt to merge the rest on top of it (-ish, it'd
probably make a hash of it if that worked), at which point github
replies with a 204 (nothing to merge) because the PR head has already
included everything which topologically precedes it.

Fix: re-sort commits topologically when fetching the PR's log. That
way they're rebased in the proper order and correctly linked to one
another.

Example problematic PR: odoo/enterprise#2794, the commits are

773aef03a59d50b33221d7cdcdf54cd0cbe0c914
    author.date: 2018-10-01T14:58:38Z
879547c8dd37e7f413a97393a82f92377785b50b (parent: 773aef03)
    author.date: 2018-10-01T12:02:08Z

Because 879547c8 is "older" than 773aef03, github returns it first,
both in the UI and via the API.

Also fixed up support for committer & author metadata in fake_github
so the local tests would both expose the issue properly and allow
fixing it.
2018-10-09 15:08:13 +02:00
xmo-odoo
d042bc541f Better reporting of staging state
* [ADD] runbot_merge: more informative states to stagings on error

Currently, when a staging fails for other reasons than a CI failure:

* the staging having been cancelled is known implicitly, because the
  staging will be deactivated but will never get a status beyond
  pending (because it's not found when looking for it since it's not
  `active`)
* the fast-forward having failed is completely silent (logging aside),
  it looks for all the world like the staging succeeded

Timeout fails the PR already, but split-on-timeout was not so fix that
one bit.

* [FIX] odoo/odoo#cb2862ad2a60ff4ce66c14e7af2548fdf6fc5961

Closes #41
2018-10-01 10:21:32 +02:00
Xavier Morel
c687e9ae8b [IMP] runbot_merge: support @ and # prefixes to delegate= logins 2018-09-25 16:42:56 +02:00
Xavier Morel
359a3cf872 [ADD] runbot_merge: support for r- on approved & ready PRs
Can be used by the PR author or a reviewer. Removes reviewed state of
PR, and cancels the staging if the PR is already staged.
2018-09-25 15:04:31 +02:00
Xavier Morel
6df9a68af2 [CHG] runbot_merge: treat github reviews as regular comments
Treating them specially turns out to be inconvenient: it becomes
harder/riskier to ask for informal reviews (for which GH reviews are
useful).
2018-09-25 14:05:41 +02:00
Xavier Morel
7fc7b78a04 [FIX] runbot_merge: security concern
The webhook used the "sender" of the event as comment author, however
if the comment is edited by a maintainer github sends a
"issue_comment" event with that maintainer as sender.

This means a random user could create a comment with a robodoo
command, and if a registered reviewer happened to edit the comment the
command would suddenly be taken in account. This was not the intention.
2018-09-24 10:06:58 +02:00
Xavier Morel
20bb058f06 [FIX] runbot_merge: make staging cancels more greppable
I just spent 10mn trying to find out why staging 28 was cancelled
(a p=0 comment). Add a common prefix to all staging cancels to make
them easier to find.
2018-09-21 15:58:30 +02:00
Xavier Morel
e6a1a0634a [FIX] runbot_merge: re-enable staging delay
staging delay was mistakenly commented in
bb664455ec

Also modified testing fixtures so the staging delay is not enabled when
running tests locally: on my box it increases the local runtime from
~70s to ~1500s (20s/staging, ~1 staging/test, 73 tests)
2018-09-21 15:57:16 +02:00
Xavier Morel
bb664455ec [IMP] runbot_merge: allow invoking bot as @bot and #bot
Before this, the bot would only acknowledge commands of the form

    <botname>: <commands>

but since the bot is an actual user, people regularly use `@<botname>`
as it seems like it should work *and* provides for autocompletion.

Support that, as well as the octothorpe in case users want to pound
robodoo.

Related to odoo/runbot#38
2018-09-21 12:19:53 +02:00
Xavier Morel
3885ca244c [FIX] runbot_merge: handling of PRs of >50 commits
A limitation to 50 commits PRs was put in place to avoid rebasing
huge PRs (as a rebase means 1 merge + 1 commit *per source commit*),
however the way it was done would also limit regular merges, and the
way the limitation was implemented was not clear.

* explicitly check that limit in the rebase case
* make error message on PR sizes (rebase 50 or merge 250) clearer
* remove limit from commits-fetching (check it beforehand)
* add a test to merge >50 commits PRs
* fix the local implementation of pulls/:number/commits to properly
  paginate
2018-09-20 15:28:55 +02:00
Xavier Morel
b62afb7673 [ADD] runbot_merge: test for previous commit
2b1cd83b07 fixed a bug in PR
squashing (introduced when it was mis-rebuilt on top of rebase) which
was immediately committed & pushed so we could fix the running
mergebot.

This adds a test for that issue, it was checked to fail for
2b1cd83b075a99da7ed905b9e62d7e5acb48b253~1 and work as of the current
head.

Turns out the previous tests checked all the new/complex features to
see if they worked correctly, but I completely forgot that the
previously working squash had been rebuild.
2018-09-20 10:52:58 +02:00
Xavier Morel
3db55a849f [ADD] runbot_merge: one more test, still doesn't repro staging 13 2018-09-19 18:49:52 +02:00
Xavier Morel
6dbfde0389 [ADD] runbot_merge: add tests to try and expose the mystery of staging 13
Staging 13 tried merging 3 PRs (27085, 27083 and 27071) and supposedly
succeeded *but* only merged one of the 3 PRs despite marking all three
as merged. I tried building a few tests constructing multi-PR graphs
and checking them, but the only thing they exposed was the local
github implementation not correctly updating merge targets.

So fixed that, which is good.

Doesn't tell me why the staging didn't work right though.
2018-09-19 17:33:25 +02:00
Xavier Morel
c2db5659d8 [FIX] runbot_merge: handle PRs with no body (not just empty) 2018-09-19 14:40:36 +02:00
Xavier Morel
7310cd1f1d [IMP] runbot_merge: link to failed runbot builds
a0063f9df0 slightly improved the error
message on non-PR ci failure (e.g. a community PR makes enterprise
break) by adding the failed commit, but that's still not exactly clear,
even for technical users (plus it requires having access to all the
repos which is not the case for everyone).

This commit further improves the situation by storing the target_url
and description fields of the commit statuses, and printing out the
target_url on failure if it's present.

That way the PR comment denoting build failure should now have a link to
the relevant failed build on runbot, as that's the target_url it
provides.

The change is nontrivial as it tries to be compatible with both old and
new statuses storage format, such that there is no migration to perform.
2018-09-17 11:04:31 +02:00
Martin Trigaux
8f7a5e55ef [FIX] runbot_merge: avoid double closes message
If the message already contains the pr number, no need to add it again
2018-09-13 10:30:47 +02:00
Xavier Morel
a0063f9df0 [IMP] runbot_merge: provide more useful message on some staging failures
If CI fails on a non-PR'd branch of a staging (e.g. given repos A and B
and a PR to A, CI fails on the staging branch to B), the error message
(log and comment on the PR) is unhelpful as it states that the staging
failed for "unknown reason".

Improve this by providing the failed CI context and the commit, which
should allow finding out the branch & CI logs, and understanding the
why of the failure.

Fixes #36
2018-09-11 10:21:24 +02:00
Xavier Morel
e98a8caffb [FIX] runbot_merge: ensure all staging branches are built/tested
Before this change, when staging batches only affecting one repo (of n)
the unaffected repositories would get a staging branch exactly matching
the target.

As a result, either runbot_merge or runbot would simply return the
result of an unrelated build, potentially providing incorrect
information and either failing a staging which should have succeeded
(e.g. change in repo A broke B, PR is making a change in repo A which
fixes B, but B's state is reported as the previous broken build) or
succeeding a staging which should have failed (change in repo A breaking
B except a previous build of the exact same B succeeded with a different
A and is returned).

To fix this issue, create a dummy commit at the head of each staging
branch. Because commit dates are included in the hash and have a second
precision it's pretty unlikely that we can get built duplicates, but
just to be completely sure some random bits are added to the commit
message as well.

Various tests fixed to correctly handle the extra dummy commit on
staging branches.

fixes #35
2018-09-11 10:21:24 +02:00
Xavier Morel
ec5d60d027 [CHG] runbot_merge: unapprove on PR update
After discussion with mat, rco and moc, if a PR is updated it should
be unapproved for safety reasons: if a reviewer approves a PR, that's
what should be merged, if there are things to fix/change a reviewer
should at least rubberstamp the changes to avoid mistakes.

This is a bit more noisy/constraining, but can be changed or tuned
afterwards if it's considered too constraining.
2018-09-03 13:55:39 +02:00
Xavier Morel
175649cd13 [IMP] runbot_merge: test readme & remove leftover pass 2018-09-03 13:16:36 +02:00
Xavier Morel
a40b4c20da [ADD] runbot_merge: flag to disable rebase before merge
rebase-and-merge (or squash-merge if pr.commits == 1) remains default,
but there are use cases like forward ports (merge branch X into branch
X+1 so that fixes to X are available in X+1) where we really really
don't want to rebase the source.

This commits implements two alternative merge methods:

If the PR and its target are ~disjoint, perform a straight merge (same
as old default mode).

However if the head of the PR has two parents *and* one of these
parents is a commit of the target, assume this is a merge commit to
fix a conflict (common during forward ports as X+1 will have changed
independently from and incompatibly with X in some ways).

In that case, merge by copying the PR's head atop the
target (basically rebase just that commit, only updating the link to
the parent which is part of target so that it points to the head of
target instead of whatever it was previously).
2018-09-03 13:16:36 +02:00
Xavier Morel
6d7c728471 [CHG] runbot_merge: toggle default merge method to rebase-and-merge
After discussion with al & rco, conclusion was default PR merging method
should be rebase-and-merge for cleaner history.

Add test for that scenario (w/ test for final DAG) and implement this
change.
2018-09-03 13:16:36 +02:00
Xavier Morel
63be381453 [FIX] runbot_merge: testing of batch contents
* Add ids accessor to the remote Model fake
* Explicitly ignore order when unnecessary, a test fails since the
  ordering of prs has been changed for UI purposes. This is only an
  issue for Remote though it's unclear why (as the local Issue/PR
  objects should still have a per-repo sequence)
2018-09-03 13:16:36 +02:00
Xavier Morel
a15ab1c481 [FIX] runbot_merge: work with admin[uid=2]
* correctly fetch the admin's uid when running remote tests
* looks like I had forgotten to add any sort of access rights, so add
  some
2018-09-03 13:16:36 +02:00
Xavier Morel
cab683ae0f [IMP] runbot_merge: remove "sync PRs", fetch unknown PRs
The old "sync pr" thing is turning out to be a bust, while it
originally worked fine these days it's a catastrophe as the v4 API
performances seem to have significantly degraded, to the point that
fetching all 15k PRs by pages of 100 simply blows up after a few
hundreds/thousands.

Instead, add a table of PRs to sync: if we get notified of a
"compatible" PR (enabled repo & target) which we don't know of, create
an entry in a "fetch jobs" table, then a cron will handle fetching the
PR then fetching/applying all relevant metadata (statuses,
review-comments and reviews).

Also change indexation of Commit(sha) and PR(head) to hash, as btree
indexes are not really sensible for such content (the ordering is
unhelpful and the index locality is awful by design/definition).
2018-09-03 13:16:36 +02:00
Xavier Morel
76c4d24bf5 [IMP] runbot_merge: don't create unstaged stagings
Previously when splitting staging we'd create two never-staged
stagings. In a system where the stagings get deleted once done
with (succeeeded or failed) that's not really important, but now that
we want to keep stagings around inactive things get problematic as
this method gunks up the stagings table, plus the post-split stagings
would "steal" the original's batches, losing information (relation
between stagings and batches).

Replace these empty stagings with dedicated *split* objects. A batch
can belong to both a staging and a split, the split is deleted once a
new staging has been created from it.

Eventually we may want to make batches shared between stagings (so we
can track the entire history of a batch) but currently that's only
PR-level.
2018-09-03 13:16:36 +02:00
Xavier Morel
8bc90283f8 [IMP] runbot_merge: stop deleting batches & stagings
If we want a dashboard with a history of stagings, maybe not deleting
them would be a good idea.

A replacement for the headless stagings would probably be a good idea:
currently they're created when splitting a failed staging containing
more than one batch, but their only purpose is as splits of existing
batches to be deactivated/deleted to be re-staged (new batches &
stagings are created then as e.g. some of the batches may not be
merge-able anymore) and that's a bit weird.
2018-09-03 13:16:36 +02:00
Xavier Morel
6494ea6cb0 [ADD] runbot_merge: webhook signature support 2018-09-03 13:16:36 +02:00
Xavier Morel
35f33cee6d [FIX] runbot_merge: a few warnings 2018-09-03 13:16:36 +02:00
Xavier Morel
323aa20053 [ADD] runbot_merge: remote github plugin 2018-09-03 13:16:36 +02:00
Xavier Morel
b35a24ca62 [FIX] runbot_merge: undesired merge conflict
Not quite sure why local didn't hit it but don't really care either.
2018-09-03 13:16:36 +02:00
Xavier Morel
50b9943d78 [FIX] runbot_merge: review action is APPROVE
Despite the review *state* being APPROVE*D*.

Fix test & handling in mock github.
2018-09-03 13:16:36 +02:00
Xavier Morel
3de8262b50 [REM] runbot_merge: tests of updating a merged or closed PR
Github apparently doesn't sync merged/closed PRs (which makes sense
but isn't really documented) so strip out test and assume that never
happens (with a log error in case it ever does).
2018-09-03 13:16:36 +02:00
Xavier Morel
7451a94358 [IMP] runbot_merge: PR source branch/label handling
Remote's labels are not entirely under our control as the part before
":" is the *owner* of the source repo => introduce additional "owned"
fixture to handle this case, as it may diverge from the "user" role if
running the tests against an organisation.
2018-09-03 13:16:36 +02:00
Xavier Morel
381c504584 [IMP] runbot_merge: logins -> user roles
Can't really assume we can get the github logins "user" or "reviewer"
to run the test suite remotely, so add an indirection and backronym
those to *roles* instead. The local test suite has identical roles &
logins, but the remote version does not.

Also use the "other" role for any random user, and don't create its
partner up-front.

Also renamed the self-reviewer user to self_reviewer, that's a bit
less weird when dealing with e.g. ini files.
2018-09-03 13:16:36 +02:00
Xavier Morel
91937aac27 [FIX] runbot_merge: incorrect changes spec/order (depth) 2018-09-03 13:16:36 +02:00
Xavier Morel
d98d40389d [IMP] runbot_merge: pluginify github mock
This is the preparation of an attempt to make these tests work with
both a local github mock (in-memory) and a remote actual github.

Move a bunch of fixtures relying on the specific github
implementation (and odoo-as-library access) to the "local" plugin,
including splitting the "repo" fixture.

The specific fixtures will likely have to be adjusted as the
remote endpoint is fleshed out.
2018-09-03 13:16:36 +02:00
Xavier Morel
5e64f79730 [ADD] runbot_merge: update squash flags when PR commits are updated
If the PR's commits count changes, the squash state should be updated
to match/reflect.
2018-09-03 13:16:36 +02:00
Xavier Morel
df2d19302b [REM] runbot_merge: support for squash+/squash-
AL thinks it's not useful and it's better to always squash/rebase a
single commit & merge multiple. Mark tests as xfail'd instead of
removing them.

Also mark test_edit_retarget_managed as skipped explicitly
2018-09-03 13:16:36 +02:00
Xavier Morel
b30bb09319 [FIX] runbot_merge: when closing a staged PR, cancel the staging 2018-09-03 13:16:36 +02:00
Xavier Morel
34497a91b4 [FIX] runbot_merge: don't try to stage urgent PRs in error 2018-09-03 13:16:36 +02:00
Xavier Morel
39a0d723af [ADD] runbot_merge: tag PR to signify state changes 2018-09-03 13:16:36 +02:00
Xavier Morel
7033952913 [ADD] runbot_merge: reviews support
Reviews are interpreted like comments and can contain any number of
commands, with the difference that APPROVED and REQUEST_CHANGES are
interpreted as (respectively) r+ and r- prefixes.
2018-09-03 13:16:36 +02:00
Xavier Morel
49c8fdbed2 [IMP] runbot_merge: prioritize p0 more
* p0 cancel existing stagings in order to be staged as soon as
  possible
* p0 PRs should be picked over split batches
* p0 bypass PR-level CI and review requirements
* p0 can be set on any of a batch's PR, matched PRs will be staged
  alongside even if their priority is the default
2018-09-03 13:16:36 +02:00
Xavier Morel
e6f5b84a19 [IMP] runbot_merge: add PR ID to commit message when merging 2018-09-03 13:16:36 +02:00
Xavier Morel
ea9203c359 [IMP] allow delegate reviewers to retry and r- 2018-09-03 13:16:36 +02:00
Xavier Morel
781b679648 [FIX] runbot_merge: de-f-string-ify 2018-09-03 13:16:36 +02:00
Xavier Morel
e52d08ecdf [ADD] runbot_merge: a merge bot
No actual dependency on runbot, should be usable completely
independently.
2018-09-03 13:16:36 +02:00