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.
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
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
* 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.
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]
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
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
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
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).
This is not super useful and causes issues with runbot as it uses
commit dates to decide how "old" branches are, and ignores (doesn't
create when it scans them) branches more than a month old.
So the forwardport branches will be completely ignored (not considered
let alone tested) if we happen to forward-port a PR last updated more
than a month ago. Which is somewhat inconvenient.
Closes#274
Before this:
* the forwardport bot always sets itself as the committer when it
creates a forward-port branch
* when creating squashed / conflict commits, it becomes both author
and committer
This is not great as it loses a fair amount of authorship information
and doesn't properly give credit where credit is due. Improve this in
the following ways:
* use the original authorship information as-is when forward-porting
commits
* the the forward port fails, use the original authorship information
as-is if there's a single commit to forward port
* otherwise if there's only a single author / committer across the
branch being forwardported use that, if there are multiple give up
and use the identity of the 'bot, since the branch will probably
need to be re-done in full the authorship information of the
placeholder commit should not matter overly much
Uses git's magic ENV variables as that's pretty much the only way to
properly override the COMMITTER date: conf items only provide for
author and committer *identity* (name and email), and while `commit`
allows overriding the *authorship* date (`--date`) it doesn't provide
any option for the *commit* date.
Fixes#255
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
If the ref we asked for does not exist, github apparently decides to
fall-back to prefix-matching. So if we're trying to delete
already-deleted branch A and someone called their branch A-x we're
going to get it as a result.
Thankfully they were apparently smart enough to return a list even if
there's only a single fuzzy match. So if we get a list (instead of a
dict) as response to git/refs/heads assume the branch was already
deleted as if we got a 404.
If a PR is *merged*, enqueue it for deletion (with a 2 weeks delay).
Mainly to avoid FW branches staying around long after they've been
merged (possibly eventually closed?), will also clean up regular
merged branches, including historical merges forgotten by their
author.
Fixes#230
In the case where we have a co-dependent forward port (co-dependent
PRs got forward-ported, which they should be together) where *one* of
the PRs got explicitly updated, the batch would "fall into a hole"
being handled as neither "this is part of a forward-port sequence" nor
"this is a new merge to forward-port" (the latter being the proper
one).
Modify & remove guards which checked that either no or all PRs in a
batch have parents: should be either all or not all.
Fixes#231
Turns out tagging PRs requires a pretty significant level of ACLs
which we may not want to give to the forwardbot?
Anyway use the mergebot ACLs (which already include tagging) for this.
* 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
Due to the title formatting of FP PRs, we'd get incorrectly formatted
commit messages if the PR was *merged* (rather than squashed /
fast-forwarded) due to either "merge" or "rebase-merge" integration
mode: in that case the PR message would be used as message for the
merge commit and that'd be along the lines of "Forward Port of #xxx to
<somebranch> (failed)", followed by the old PR message (e.g. see this
commit message).
* re-extract and reuse original PR title, just prefix with "[FW]"
* finally add support for tagging, and use that to tag the PRs,
especially for the failed / conflict marker which is quite important
Closes#229
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.
Closesodoo/runbot#228
* 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
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).
The queue would get items to process one at a time, process, commit,
and go to the next. However this is an issue if one of the item fails
systematically for some reason (aka it's not just a transient
failure): the cron fails, then restarts at the exact same point, and
fails again with the same issue, leading to following items never
getting processed.
Fix by getting all the queue contents at once, processing them one by
one and "skipping" any item which fails (leaving it in place so it can
get re-processed later).
That way, even if an item causes issues, the rest of the queue gets
processed normally. The interruption was an issue following
odoo/enterprise#5670 not getting properly updated in the
backend (backend didn't get notified of the last two updates /
force-push to the PR, so it was trying to forward-port a commit which
didn't exist - and failing).
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
When running tests on some machines, it's apparently possible for the
PR-creation webhook to come back before the PR creation request has,
leading to the creation of the PR from the API call duplicating that of
the webhook and blowing up.
To fix, immediately commit the transaction then check if we already have
the PR we just created in the system, and only create it explicitly if
not.
User should probably be warned when they try to set the limit ("up
to") in a context where it's going to be ignored:
* on a forward port PR (should be set on the source)
* on a merged source (should be set before the PR is merged)
Closes#213
In selecting the parent commits to list on the last PR, we would miss
the *first* forward-port of the sequence. Not sure why we added a
detrimental check on source_id there.
Also add a missing space between "chain" and "containing" in the case
where there's at least one forward-port PR other than the final one.
Fixes#212
Test is probably more complex than necessary (thinking about it, the
failed staging is probably unnecessary) but that triggers the issue
and matches the original scenario.
The problem was really with new CI events being received on the last
forward-port PR of a sequence: previous PRs would have a child PR so
the check would abort, but for the last PR it would go through, fail
to find an active batch, then blow up as it tries to create a
forwardport.batch without an actual batch.
Change this to use the existence of an inactive batch not linked to a
staging as a flag that the PR has been processed and forward-ported.
Closes#218
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).
If a PR is explicitly updated, it gets converted to a normal
PR[0]. Before this, users had no indication that this had happened and
might be wondering what they're supposed to do (or try to r+ via the
forwardbot, which doesn't work on a root PR).
[0] to an extent: the PR still has a source and might have children,
in which case the followups will be created from the source &
existing followups should be updated to match
Closes#206
When creating the conflicting commit of a single commit PR, reuse the
original commit's meta-information so the developer / fixer can more
easily update it in-place.
Closes#204
In the case where an FP sequence is interrupted (e.g. there was a
conflict during one of the intermediate steps), followups get linked
to the original source but don't get linked to the "interruption" PR
which is a bit confusing.
Link FP PRs to both source and root if they're different.
* always allow specifying the PR's own branch as a forward-port limit
/ target (even if deactivated or disabled)
* add an "ignore" alias to "up to <pr target>" for clarity
* add dedicated feedback when deactivating forward-port of a PR
Fixes#191
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
* don't warn on every PR on the dot every week, instead wait for the
PRs to be "sufficiently old" (at least 3 days)
* after discussion with bugfix, the reminder ping should be sent every
day following the PR being "old enough"
* run the cron every day instead of every week
* add an override context key for test purposes
Closes#198
Working copies were created in tempdir under the assumption that
they're, well, temporary.
However after thinking about it more there are two issues with this:
* tempdirs might not be in the same FS as the cache dir, meaning
meaning `git clone` can't hardlink the repo objects and has to
copy them
* tempdirs are often on RAM-backed tmpfs, which is not great when we're
filling them with multiple GB worth of git repository...
If the default limit of a forward-port sequence is not a valid
target (either disabled or not actually forward-ported to), the last
effective forward port in a sequence will be commented on as any
intermediate PR rather than get a proper ping and r+ instructions.
Also remove a bunch of leftover prints in the tests.
Fixes#192
* a high renamelimit increases the cherrypick runtime and can
apparently fail for reasons other than actual conflicts (cf
odoo/odoo#36893) so first attempt to cherrypick with the default
renamelimit, and fallback to renamelimit=0 if that fails
* in that case, filter out the spam of "progress" messages about
inexact rename detection as it's not really useful. There seems to
be no built-in way to suppress these.
Closes#196
Turns out bare repositories (unlike normal ones) don't have any
refspecs by default. So adding the PR refspec would... replace the
default behaviour (apparently?) and the base branches would never get
fetched at all.
--mirror looks to be an other option as it has a default refspec but
it's a bit of an odd duck: it has pull requests in refs/pull but not
all of them? And open / closed doesn't seem to be a factor.
Attempts to use rev-list seem to work locally but they fail *a lot*
when live. The previous attempt to fix it in
0f4c22490c made things worse rather than
better once deployed.
Give up on that (at least for now?), and just ask github what the PR's
commits are then cherrypick exactly that.
The ping message was not clear
- don't know if anything is needed from the author
- don't know if the last step in the chain
Ping the authors only when something needs to be done (error or last
step).
Do not ping non-reviewers as they will not have the rights to r+ or
modify the PR anyway
Closes#192
There's an issue of too many commits being selected for
cherry-picking. It still isn't quite clear, but the forward ports to
11.0 systematically seem to get everything from
5045b5f4c346e60c9b127403ef1fde453d49396a to the PR head, and that
commit was one of the first to land after the last merge-based forward
port.
So the commits selection seems to behave as if the commits range was
`..origin/pull/36692` rather than the `origin/10.0..origin/pull/36692`
which is passed to rev-list. This might be an issue of concurrent
access / update of the refs (though I can't reproduce it locally,
missing refs generate an error).
This change attempts to "pin" the local branch in the working copy
rather than go and get it from the repo. It's unclear that this will
change / fix anything, but it might just work.
Also stop creating shared clones, that seems like an unnecessary
risk (and might be the actual source of the issue).
The original method was to `git diff | git apply` in order to get a
complete overview of conflicts generated by the forward port (if
any).
However this turns out to have a huge issue in the presence of renamed
or removed files: in that case `git apply` will simply not do
anything, and fail with a completely clean working copy. Which is very
much undesirable.
-> alternative method, squash the PR to a single commit then
cherry-pick that single commit, this should provide us with proper
conflicts & their markers.
Also add tests for conflicts due to deleted files...
* 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.
