Commit Graph

577 Commits

Author SHA1 Message Date
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
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
Xavier Morel
a5794a1a24 [FIX] forwardport: better version of previous fix
Turns out we don't want to close the cursor on success, we just want to
commit, but that's not what the default context manager does.

So don't use said context manager.
2019-10-01 09:57:35 +02:00
Xavier Morel
eb9eeb670a [IMP] forwardport: avoid locking cron when a _validate blows up
If a _validate call blows up, the entire Commit._notify cron gets
stuck, which is an issue because not only does it stop creating
forward ports, it also stops "progressing" stagings.
2019-10-01 07:56:24 +02:00
Xavier Morel
7659293a2b [IMP] runbot_merge: update staging timeout on 'pending' CI
If the CI is greatly backed up (either insufficient capacity or jobs
spike) a timeout which is normally perfectly fine might be
insufficient e.g. given a 2h timeout, if a job normally takes 80mn but
the staging's job starts 40mn after the staging was actually created
we're sunk. And cancelling the staging once the job has finally gotten
started is not going to improve load on the CI, it just wastes a CI
slot.

Therefore assume a `pending` event denotes the actual start of the job
on the CI, and reset the timeout to start from that moment so
ci_timeout is the timeout of the CI job itself, not of the staging
having been created.

Closes #202
2019-09-23 15:42:18 +02:00
Xavier Morel
78ad4b4e4b [IMP] runbot_merge, forwardport: consolidate conftests
Converge the pytest setups of runbot_merge and forwardport a bit
more (the goal is obviously to eventually share the infrastructure so
they run the same way).
2019-09-23 13:54:42 +02:00
Xavier Morel
66d65ba550 [IMP] runbot_merge, forwardport: variable-user feedback
Having all the feedback be sent by the mergebot user (github_token) is
confusing. Add a way to specify which field of project should be used to
source the token used when sending feedback.

Fixes #190
2019-09-21 15:23:42 +02:00
Xavier Morel
52699d901a [IMP] forwardport: ping on CI failure
It's especially important as users / assignees don't get
pinged *during* the forwardport process.

closes #203
2019-09-18 08:32:38 +02:00
Xavier Morel
f671dcc828 [ADD] forwardbot
* Cherrypicking is handrolled because there seems to be no easy way to
  programmatically edit commit messages during the cherrypicking
  sequence: `-n` basically squashes all commits and `-e` invokes a
  subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it
  interrupts the process before each commit), however there doesn't
  seem to be clean status codes so it's difficult to know if the
  cherrypick failed or if it's just waiting for a commit of this step.

  Instead, cherrypick commits individually then edit / rewrite their
  commit messages:

  * add a reference to the original commit
  * convert signed-off-by to something else as the original commit was
    signed off but not necessarily this one

* Can't assign users when creating PRs: only repository collaborators
  or people who commented on the issue / PR (which we're in the
  process of creating) can be assigned.

  PR authors are as likely to be collaborators as not, and we can have
  non-collaborator reviewers. So pinging via a regular comment seems
  less fraught as a way to notify users.
2019-09-05 10:00:07 +02:00
Xavier Morel
1b1aa637fe [IMP] runbot_merge: commit message edition abstraction
Prepares for more complex edition operations on the forwardbot side

* split out the pseudo-headers from the message body
* don't separate the co-authored-by headers from the others, seems
  unnecessary, we just need to ensure they're at the end so github
  doesn't miss them (/it)
2019-09-05 10:00:07 +02:00
Xavier Morel
ef24adad88 [FIX] runbot_merge: cancel button on staging
* split action_cancel (UI button) from cancel (internal): since the
  xhr mapping is weird, if there are available args the mapper thinks
  it should pass the call context as reason which is unexpected
* make cancel a no-op when called on already inactive stagings
* make cancel work when called on multiple statgings
* make computing the active staging work properly in an
  active_test=False context (e.g. when it's interacted with from the
  form view because that comes from the list view which is
  active_test=False, probably so we can see not just the stagings but
  recursively see deactivated batches in deactivated stagings)
* don't show the cancel button on inactive stagings
2019-08-27 12:28:53 +02:00
Xavier Morel
b1b959d472 [FIX] runbot_merge: properly handle retarget from deactivated branch
A deactivated branch is generally treated as unmanaged which is mostly
correct except for the case of retargeting an existing PR.

