Commit Graph

90 Commits

Author SHA1 Message Date
Xavier Morel
8e8a238a66 [FIX] forwarport: mark branches as fp-targets by default
otherwise the freeze wizard gets very confused
2022-02-07 08:10:03 +01:00
Martin Trigaux
ede5384607 [IMP] forwardport: add missing space
Instead of "[FW]Create README.md", the forwardport PR is now named
"[FW] Create README.md"
2021-12-03 16:03:08 +01:00
Xavier Morel
08c4e94ca9 [FIX] forwardport: decoration of PullRequests.create
Apparently multi is the default, so the method not being decorated
blows up when creating fake PRs through the interface.
2021-11-12 16:03:28 +01:00
Xavier Morel
6c60d59b11 [IMP] forwardport: hall of fame layout and informations
The list of outstanding forwardports was pretty messy as the ordering
was unclear and there was little way to really drill into the thing.

* Shows outstanding forward ports sorted by merged date ascending, the
  oldest-merged PRs are the ones most in need of fixing while PRs
  which were only just merged can safely be ignored.
* List reviewers with outstanding forward-ports, allow filtering by
  clicking on their name, allow deseleting through the subtitle of the
  page.
* Don't display reviewer in list when page is already filtered by
  reviewer.

Also improve PR page a bit:

