I DECLARE BANKRUPTCY!!!
The previous implementation of labels lookup was really not
intuitive (it was just a char field, and matched labels by equality
including the owner tag), and was also full of broken edge
cases (e.g. traceback if a label matched multiple PRs in the same repo
because people reuse branch names).
Tried messing about with contextual `display_name` and `name_search`
on PRs but the client goes wonky in that case, and there is no clean
autocomplete for non-relational fields.
So created a view which reifies labels, and that can be used as the
basis for our search. It doesn't have to be maintained by hand, can be
searched somewhat flexibly, we can add new view fields in the future
if desirable, and it seems to work fine providing a nice
understandable UX, with the reliability of using a normal Odoo model
the normal way.
Also fixed the handling of bump PRs, clearly clearing the entire field
before trying to update existing records (even with a link_to
inbetween) is not the web client's fancy, re-selecting the current
label would just empty the thing entirely.
So use a two-step process slightly closer to the release PRs instead:
- first update or delete the existing bump PRs
- then add the new ones
The second part is because bump PRs are somewhat less critical than
release, so it can be a bit more DWIM compared to the more deliberate
process of release PRs where first the list of repositories involved
has to be set up just so, then the PRs can be filled in each of them.
Fixes#697
Previously the mergebot would only sync the head commit, but synching
more is useful.
Also update the final sanity check on staging:
- as with check, update the message & target branch
- reset PR state and post a message when updating message instead of
doing so silently
Note: maybe only fail the staging if the message is updated *and*
relevant to staging (aka there's a merge method and it's not
`rebase`)?
Fixes#680
Currently, if a PR forward-port PR gets detached the reason for it is
not always obvious, and may have to be hunted in the logs or in
"sibling" PRs.
By writing a forward port reason (hopefully) ever time we detach a PR,
and displaying that reason in the form and dashboard, the
justification should be a lot more obvious.
Fixes#679
When inserting a new branch, if there are extant forward ports
sequences spanning over the insertion, new forward ports must be
created for the new branch.
This was the case, *however* one of the cases was handled incorrectly:
PR batches (PRs to different repositories staged together) must be
forward-ported as batches. However when creating the "insert" batches,
these PRs would be split apart, leading to either inconsistent states
or an inability to merge the new forward ports (if they are
co-dependent which is a common case). This is because the handler
would look for all the relevant PRs to forward ports, but it would
fail to group them by label thereafter.
Fix that, create the `insert` batches correctly, and augment the
relevant test with an additional repository to test this case.
No gh issue was created, but this problem was reported on
odoo/odoo#110029 and odoo/enterprise#35838 (they were re-linked by
hand in the runbot and mergebot, but on github the labels remain
visibly different, one having the slug ZCwE while the other
hasO7Pr).
It's possible that the problem had occurred previously and not been
noticed (or reported), though it's also possible this issue had never
occurred before: for the latest freeze (16.1) there were 18 forward
ports spanning the insertion, but of them only 2 were the same batch.
It's almost certainly not useful, save as a minor convenience for
tests: decorrelating the branch sequence and the fp sequence seems
like it would only be extremely confusing, and on the mergebot all the
fp_sequence values are set to the default while the sequence values
are set to something useful and sensible (kinda).
Fixes#584
In case where the last branch (before the branch being frozen) is
disabled, the forwardport inserter screws up, and fails to correctly
create the intermediate forwardports from the new branch.
Also when disabling a branch, if there are FW PRs which target that
branch and have not been forward-ported further, automatically
forward-port them as if the branch had been disabled when they were
created, this should limit data loss and confusion.
Also change the message set on PRs when disabling a branch: because of
user conflicts in test setup, the message about a branch being
disabled would close the PRs, which would then orphan the followup,
leading to unexpected / inconsistent behaviour.
Fixes#665
- avoid pinging the author of the fw PR (which is the forward-bot
itself)
- instead ping the author and reviewer of the source, and possibly the
reviewer of the PR if any
- might also be a good idea to ping reviewers of intermediate PRs?
Github can fail to create the magic refs on PRs
(`pull/refs/?/head`). Since forwardport relies on these refs to fetch
PR content this is an issue when it occurs, as the forward ports fail
in a loop.
After discussion with Github support, it turns out Github enabled
`allowReachableSHA1InWant` a while back, meaning it's possible to
fetch content by commit (rather than ref) as long as the content is
"in network".
Use this property as fallback when checking if we can see the PR head
before forward porting.
Also:
- remove explicit configuration of GC during fetch, it doesn't disable
the autogc (yet?) but that's likely going to happen anyway
- update logging and logger hierarchy during forward port to make
things clearer and easier to extract, although based on PR id rather
than number
- rate limit failing forward ports to avoid running them on every cron
(~ every minute), run them every ~30mn instead, this provides higher
odds of recovery with less log garbage in case of transient github
failure, and if the PR is stuck it limits the log pollution
Fixes#658
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).
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
- 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
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
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
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
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.
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)
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
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
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
* 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
"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
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
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
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
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
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
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)
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.
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
2.29 changed the formatting of conflict labels (the stuff added at the
end of the closing conflict marker):
6cceea19eb
Used to be
>>>>>>> <commit>... <text>
now is
>>>>>>> <commit> (<text>)
Just ignore everything after the commit hash.
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
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
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
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
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
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.
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.
fixesodoo/runbot#310
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
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]
Add handling of branch filtering to the forwardport module:
* don't forward port (and trigger an error) when trying to port
PRs to different next targets
* otherwise port normally
e.g. given a project with repos A and B and branches a, b and c, with
branch b being excluded from repo B:
* a PR merged into A.a will be forward-ported to A.b and A.c
* a PR merged into B.a will be forward-ported to B.c (skipping the
excluded B.b)
* a PR set merged into (A.a, B.a) will *not* be forward-ported, and a
message will be posted to each PR denoting the incompatibility
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).