When a branch is deactivated the corresponding PRs are not removed, so
it's possible to have live PRs associated with ~unmanamaged
branches. When retargeting those PRs to active branches, the mergebot
would assume there was no existing PR and would create a duplicate,
then either get completely lost (before
a84595ea04) or blow up (after the same).

Properly search amongst deactivated branches for retargeting sources
so we update the relevant PR instead of trying to create duplicates.

Fixes #169
2019-08-27 11:27:02 +02:00
Xavier Morel
e40e814b90 [IMP] runbot_merge: show heads on stagings
Stagings have a "statuses" field which was shown but useless (as it's
a binary), they also have a "heads" field which only provides a
mapping of repository names to commits.

This change provides the staging heads as a commits m2m.

Fixes #178
2019-08-26 17:22:21 +02:00
Xavier Morel
28bcc6b5d7 [IMP] runbot_merge: refactor some bits
* extract method to create a PR object from a github result (from the
  PR endpoint)
* move some of the remote's fixtures to a global conftest (so they can
  be reused in the forwardbot)
2019-08-26 13:53:37 +02:00
xmo-odoo
02d85ad523
[FIX] runbot_merge: less restrictive commands matching
Fixes #167 ignores casing when matching bot name
Fixes #168 ignores leading whitespace when matching commands lines
2019-08-26 13:41:33 +02:00
xmo-odoo
8b74e79da9
[FIX] runbot_merge: FF error logging message
In case of error while fast-forwarding a staging to its source, we'd
log the target to which we couldn't FF. Sadly this relied on a
`repo_name` variable which (likely since the introduction of the
"safety dance" fast forwarding) can not actually be set in case of
failure.

So stash the relevant bit (the repo name) inside the FF error exception
and use that to compose our logging message instead of a variable which
can only be None.
2019-08-26 13:41:11 +02:00
Xavier Morel
222f591deb [IMP] runbot_merge: ACL & PR name
* add missing ACL for PR feedback object
* configure name_get for PRs (which don't have a name), fixes some
  layout issues & stuff
2019-08-26 13:22:29 +02:00
Xavier Morel
0bfb018e49 [IMP] runbot_merge: table for staging history
That way all staging labels (timestamps) have the same width, and PRs
/ batches being wrapped don't look like weird-named stagings.
2019-08-26 11:56:42 +02:00
xmo-odoo
c4b7604999
[ADD] runbot_merge: staging history per branch
Closes #175
2019-08-21 14:15:10 +02:00
xmo-odoo
a84595ea04
[FIX] runbot_merge: indexing of PR objects
Github constrains a single issue (/PR) number per repository, having
different targets does not allow two PRs to share a number.

Doesn't fix but should mitigate #169 slightly.
2019-08-21 11:21:06 +02:00
xmo-odoo
429257d013
[FIX] runbot_merge: resync tags on stage change
Before this change mergebot assumes github's tags are in sync with its
"previous" state, but because tags update was highly non-atomic (one
call per removal plus one for additions) and state can further change
between a failure and an update retry (especially as the labels endpoint
fails *a lot*), it's possible for set tags (in github) to be completely
desync'd from the mergebot state, leading to very misleading on-pr
indications.

This first fetches the current tagstate from github (to not lose non-
mergebot tags) then (hopefully atomically) resets all tags tags based on
the current mergebot state. This should avoid desyncs, and eventually
resync PRs (if they change state).

Fixes #170
2019-08-21 11:17:04 +02:00
Xavier Morel
9c9b312f8a [ADD] runbot_merge: staging dependencies 2019-08-09 14:31:21 +02:00
xmo-odoo
cfc7478fcf
[FIX] runbot_merge: PR splits should be updated on PR state change
On a PR being updated, closed or unreviewed, if it is part of an
active staging that staging would get cancelled (yay). However, if the
PR was part of a pending *split*, then the split would *not* get
cancelled / updated (to remove the PR from it), and the PR could go on
to get staged as if everything were right in the world which is an
issue.

It doesn't look like it actually happened (at least I got no echo of
it), but it almost did at least once.

fixes #160
2019-07-31 09:20:02 +02:00
xmo-odoo
6cb58a322d
[IMP] runbot_merge: send feedback when approving PR which failed CI
Also add test for it & feedback of an approved PR failing CI, and fix
corner case with it (might not send a warning immediately on CI failure
depending on status requirement ordering).