* Add reviewer.
* Add direct link to backend (closes #524).

Closes #529
2021-10-20 15:16:35 +02:00
Xavier Morel
1dcbb4a5ac [FIX] forwardport: allow delegates on fw PRs to review followups
The forward-port process currently automatically adds delegates of a
PR as delegates on its forward ports, but that only works for
the *source* pull request.

If a delegate is added to a forward-port, they were not able to
approve the followups to that initial port, which makes little sense.

Fixes #548
2021-10-20 14:36:50 +02:00
Xavier Morel
947bf3bb47 [FIX] forwardport: don't re-approve approved PRs during an fw-bot r+
When using the forwardport's shortcut, the bot would not skip
already-approved PRs leading to a warning from the mergebot that the
PR was already approved (out of nowhere which was weird).

During the walk to the ancestors, skip any PR which is not
approvable (either already approved or in a state where that makes no
sense e.g. closed).

Fixes #543
2021-10-20 14:36:50 +02:00
Xavier Morel
bf34e9aa95 [FIX] runbot_merge: ensure PR description is correct on merge
Because sometimes github updates are missed (usually because github
never triggers it), it's possible for the mergebot's view of a PR
description to be incorrect. In that case, the PR may get merged with
the wrong merge message entirely, through no fault of the user.

Since we already fetch the PR info when staging it, there's very
little overhead to checking that the PR message we store is correct
then, and update it if it's not. This means the forward-port's
description should also be correct.

While at it, clean the forward port PR's creation a bit:

- there should always be a message since the title is required on
  PRs (only the body can be missing), therefore no need to check that
- as we're adding a bunch of pseudo-headers, there always is a body,
  no need for the condition
- inline the `pr_data` and `URL`: they were extracted for the support
  of draft PRs, since that's been removed it's now unnecessary

Fixes #530
2021-09-24 15:18:36 +02:00
Xavier Morel
678d2216b8 [IMP] forwardport: provide clearer picture of conflicts
On conflicts in multi-commit PRs developers sometimes get confused as
to what happened why.

If a conflict occurs and the source pull request had multiple
commits, list all the source commits and show which one broke.

Related to #505
2021-08-24 15:39:47 +02:00
Xavier Morel
82174ae66e [IMP] *: add draft support to mergebot, kinda
* Remove the forwardport creating PRs in draft, that was mostly to
  avoid codeowners triggering but we've removed the github one and
  hand-rolled it, so not a concern anymore.
* Prevent merging `draft` PRs, the mergebot rejects approval on draft
  PRs and insults people.

TBD (maybe): try to create *conflicting* forward-port PRs in draft so
it's clearer they need to be *fixed*? Issue of not being able to do
that on all private repositories remains so~~

Fixes #500
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
d249417ceb [FIX] forwardport: fix deduplication of authorship in multi-pr conflict
a45f7260fa had intended to use the
original authorship information for conflict commit even if there were
multiple commits, as long as there was only one author (/ committer)
for the entire sequence.

Sadly the deduplication was buggy as it took the *authorship date* in
account, which basically ensured commits would never be considered as
having the same authorship outside of tests (where it was possible for
commits to be created at the same second).

Related to #505
2021-08-24 15:39:47 +02:00
Xavier Morel
32829cf880 [FIX] forwardport: missing login in feedback message
Closes #503
2021-08-24 15:39:47 +02:00
Xavier Morel
670f56b491 [ADD] forwardport: views of outstanding forwardports
Though the forwardport posts regular reminders that an fw is outdated,
it can be easy to miss for the non-subject (and apparently the
subjects often just ignore the information entirely).

Add a few relevant links there:

* on PR pages, add a link to either the source or the
  forward-ports (if applicable), as well as the merge date
* add a new page which lists all the PRs with outstanding
  forwardports, as well as the forwardports in question

Fixes #474
2021-08-24 15:39:47 +02:00
Xavier Morel
ca2742a12c [IMP] forwardport: error handling when creating PR
Don't try to parse the response as JSON in the error case(s): if the
errors are bad enough github can return complete non-parseable
garbage.

Only access the "text" property (response body, decoded, but unparsed)
in error cases, only parse in the success case.

Also avoid reusing variables for completely different values, even if
they're of the same type, especially if they can overlap.

fixes #470
2021-08-24 15:39:47 +02:00
Xavier Morel
8178b64c01 [IMP] forwardport: error message when trying to r+ via fwbot
Initial thinking was to remove the check entirely and leave it to the
mergebot, but the lack of error reporting / forwarding means while
technically correct it would probably be somewhat difficult to grok.

Instead, improve the error reporting:

* add a dedicated message when trying to r+ via fwbot on a non-fw
  PR (note: maybe the fwbot should not care? and just send it as-is
  to the mergebot in that case?)
* clarify the ACL error
* post both message as the forwardbot rather than the mergebot

Also add a missing token note for the feedback from the forwardport
limit.

fix #469
2021-08-24 15:39:47 +02:00
Xavier Morel
f10d33ee85 [FIX] fwbot: properly prevent @up to on forward-port PRs
There was already a check, but the way the check behaved
means *detached* PRs would not be prevented from setting their
forward-port, despite that not doing anything.

Fix it by checking if the current PR has a source, not a parent.

Fixes #465
2021-08-24 15:39:47 +02:00
Xavier Morel
8a924fb4b7 [FIX] forwardport: duplicates forwardport on edition
On edition of an intermediate PR in a chain, merging the PR would lead
to *it* being forward-ported, duplicating the PRs already created
from *its* source.

Add a check for PRs in the target branch with the same source,
suppresses the forward-porting of the newly merged PR.

Fixes #451 (hopefully)
2021-03-02 14:28:32 +01:00
Xavier Morel
952dafa45c [IMP] forwardport: feedback on conflict
append `git status` data to stderr, should be somewhat more
informative especially when a conflict is a DU (where the file has
been deleted on one side, so there is no conflict marker anywhere).

Fixes #461
2021-03-02 14:28:32 +01:00
Xavier Morel
a5f2d14707 [CHG] forwardport: try to create PRs in draft mode
Fall back to creating in not-draft mode if that doesn't work: draft
pull requests on private repositories requires the Team plan.

Closes #459
2021-03-02 14:28:32 +01:00
Xavier Morel
9c1585383a [IMP] forwardport: git commands logging
- When updating the local repo cache, always capture both stdout and
  stderr and log them out rather than having them in journalctl hard
  to relate to the main log.
- In the git layer, capture stderr by default and log it automatically
  on command failure.
2021-03-02 14:28:32 +01:00
Xavier Morel
b0576d1d41 [IMP] forwardport: cherrypick logging
Enable GIT_TRACE to try and get more information pertaining to the
cherrypicking process during failure.

Related to #449
2021-01-18 07:44:17 +01:00
Xavier Morel
1b7040cd26 [IMP] forwardport: logging during cherrypick process
* fix small error which generated an extra commit in case of conflict
  probably (would create a `temp` commit, then put the conflict
  information on an empty commit on top of it). Avoids committing the
  cherrypick though a probable alternative would be to commit the
  message with `amend`.
* improve the cherrypick header: instead of one log line per commit,
  put all commits within the sole header log line (with a newline),
  should make things less noisy
* put *all* log records below the correct logger (two of them were on
  the toplevel logger)
* log a purported inflateInit error as warning, should be sent to
  Sentry instead of having to wait for people to wonder why the thing
  is completely broken
2021-01-15 14:19:17 +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
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
d751cf46a0 [IMP] forwardport: add logging to branch reinsertion
When a new branch is created between other branches, the process to
try and look for forward-port PRs to create to "fill in" currently has
no logging so it's very difficult to figure out why it decides not to
do something.

Add some logging to the process to try and better understand what
happens.

Fixes #441
2021-01-12 12:54:37 +01:00
Xavier Morel
4b5dfc2005 [FIX] forwardport: broken fp if intermediate commit requires renamelimit=0
On a cherrypick failing due to renamelimit issues, the cherrypick
would be retried after resetting the target to its *original* commit.

This only works correctly if the first commit works after a retry, if
a latter commit has to be retried then it gets re-cherry-picked onto
the target branch rather than its own parent.

Fix by remembering the previously successful cherry-picked commit and
resetting to *that*. However I can't really test it because it's
rather hard to get into a situation where the rename detection fails
using synthetic tests.

While at it, clean the logs by stripping the "performing inexact
rename detection" stuff from all stderr (both the CherrypickError from
which it was already stripped and the debug messages).

