Commit Graph

144 Commits

Author SHA1 Message Date
Xavier Morel
13f239826e [FIX] forwardport: avoid logging git error if there's no stream data
If no stream data was captured (no stderr and no stdout), would just
log

    git call error:

as error, with no further information.

Don't do that if we have neither stderr nor stdout data, since we're
re-raising the exception anyway, it's just confusing.
2022-10-26 14:47:00 +02:00
Xavier Morel
22c3406659 [FIX] forwardport: error reporting when git command fails
- if stderr was empty or had been redirected to stdout, no useful
  information would be show, making debugging more complicated
- the fallback is the error itself, but since it's reraised odds are
  pretty high the caller will eventually log the error itself, so
  it's redundant

=> fallback to stdout if stderr is empty, and only log if either is
non-empty
2022-10-26 14:47:00 +02:00
Xavier Morel
65c2ffc997 [MERGE] from 13.0
Get mergebot updates from since the runbot was upgraded.

NOTE: updates forwardport.models.forwardport.Queue with slots for
compatibility with commit
odoo/odoo@ea3e39506a "use slots for
BaseModel", otherwise we get

    TypeError: __bases__ assignment: 'ForwardPortTasks' object layout differs from 'BaseModel'
2022-08-23 14:41:35 +02:00
Xavier Morel
e9278c021d [IMP] forwardport: commenting of test_author_can_close_via_fwbot 2022-08-05 15:35:51 +02:00
Xavier Morel
60266a1b23 [FIX] *: git invocation error and a few bits
- if stderr has been rerouted or explicitely rerouted to STDOUT,
  `e.stderr` is `None` and the error reporting blows up (which is
  inconvenient). Handle this case.