Fixes #158
2019-07-31 09:19:50 +02:00
xmo-odoo
85ac2e5d5e
[IMP] runbot_merge: map PR commits to integrated commits
* when rebasing, store a map of rebased to source, that way it'll be
  possible to link cherry-picked forward ports to the originally
  integrated commit rather than just the one from the PR (which was
  likely not itself integrated as the straight merge mode is somewhat
  rare: as of 5600 PRs merged so far only 100 were straight merged)
* while at it, store the "merge head" of the PR (whether squashed,
  merged or rebased) and put *that* in the commit message

fixes #161
2019-07-31 09:19:39 +02:00
xmo-odoo
955b97a023
[IMP] runbot_merge: p=1 > split
Allows merging a fix for e.g. a common false positive during a split
but without cancelling a staging which might just pass (you never
know).
2019-07-31 09:19:28 +02:00
Christophe Simonis
5e611f54cb [IMP] runbot_merge: allow sorting & deactivating branches
Closes #144, closes #145
2019-06-28 11:31:06 +02:00
Christophe Simonis
483fc5319a [IMP] runbot_merge: show failed PR on dashboard
Closes #138
2019-06-11 11:31:24 +02:00
Xavier Morel
76ea2a4f5d [FIX] runbot_merge: ensure co-authored-by are at the bottom
Sometimes people add co-authored-by lines in the middle of their
message, where github ignores them.

Since we previously added properly handling existing (correct) C-A-B
lines in the case where we're adding fixes and signed-off-by, we might
as well fix-up existing but mispalced co-authored-by lines.

Fixes #107
2019-05-07 13:22:13 +02:00
Xavier Morel
c9f9b3050b [FIX] runbot_merge: possibly missing commit object in try_splitting
Would trigger a TypeError when trying to json.loads(False).
2019-05-07 12:53:55 +02:00
Xavier Morel
eb91e2371b [FIX] runbot_merge: check CI status when reopening a PR
Previously, creating a PR would validate the head (in case it had
already passed CI) but reopening it would not, which is inconvenient
as the CI would not automatically run on a reopened PR.

Update both the state and the head of the PR on reopen to force a
revalidation, that way if the head has already passed CI the PR will
be reopened validated and there won't be an unclear need to perform an
explicit CI run.

Fixes #119
2019-05-07 12:32:02 +02:00
Xavier Morel
b59ffc68e0 [FIX] runbot_merge: handle the bot user not being able to comment
If the author of a PR has blocked the bot user, commenting on the PR
will fail. While comment failure is technically handled in the feedback
cron, the cron will simply retry commenting on every run filling the
log with useless unactionable garbage.

Retrying is the right thing to do in the normal case (e.g. changing tags
often has transient failures), but if we figure out we're blocked we
might as well just log a warning and drop the comment on the floor, it's
unlikely the situation will resolve itself.

Couldn't test it, because the block API is a developer preview and I
just can't get it to work anyway (404 on /user/blocks even providing the
suggested media type).

And the way the block is inferred is iffy (based on an error message),
the error body doesn't seem to provide any clean / clear cut error code:

    {
        "message": "Validation Failed",
        "errors": [
            {
                "resource": "IssueComment",
                "code": "unprocessable",
                "field": "data",
                "message": "User is blocked"
            }
        ],
        "documentation_url": "https://developer.github.com/v3/issues/comments/#create-a-comment"
    }

No useful headers either.

Fixes #127
2019-05-07 10:36:53 +02:00
xmo-odoo
4206d75256
[IMP] runbot_merge: wait for (and log) repo update / staging visibility
The race condition which prompted STAGING_SLEEP rears its ugly head
again: when pushing a base repo and its dependents, it's possible for
the update to the base repo's new head to take much longer to be visible
than the dependents (or so it seems?).

In this case, CI might pick up the correct dependent but pick an older /
incorrect revision of the base, leading to a staging failing for no good
reason.

This change uses info/refs to check for the updated staging head to be
visible at the repo level after it's been set / updated via the API. It
assumes repos are in topological order.
2019-04-29 12:42:54 +02:00
Xavier Morel
aa614c6077 [IMP] runbot_merge: more reliable blocked attribute
Use the proper / actual "is there any stageable PR" query to check if
a PR is blocked as well, that way they shoudn't be diverging all the
time even if it might make PR.blocked a bit more expensive.