Fixes #444
2021-01-11 14:43:36 +01:00
Xavier Morel
7ab7eed27e [IMP] forwardport: lower level of waiting for batch to be ready
Waiting for the current batch to validate before forward-porting it is
normal, it's useful information but should not be warned about.
2020-07-30 13:20:41 +02:00
Xavier-Do
04ad4f6b67 [FIX] forwardport: nicer skipci message 2020-07-19 14:32:21 +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
db9e86f760 [IMP] forwardport: reliability of PR reminders
The exponential backoff offsets from the write_date of the children
PRs, however it doesn't reset, so the offsetting gets bumped up way
more than originally expected or designed if the child PRs are under
active development for some reason.

Fix this by adding a field to specifically record the date of merge of
a PR, and check that feature against the backoff offset. This should
provide more regular and reliable backoff.

Fixes #369
2020-05-26 15:56:36 +02:00
Xavier Morel
4d528465d9 [FIX] forwardport: incorrect / misleading conflict message
Given a PR batch getting forward-ported together, if one of the PRs
has a conflict the others should be considered "in conflict" as well,
and should have a note pointing in that direction and indicating that
the PR should be approved the normal way eventually. Which they do.

However, the message is confusing as it gets bolted on the normal
non-conflicting message, either noting that it's part of a chain
or (worse because it gives conflicting indication) the "terminal"
message recommending using the forwardbot to approve of the entire
chain.

I've no idea why I did it that way instead of just adding a case to
the conditional, and the commit message provides no indication. But
perform that change, it seems innocuous, hopefully there weren't good
reasons I forgot about for doing it the other way around.

Fixes #367
2020-05-20 07:43:53 +02:00
Xavier Morel
d99d1c2ad6 [ADD] forwardport: ability to skip CI when forward porting
Provides a `skipci` command to PR reviewers. This makes it so the
followup PRs (after the first one) get created immediately, without
waiting for CI to succeed on a given forward-port PR.

This can be useful if for some reason a change *must* be merged in
branch N+1 before it can be merged in branch N.