- handle the case where `fp_github_name` has not been configured (it's
  not a super useful handling but meh, apparently git/hub doesn't
  really care if there's a username when using API tokens)
- minor improvement to the refline parser: per-spec, the trailing
  newline is optional, so don't fail if it's missing
2022-08-05 15:35:51 +02:00
Xavier Morel
32bf0deda6 [IMP] *: store filestore & forwardport checkouts in temp dirs
I'm surprised this ever worked, I guess concurrent tests stopped
working long before that? Or I misunderstood some of the historical
failures as transient?

During the cleanup of the forwardport test, I'd empty out the
`user_cache_dir('forwardport') / owner`, except the owner is always
the same (more or less) so all the tests check out their repos (and
working copies) in the same directory. If one test is cleaning up
while an other is performing a forward port, the second will blow up.

Also move the filestore to a tempdir, especially during creation of
the template db: it gets leaked so over time that generates gigabytes
of data which doesn't get cleaned up. But the template db filestore is
only "necessary" during the creation of the template, once the
template's been created it's of no use and won't be copied to create
the test dbs (though it could be, I guess).
2022-08-05 15:35:51 +02:00
Xavier Morel
b86092de83 [IMP] *: freeze wizard v3, freezer and wizarder
Stop *staging* release PRs: they are normally fairly simple and should
not fail their staging outside of unreliable tests (or possibly a few
edge cases e.g. forgot one version change thing), however staging them
creates the possibility of a "version hole" on the release branch
which is undesirable.

Instead, immediately and unconditionally push the release commits onto
the newly created branches, if there are things which don't work they
can be fixed afterwards (and the process refined, maybe).

Also add the same feature for *bump* PRs, with the difference that the
bump PRs are not created / requested by default (they have to be opted
in individually).

For convenience, add a feature which automatically finds the PRs via
inputting the label (not really tested yet).

Closes #603
2022-08-05 15:35:51 +02:00
Xavier Morel
64eeb6142e [FIX] runbot_merge: also show banners on PR pages
It's likely that the PR pages are seen more commonly than the
dashboard by most users, so add alerts there in case users wonder
what's happening.

Fixes #580
2022-08-05 15:35:51 +02:00
Xavier Morel
2a268d777c [IMP] forwardport: check commit liveness on the PR we're porting from
Check remains a tad dodgy, but since we're actually porting from
`root` (the earliest ancestor of `self`) it makes more sense to
sanity-check that *its* commits remain visible.

Also log that as it makes a tad more sense, hopefully.

Closes #600
2022-06-30 15:07:49 +02:00
Xavier Morel
e8ae5ec263 [IMP] forwardport: flag detached PRs in their dashboard
This is an important bit of information but it was not visible without
going into the backend.

`user-select-none` doesn't work in BS3 but that way it'll be ready for
an eventual update. Currently when hovering the badge the cursor
switches to text selection, and the text is selectable, which is
useless.

Complements 4e235a2 and finishes the fixes for #617
2022-06-30 15:07:49 +02:00
Xavier Morel
4e70b1acbb [FIX] forwardport: detached / closed PR clarifications
- trying to r+ a detached PR *via the forwardbot* should warn, same as
  a non-forwardport PR
- the following sibling of a closed PR should be detached from
  it (probably)
- when a closed forward-port PR is reopened, there should be a
  notification that it is detached and merged via mergebot

Fixes #617
2022-06-30 15:07:49 +02:00
Xavier Morel
fb45f089b0 [FIX] forwardport: use diff3 for conflict style
Existing conflict style is the local default ("merge", most
likely). `diff3` is a lot more informative as it provides the common
ancestor's code for the hunk, which helps see how the two branches
diverged and thus resolve the conflict.

Even better would be zdiff3 but that's a bit too recent...

Fixes #619
2022-06-30 15:07:49 +02:00
Xavier Morel
f430c014c1 [IMP] *: review mergebot & forwardbot messages for pinging
Old messages were quite inconsistent in their pinging of the PR author
and reviewer.

Reviewed messages (probably missed some but...) and try to more
consistently ping when the feedback requires some sort of action in
order to proceed.

Fixes #592
2022-06-30 15:07:49 +02:00
Xavier Morel
2204c0410a [IMP] mergebot, forwardbot: various UI bits
- code in the various menus added over time through the UI (queues,
  configuration, ...)
- update / improve PR layout a tick
- fix "outstanding forward ports" count on the dashboard
- improve hover title / help on dashboard
  - add date of last modification (usually date of success / failure)
  - make casing more coherent (everything lowercase)
  - add explicit note that UTC date on staged at label is staged at datetime
  - rediscover yet again that the staging information is when hovering
    on the staging *except the staged at label*
- improve `PullRequest.unstage` to always insert the PR at the start of the
  reason when cancelling the staging, for clarity / traceability

Closes #560, closes #609
2022-06-30 15:07:49 +02:00
Xavier Morel
2337bd8518 [FIX] forwardport: chain crash on insert in forward-port cron
On two of the freezes, thereafter the logs showed serial crashes in
the forwardport cron when trying to find the insertion point for a new
forward-port.

The first time was not really diagnosed, the second time the cause was
found to be a retargeted PR which led to a failure of the "insertion"
forward port, which did not take that possibility in account (it
assumed -- sensibly I believed -- that an intermediate FP following a
branch insertion would always succeed, sadly the malevolent universe
had other plans).

So only insert the new forward port inside its sequence (if necessary)
if the forward port actually succeeded, otherwise ignore it.

Fixes #551
2022-02-10 13:51:33 +01:00
Xavier Morel
e05cc77a57 [FIX] forwardport: don't forwardport freeze PRs
The freeze wizard has support for merging freeze / release PRs on each
of the newly created branches. But since this would be done by, well,
merging, those PRs would get forward-ported to master, and would have
to be closed there.

This creates additional work for the freeze master, and noise /
parasitic PRs.

Obviously it's possible for the freeze master to set some nonsense `up
to` (nonsense because the "real" limit doesn't exist yet at that
point), but really it never makes any sense to forward port release
PRs, so the wizard should do it.
2022-02-08 10:11:57 +01:00
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
8c11db21a2 [FIX] forwardport: merge error message change in git
Apparently 2.34:

* flipped around the "auto-merging" and "CONFLICT" messages on stdout,
  so just match the second one with wildcards around to ignore the
  location of the first
* changed the casing and content of everything after the `error` line
  on stderr, so just ignore it all (none of it's actually useful
  anyway)
2021-11-18 08:20:50 +01:00
Xavier Morel
4da0f5df69 [ADD] runbot_merge: ~~tree~~ freeze wizard
Provides a less manual interface for creating the freeze:

* takes the name of the branch to create
* takes any number of PRs which must be part of the freeze
* takes PRs representing the HEADs of the new branches

Then essentially takes care of the test.

Implementation of the actual wizard is not trivial but fairly
straightforward and linear, biggest issue is not being able to
`project_id.branch_ids[1]` to get the new branch, not sure why but it
seems to ignore the ordering, clearing the cache doens't fix it.

When creating the branches, add a sleep after each one for secondary
rate limiting purposes. Same when deleting branches.

Also the forwardbot has been updated to disable the forwardport cron
while a freeze is ongoing, this simplifies the freezing process.

Note: after recommendation of @aab-odoo, tried using `_applyChanges`
in `_checkState` but it simply did not work: the two relational fields
got completely frozen and were impossible to update, which is less
than ideal. Oh well, hopefully it works well enough like this for now.
2021-11-17 10:40:12 +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
0e087e7433 [ADD] mergebot, forwardbot: changelog
* Adds a changelog page, linked from the main, with content
  automatically loaded from the source. To avoid conflicts, each entry
  is its own file and entries are grouped by the month during which
  the update will (probably) be deployed
* The last group (most likely "last update") doesn't have a title, the
  rest do.
* Add changelog entries from the last update so it's not too empty.
* Also update the layout for the alerts a bit: remove bottom margin to
  reduce loss of whitespace.
2021-10-20 15:16:48 +02: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
c6755a045a [FIX] forwardport: possible race in forwardport followup update
Normally when a forwardport is updated the forward-bot cascades the
update onto its followups (if it has any), but takes care to keep the
followups attached as they were not updated "by hand".

In the case of odoo/odoo#77677 however that did not work and the
followup PRs got detached. Looking at the logs, it becomes flagrant
that there was a race condition: either Git took a long time to
respond to the push, or there was an IO slowdown which led to the
"local update" taking a very long time. Either way this allowed the
"synchronize" webhook from github to arrive before the current
transaction was committed, rolling back said transaction and making
the forwardbot assume this was a "real" sync and detach the followup
from its parent.

Locking the PR row up-front ought fix the issue, and also move the
local update to before having pushed: the "extra" commits in the local
cache don't matter too much even if pushing to github fails, they'll
be cleaned up by a GC eventually.

Also migrate the `-f` on push to `--force-if-includes` in order to
avoid possible race conditions on the push (since we're not fetching
the current branch, use the full-syntax explicit CAS form, that's
exactly what we're looking for anyway).

Fixes #541 (hopefully)
2021-10-20 14:36:50 +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
a7808425e3 [IMP] runbot_merge: reject review without email
If a reviewer doesn't have an email set, the Signed-Off-By is an
`@users.noreply.github.com` address which just looks weird in the
final result.

Initially the thinking was that emails would be required for users to
*be* reviewers or self-reviewers, but since those are now o2ms / m2ms
it's a bit of a pain in the ass.

Instead, provide an action to easily try and fetch the public email of
a user from github.

Fixes #531
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
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
52b6776204 [FIX] forwardport: add override to repository view
Otherwise the forwardport target can not be set, which is a bit of an
issue.
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
6b1f698c23 [IMP] forwardport: handling of updates causing conflicts on followups
If a PR is updated and has extent forward-ports, those forwardports
get updated automatically ("followup").

However there is an issue if the udpate causes a conflict in the
followup: the conflict gets silently pushed, and may fairly easily get
merged if it occurs in an area which the CI doesn't cover.

It's unclear what the policy really should be for this issue, and
there is no real way to *block* a pull request at the moment (save by
putting it in error at the mergebot level I guess?), so for now
clearly notify the user on both the modified PR and the followup,
with a comment on both.

We may want to revisit this eventually.

Fixes #467
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-Do
b843fee8d9 [FIX] runbot: adapt routes and licenses 2021-07-27 16:11:49 +02:00
Xavier Morel
0b1e33da7c [IMP] forwardport: mitigate cat-file not finding commit on updates
Fix #457 hopefully: I didn't manage to repro / create a test for.

It looks like in some cases during the update process the PR ref lags
behind the branch itself. This means `forwardport.updates` creates a
new commit, pushes it, then on the next iteration updates the local
cache, tries to find the commit we just pushed... and that fails.

I can only assume this is because when there's enough load on the
github side the update to the `info/refs` pseudo-file can fall
behind (it's now 4MB and holding nearly 65k refs).

So cheat: take the commit we just pushed to the dev remote
and... immediately push it to the local cache under a dummy branch,
which we delete. Since we only gc "1 day ago" this should not vacuum.
2021-03-02 14:28:32 +01: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
2aeecb68b9 [FIX] forwardport: completely update PR data when forwarding updates
The process did properly update the state, but not the squash state.

It's somewhat unclear whether the state should be fully reset and
require reapproval though. Maybe only the validation should be reset?
The CI will eventually run and either succeed (re-validating) or
fail (devalidating, hopefully) but I'm not entirely sure this is
correct.
2021-03-02 14:28:32 +01:00
Xavier Morel
5e05d3cd96 [REF] forwardport: move PR update tests to their own module 2021-03-02 14:28:32 +01:00