fixes #111
2019-04-05 08:23:56 +02:00
Xavier Morel
e12e6db653 [IMP] runbot_merge: lock commit to update its status in hook
A status being updated on a commit is a read/modify/update, meaning
it's possible for somebody else (including a concurrent event?) to
concurrently update the commit and conflict leading to the webhook
blowing up, which is undesirable as it's a data loss (whereas if it
blows up on the other side e.g. in the cron's commit processor the
cron will just take it up next iteration).
2019-03-11 14:54:58 +01:00
Xavier Morel
f5d783eb4b [FIX] runbot_merge: error in template
8d011e03d2 contains poopy bits in the
template, fix them.
2019-03-11 14:51:15 +01:00
Xavier Morel
e0320664f9 [ADD] runbot_merge: sentry integration
Might eventually extract / generalise, but for now it's simpler to
just do it in runbot_merge's post_load, that way there's no setup
change (just a small bit of configuration), and it's only enabled on
the instances runbot_merge is installed on.

fixes #97, closes #103
2019-03-07 11:56:45 +01:00
xmo-odoo
4944d6a503
[FIX] runbot_merge: small typo in error message 2019-03-06 22:50:55 +01:00
Xavier Morel
8d011e03d2 [IMP] runbot_merge: styling of awaiting and blocked lists on dashboard 2019-03-06 09:46:57 +01:00
Xavier Morel
48e08b657b [IMP] runbot_merge: send feedback on CI failure following r+
Will comment any time a statuses update folds to a CI failure on a
reviewed pull request. Might be somewhat spammy, we'll see.

No notification if the PR is not reviewed yet.

fixes #87
2019-03-05 09:03:26 +01:00
Xavier Morel
5aa9f5a567 [IMP] runbot_merge: extract commit validation to cron
Before this, impacting a commit's statuses on the relevant PR or
staging would be performed immediatly / inline with its
consumption. This, however, is problematic if we want to implement
additional processing like #87 (and possibly though probably not #52):
webhook handlers should be kept short and fast, feeding back into
github would not be acceptable.

- flag commits as needing processing instead of processing them
  immediately, this uses a partial index as it looks like the
  recommended / proper way to index a boolean column in which one of
  the values is searched much more than the other (todo: eventually
  check if that actually does anythnig)
- add a new cron for commits processing
- alter tests so they use this new cron (mostly by migrating them to
  `run_crons` though not solely as some still need more detailed
  management to properly check intermediate steps)

Fix an issue with closing a staged PR while at it (the "merging" tag
would potentially never be removed).
2019-03-05 08:07:19 +01:00
Xavier Morel
360d0e17ca [IMP] runbot_merge: don't quote signoff
Proper RFC5322 makes for much noisier messages, and seems completely
unnecessary as examples of sign-off on the internet don't quote spaces
/ names.

closes #102
2019-03-04 13:17:10 +01:00
Xavier Morel
1f30af4345 [IMP] runbot_merge: dashboard clarity
* split out truly awaiting PRs from those waiting on an event of some
  sort
* if a staging is active but doesn't have a state yet, it should be
  considered pending not cancelled

closes #74
2019-03-04 12:11:34 +01:00
Xavier Morel
b699ea7f47 [FIX] runbot_merge: validate PRs on head update
If a PR gets sync'd to a known-valid commit, it should be marked as
valid rather than get in this weird state where it's merely open but
github knows it passes CI.

Fixes #72
2019-03-04 10:34:40 +01:00
Xavier Morel
1d2c264728 [FIX] runbot_merge: properly update squash flag on PR retarget
closes #82
2019-03-04 09:52:21 +01:00
Xavier Morel
8ab72ce8d1 [FIX] runbot_merge: gap in PR names in awaiting list
Repeatedly fixed it live, but apparently still forgot to fix it in the
source.
2019-03-01 21:34:31 +01:00
Xavier Morel
c693a7f841 [ADD] runbot_merge: button to manually cancel stagings
This is somewhat less useful with runbot's fail-fast as a runbot
failure (false positive or not) will now very quickly trigger an end
to the current staging.

Still, could be of use.

closes #89
2019-03-01 17:29:37 +01:00
Xavier Morel
eea3211f2b [IMP] runbot_merge: add logging to PR sync and reset error PRs to open
The choice to keep sync'd PRs in error means it's possible to update
the code and re-run the PR directly without it going through review &
CI again, which is a bit odd. Remove the special case and always reset
a sync'd PR to opened for clarity and simplicity.