Fixes #363
2020-04-17 08:09:01 +02:00
xmo-odoo
49c1d960fa
[FIX] forwardport: race condition on PR creation (#357)
e9e08fec3c attempted to fix the issue
but obviously failed as it still occurs: when creating a PR through
the API, it's possible that the webhook gets triggered fast enough the
transaction creating the PR from the webhook commits before we get
around to creating our own PR from the API call. In which case the
forward port process aborts.

The process is re-run later on and generally succeeds fully, but we're
left with a dangling PR we created but couldn't do anything with as
its use broke.

This issue seems to be getting more frequent so it's becoming quite
important to fix it. Therefore we give our Raging Bull a Big Gun and
now he has 20 attack *cough cough* we lock the bloody table down
tight (only allow concurrent `SELECT`) until we've got the PR back and
we've done the updates we need to it and nobody can mess with it...
probably.

This is not ideal as it's going to block updates to completely
unrelated PRs but it doesn't seem like postgres really allows for
locking out creations without locking out the rest, short of using
advisory locks maybe? E.g. in the `create` override get a
`pg_advisory_xact_lock_shared`, then get a `pg_advisory_xact_lock` in
the forward-port process that way we're just blocking the concurrent
creation of PRs during forward port, but creations don't block one
another and we don't block updates.

Application-level locks wouldn't really work as the 'bot could be
deployed using multi-worker scenarios so we'd need cross-process locks
or something.

Hopefully fixes #352
2020-04-07 15:02:39 +02:00
Xavier Morel
1440aec251 [IMP] forwardport: exponential backoff on reminders
The reminder feature is a bit brutal when people go on holidays or
whatever as it keeps commenting every day.

This should comment every day for a few days, then quickly taper down.

Closes #285
2020-03-16 15:03:11 +01:00
Xavier Morel
ddf3f5013e [FIX] forwardport: fix reminder cron to avoid multiple messages
Because the reminder cron uses groupby to "merge" open PRs related to the
same source and send a single message for all of them (e.g. PR 6548
forward-ported to 6587 and 6591 should have a single reminder message per
day not one per descendant), the PRs with the same source need to be
consecutive in the search sequence.

However there was no order specified so the search would yield PRs in id
order or something, and if there happened to be an other forward-port PR
inbetween the descendants of the original would not get coalesced and would
therefore trigger a message per descendant per day (doubling or tripling the
intended spam rate).

Ordering by source_id should fix the issue as it ought make all PRs
forward-ported from the same thing contiguous, and therefore grouped
together before sending reminder messages.

An alternatively solution would be to use `groupby` instead of `search` but
it would require more modifications as we'd need to re-browse the sources
and descendants, etc...

First part of fixing #285 as this is likely why odoo/enterprise#7204 got
spammed so much: its descendants were odoo/enterprise#7367 and
odoo/enterprise#7369 and it just so happens that odoo/enterprise#7368 was
*also* a forward port PR, causing the issue explained above.
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
Christophe Monniez
3e40c38e89 [FIX] forwardport: fix forward-port Odoo wiki link 2020-03-10 08:00:07 +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
aa2248e379 [IMP] forwardport: close command
This is useful as the author of the original PR doesn't necessarily
have (write) access to the repository where the forward-port PR was
created. As a result, while they can r+ the PR they're unable to close
it (via github's interface).

Since the forwardport bot created the PR, it can also close it, which
seems like a useful feature.

Closes #341
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
Paul Morelle
1f9713cca0 [FIX] forwardport: update the wiki link
On github wiki, the link changed from
https://github.com/odoo/odoo/wiki/Mergebot
to
https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot

Reflect these changes in the comments posted by the bot.
2020-02-12 14:06:36 +01:00
Xavier Morel
fd24b791b3 [FIX] forwardport: prefix of working copy when forward porting
The missing dash makes it hard to differentiate the target's name and
the random crap TemporaryDirectory tacks on. Add a separator dash.
2020-02-07 08:29:55 +01:00
Xavier Morel
3f61dc9ce4 [IMP] forwardport: warning message when co-dep PR has a conflict
* Add some more information as to why the user *should* do on the PR
  the message is printed on, the previous message left that to their
  imagination
* The PR selection was *completely* wrong as it would select the old
  PRs which really isn't what we want. And turns out there's no good
  reason to create & send the feedback in the loop creating the
  forward-port prs, that can be moved to a followup loop where we have
  created hopefully created all the forward-port PRs.

  Also technically we could do even better than currently and remap
  the prs mapped to conflict data to the new PRs and know exactly
  which of the forward-ported PRs is faulty, but that seems overkill
  for now.
2020-01-30 13:56:01 +01:00
Xavier Morel
e065b7aba7 [FIX] forwardport: reintroduce final newline in a comment
Tests check for it, easier to fix one line than change all the
relevant tests.
2020-01-30 09:41:16 +01:00
Xavier Morel
9aac1b4a3e [ADD] forwardport: special handling of adding branches to projects
If a new branch is added to a project, there's an issue with *ongoing*
forward ports (forward ports which were not merged before the branch
was forked from an existing one): the new branch gets "skipped" and
might be missing some fixes until those are noticed and backported.

This commit hooks into updating projects to try and see if the update
consists of adding a branch inside the sequence, in which case it
tries to find the FP sequences to update and queues up new
"intermediate" forward ports to insert into the existing sequences.

Note: had to increase the cron socket limit to 2mn as 1mn blew up the
big staging cron in the test (after all the forward-port PRs are
approved).

Fixes #262

[FIX]
2020-01-29 15:59:43 +01:00
Xavier Morel
43e6f5e5ec [FIX] forwardport: logging.warn -> logging.warning
logging.warn is deprecated but I've a hard time shaking the habit.
2020-01-29 15:59:43 +01:00
Xavier Morel
a0fe545d86 [IMP] forwardport: warn when linked PR failed to FW
Before this change, if multiple co-dependent PRs get forward-ported
and one of them has a conflict the notice on the others is very
limited: they're tagged as `conflict` but there is no other
information provided in the PR description or in the subsequent
message.

Add a small warning to these other PRs, for clarity.

Closes #302
2020-01-29 15:55:06 +01:00
Xavier Morel
5a8f821de0 [FIX] forwardport: recover from failure to create forward port PR
Currently if the creation of a forward-port pull request fails:

* the branches are left un-cleaned
* preceding PRs are left open
* the PR whose creation failed may or may not have actually failed,
  and may or may not still be open

We need to delete the forward port branches anyway, and IIRC
that *should* automatically close the PR. Sadly making it so github
predictably / reliably blows up when trying to create a PR via the API
is difficult so this is essentially untestable.

Closes #296
2020-01-29 15:43:57 +01:00
Xavier Morel
5fae01e53e [IMP] forwardport: feedback on commands
The forwardbot's command parsing was missing feedback when trying to
use commands without the proper ACL. This would make some situations
of comments seemingly being lost hard to diagnose.

Closes #300
2020-01-29 15:09:09 +01:00