closes #71
closes #83
2019-03-01 16:46:09 +01:00
Xavier Morel
c34e8ca083 [FIX] runbot_merge: race condition between closes #x and merging/FF
Turns out skipping locks is not very useful when there are no locks
being held because we only touch the PRs *after* the merge has been
applied.

So finally do that, lock all of a staging's PRs before we try to
fast-forward the relevant repositories, so a close command coming back
from github (from having seen the closes #xxx annotation) doesn't
screw us over.
2019-03-01 16:46:09 +01:00
Xavier Morel
0cd587fce7 [FIX] runbot_merge: don't blow the fetch loop when a PR has no label
No test because I don't understand how / why it's triggered, it's just
that some PRs don't have a label. I assumed the issue occurred when
the source branch or even repo (cross-repo PR) was deleted, but it
doesn't seem to trigger the issue (or in any case not in as short a
time as a test, maybe GH eventually does some vacuuming which causes
the issue?

Anyway we may eventually want to reclaim these PRs (allowing a lack of
label and treating them like the patch-\d labels: with no semantic
value) however the simplest thing to do for now is to just ignore the
corresponding PR.

closes #101
2019-03-01 16:42:58 +01:00
Xavier Morel
55ece42d8f [IMP] runbot_merge: delete repos being created in remote tests
In remote tests, if the deletion of a test repository fails (because
gh glitch) or the repo creation succeeded but reported a failure (for
some reason) the entire run is hosed because every test trying to
create a similarly named repository will explode.

Alter repomaker to just try to delete the repo, unless --no-delete
mode in which case just skip any further test trying to use the same
repository (not deleting the repo is the entire point of --no-delete,
as its purpose is the ability to do post-mortem debugging on
repository state).

closes #99
2019-03-01 16:42:57 +01:00
Xavier Morel
79b03a6995 [IMP] runbot_merge: retry FF on failure in case it's transient
Further improvements are possible, but that seems like a good
start (hopefully).

closes #94
2019-03-01 16:42:57 +01:00
Xavier Morel
42046cb21c [IMP] runbot_merge: logging on github requests failures
Github is subject to a fair amount of transient failures, which are
currently ill-logged: an exception is raised and the caller /
responsible might eventually log something, but it's not really
formalised and centralised, and is thus inconvenient to try and
post-mortem issues with github's support.

Change this such that *almost* all github API calls get extensively
logged (status, reason, all headers, body) on failure.

Also automatically sets debug logging for odoo in local tests, and
alter the fake response constructor thing so it doesn't set a json
mimetype when the body is not valid json.

Closes #98
2019-03-01 16:42:57 +01:00
Xavier Morel
f579b28a93 [FIX] runbot_merge: logging.warn is deprecated, use logging.warning 2019-02-28 13:50:25 +01:00
Christophe Simonis
19ffcdd4a2 [ADD] runbot_merge: sign off commits by reviewer
closes #50
closes #54
2019-02-26 13:36:46 +01:00
Xavier Morel
41cdc7e5f9 [IMP] runbot_merge: add number to PR search view
The number is probably the most common search criteria for PRs (to
track their status / issues). Having to go through custom filters to
find one is a pain in the ass.

Already done live by editing the view, but means it's getting lost
every time the module gets updated.

closes #73
2019-01-25 15:49:20 +01:00
Xavier Morel
4490c8f119 [IMP] runbot_merge: debug logging when rebasing 2019-01-25 15:45:38 +01:00
Christophe Simonis
e169934e61 [IMP]: runbot_merge: sort unready PRs alphabetically
The `_order` of pull requests are just the `number`.

Allow test `test_two_of_three_unready` to be reliable as both unready
PRs have the same number.
2019-01-25 15:45:12 +01:00
Christophe Monniez
549452f12d [IMP] runbot_merge: send feedback when merge method is changed
When a user changes the merge method via github messages, no feedback is
sent. This could lead to strange behavior, for example when a user try
to joke with the mergebot like this:

> robodoo are you goin ti merge my PR rogntudju !

This sets the merge method to "merge" and the user is not aware of it.
2018-12-13 13:28:20 +01:00
Xavier Morel
e01ad86171 [IMP] runbot_merge: make feedback more deterministic
Before this change, the order of PRs to list in an "unready" feedback
message was whatever the DB returned which could vary. This change
fixes the order by applying model order.
2018-12-12 15:29:59 +01:00
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
2f4a6f82ff [IMP] runbot_merge: quickfix to mark PRs as merging
Apparently the split and move arounds caused the _tagstate to get
computed/updated earlier (/differently), and thus the tagging update
relying on it to... not work anymore.

At least restore adding a "merging" tag when a PR is staged.
2018-10-17 16:55:53 +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
ea0d55a0f7 [IMP] runbot_merge: logging of set_ref
Actual flow was only partially logged, making post-mortem debugging harder than necessary
2018-10-17 12:45:54 +02:00
Xavier Morel
2521ac8439 [IMP] runbot_merge: dashboard template
Forgotten code-port of direct UI changes
2018-10-17 11:31:07 +02:00
Xavier Morel
fe8a4588d3 [FIX] runbot_merge: try to fix race condition again
Original issue (staging would get cancelled just as it was being
merged) was not really fixed but traded for a new one: serialization
errors which can lock up the mergebot for a long time, stopping
handling of all incoming signals (possibly/probably because all of
them try to write on the PR which is locked?).

Splitting the tagging cron out should already way improve things as
the status update cron should be way shorter (and thus hold its locks
for a smaller amount of time). This should also avoid the "close"
handler waiting on the extant transaction, and make the "pr update"
transaction be much shorter as each staging gets its own tnx.
2018-10-16 15:10:43 +02:00
Xavier Morel
eef9ede686 [IMP] runbot_merge: dashboard template, add awaiting PRs 2018-10-16 13:15:00 +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
41400f791b [FIX] runbot_merge: remove commit from message when closing a PR
As noted in the old comment, the provided commit was the head of the
staging branch. This confused many users as they'd click the link
expecting to see their commit and potentially got something completely
unrelated.

Since we already get backlinks from the commit through the "closes
<pr_number>" added either by the committer or by the bot itself, the
information is already available.
2018-10-12 16:56:25 +02:00
Xavier Morel
bab7e25a9a [IMP] runbot_merge: split the MegaCron into sub-methods 2018-10-12 16:24:35 +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
f238304c44 [IMP] runbot_merge: stagings views
* fix "Active" filter which was not updated when the active field was
  added
* properly enable it by default instead of relying on active_test
* disable active_test on the Stagings action, otherwise the batches
  are not visible in the staging once the staging and batches have been
  disabled
2018-10-12 13:46:22 +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
Xavier Morel
16c492ef0a [FIX] runbot_merge: merge() can be non-json in some cases (???)
Rather than blow up with a json error and take down the cron, convert
json decode error to a mergeerror in order to put the PR in error and
try to dump however much data we can.
2018-10-08 16:52:33 +02:00
Xavier Morel
dd29e6b8a8 [IMP] runbot_merge: attempt to fix race condition
Because mergebot cron can run on any runbot, it's apparently possible
that a staging gets merged and the "closed" feedback from github
overwrites the merged status which the mergebot is supposed to set
despite the supposed protection.
2018-10-08 16:52:33 +02:00
Xavier Morel
e590d565c1 [FIX] runbot_merge: cancel reason on PR close
Was not properly formatted, so the message would just be
"PR %s:%s closed by %s".
2018-10-08 15:31:07 +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
5ebb53cdc7 [IMP] runbot_merge: error logging on 422 responses from GH 2018-09-28 13:05:41 +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
ac2adfbdea [IMP] runbot_merge: only show mergebot configuration to mergebot admins 2018-09-22 16:10:54 +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
f3383daf60 [IMP] runbot_merge: remove ref update while squashing/rebasing
It should be unnecessary: creating commits directly does not update
the ref (hence 2b1cd83b07) and we're
forcefully setting the ref afterwards, either resetting it to the
original head (for rebase) or updating it to the commits we've just
created (for squash).
2018-09-21 13:53:09 +02:00
Xavier Morel
7a31099d72 [IMP] runbot_merge: looks of dashboard page 2018-09-21 12:19:53 +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
0d0140fad5 [IMP] runbot_merge: staging delay
Continuation of fa94b269de which is
apparently not sufficient:

1. log the staging event so we can check that we're staging in the
   correct order
2. add a delay after each staging in case there's some sort of race
   in the updating of codependent repositories
2018-09-20 16:42:35 +02:00
Xavier Morel
300f55d864 [FIX] runbot_merge: screwed up associativity of if/else 2018-09-20 16:23:27 +02:00
Xavier Morel
8a55407f87 [FIX] runbot_merge: like PRs, review bodies are null if left empty 2018-09-20 16:11:43 +02:00
Xavier Morel
fa94b269de [FIX] runbot_merge: order of staging branch updates
When creating staging branches from tmp, use the iteration order of
the repos in the project (that way it's easy to see and eventually
configure if we add sequences or whatever, in the short term it's the
order in which the repos were added which is the one we want).

This ensures we stage odoo/odoo before we stage odoo/enterprise
without relying on dict order of iteration, or needing meta to be an
OrderedDict.

The issue is if stagings are created/updated the other way around, the
runbot may pick up staging on odoo/enterprise before odoo/odoo has
been updated, and thus build odoo/enterprise with the wrong odoo/odoo
commit, and defeat the entire point of it.

Example: http://runbot.odoo.com/runbot/build/376112 was triggered by
the same staging as http://runbot.odoo.com/runbot/build/376113, but
used the previous staging head.

The creation order of tmp branches should not matter so ignore it.
2018-09-20 15:50: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
2b1cd83b07 [FIX] runbot_merge: forcefully update dest after non-reset rebase 2018-09-20 10:08:08 +02:00
Xavier Morel
3d8add9c11 [IMP] runbot_merge: add change of target branch between merges 2018-09-20 10:04:13 +02:00
Xavier Morel
58f1c34e3f [ADD] runbot_merge: logging of github operations 2018-09-20 09:25:13 +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
d3952de3d5 [IMP] runbot_merge: add logging to staging
I can't repro staging 13, so adding some logging.
2018-09-19 18:09:45 +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
2a7a3c6167 [FIX] runbot_merge: don't close PRs pointing to dummy commit
e98a8caffb added dummy commits to the
heads of stagings and fixed most places to make a difference between
the staging head (including dummy commit) and the actual merge head,
but the difference was missed in the comment closing a PR, which was
still using the staging head and thus pointing to the dummy commit
e.g. (https://github.com/odoo/odoo/pull/26821#issuecomment-420244592)
2018-09-11 13:53:41 +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
2a17bbec82 [FIX] runbot_merge: wrong model name in a view 2018-09-10 12:32:33 +02:00
Christophe Monniez
f9b057840a
[IMP] readme: add the Odoo workflow
Closes #34
2018-09-10 09:50:17 +02:00
Xavier Morel
47c3e752e9 [IMP] runbot_merge: add action for planned PR fetches 2018-09-03 17:53:43 +02:00
Xavier Morel
fd705d241a [IMP] runbot_merge: secret field & titles for o2m tables 2018-09-03 17:50:18 +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
2724e47633 [IMP] runbot_merge: readme updates 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
0afe8797f4 [IMP] runbot_merge: avoid unnecessary fetches
* avoid fetching PRs for un-managed branches if we know up-front
* avoid processing comments with no commands (avoids fetching the
  corresponding PR which we know nothing about yet and which may or
  may not be for a managed branch)
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
899ee4e2c4 [IMP] runbot_merge: error-reporting during PR sync 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
3b0a794c7b [ADD] runbot_merge: trivial dashboard 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
0725c77323 [IMP] runbot_merge: logging of comment commands (or lack thereof) 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
f67bdd8c5e [FIX] runbot_merge: incorrect relation name in M2O 2018-09-03 13:16:36 +02:00
Xavier Morel
4005eea72a [FIX] runbot_merge: github API error
Turns out PATCH /git/refs/:ref returns a 422 when the ref does not
exist, rather than the 404 I'd expected.

Also improve the error message by including the JSON body which tends
to be more descriptive/helpful than the reason for Github's API.

Maybe I should replace all raise_for_status() by printing the JSON
body instead...
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
xmo-odoo
7233179d6a [FIX] runbot_merge: only cancel active staging on p=0
Not when any priority is set on any PR…
2018-09-03 13:16:36 +02:00
Xavier Morel
9286b346e8 [FIX] runbot_merge: IDE warnings 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
47428a4580 [IMP] runbot_merge: only label PR if not created merged/closed
Would generate a ton of GH API churn when synchronising.
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
933559da04 [IMP] runbot_merge: repos shoud be unique
Having the same repository in two different projects makes no sense,
the two projects will conflict with one another.
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