NextERP/runbot
5
0
Fork 0
mirror of https://github.com/odoo/runbot.git synced 2025-03-17 08:25:46 +07:00
Code Issues Actions Packages Projects Releases Wiki Activity
18.0-fixes
runbot/forwardport/tests/test_simple.py

1200 lines
44 KiB
Python
Raw Permalink Normal View History

[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# -*- coding: utf-8 -*-
import collections
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
import re
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
import time
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
from datetime import datetime, timedelta
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
import pytest
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of, to_pr, matches
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[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-20 17:42:45 +07:00
FMT = '%Y-%m-%d %H:%M:%S'
FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT)
[IMP] forwardport: less spammy reminder cron * 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
2019-09-16 20:28:03 +07:00
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# need:
# * an odoo server
# - connected to a database
# - with relevant modules loaded / installed
# - set up project
# - add repo, branch(es)
# - provide visibility to contents si it can be queried & al
# * a tunnel so the server is visible from the outside (webhooks)
# * the ability to create repos on github
# - repo name
# - a github user to create a repo with
# - a github owner to create a repo *for*
# - provide ability to create commits, branches, prs, ...
def test_straightforward_flow(env, config, make_repo, users):
# TODO: ~all relevant data in users when creating partners
# get reviewer's name
reviewer_name = env['res.partner'].search([
('github_login', '=', users['reviewer'])
]).name
prod, other = make_basic(env, config, make_repo)
other_user = config['role_other']
other_user_repo = prod.fork(token=other_user['token'])
b_head = prod.commit('b')
c_head = prod.commit('c')
with prod, other_user_repo:
# create PR as a user with no access to prod (or other)
[_, p_1] = other_user_repo.make_commits(
'a',
Commit('p_0', tree={'x': '0'}),
Commit('p_1', tree={'x': '1'}),
ref='heads/hugechange'
)
pr = prod.make_pr(
target='a', title="super important change",
head=other_user['user'] + ':hugechange',
token=other_user['token']
)
prod.post_status(p_1, 'success', 'legal/cla')
prod.post_status(p_1, 'success', 'ci/runbot')
# use rebase-ff (instead of rebase-merge) so we don't have to dig in
# parents of the merge commit to find the cherrypicks
pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
[IMP] *: modernise tests via `to_pr` The `to_pr` helper was added a *while* ago to replace the pretty verbose process of looking a PR with the right number in the right repository after a `make_pr`. However while I did some ad-hoc replacement of existing `search` calls as I had to work with existing tests I never did a full search and replace to try and excise searches from the test suite. This is now done. I've undoubtedly missed a few, but a hundred-odd lines of codes have been simplified.
2024-12-02 20:17:28 +07:00
pr_id = to_pr(env, pr)
[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-20 17:42:45 +07:00
assert not pr_id.merge_date,\
"PR obviously shouldn't have a merge date before being merged"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
# should merge the staging then create the FP PR
env.run_crons()
[FIX] forwardport: fix pinging on forwardport PRs - 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?
2022-11-03 17:46:31 +07:00
assert datetime.utcnow() - datetime.strptime(pr_id.merge_date, FMT) <= timedelta(minutes=1),\
[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-20 17:42:45 +07:00
"check if merge date was set about now (within a minute as crons and " \
"RPC calls yield various delays before we're back)"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
p_1_merged = prod.commit('a')
assert p_1_merged.id != p_1
assert p_1_merged.message == MESSAGE_TEMPLATE.format(
message='p_1',
repo=prod.name,
number=pr.number,
headers='',
name=reviewer_name,
[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-06 18:06:53 +07:00
email=config['role_reviewer']['email'],
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
)
assert prod.read_tree(p_1_merged) == {
'f': 'e',
'x': '1',
}, "ensure p_1_merged has ~ the same contents as p_1 but is a different commit"
[p_0_merged] = p_1_merged.parents
# wait a bit for PR webhook... ?
time.sleep(5)
env.run_crons()
pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
assert pr0.number == pr.number
# 50 lines in, we can start checking the forward port...
assert pr1.parent_id == pr0
assert pr1.source_id == pr0
other_owner = other.name.split('/')[0]
assert re.match(other_owner + ':' + REF_PATTERN.format(target='b', source='hugechange'), pr1.label), \
"check that FP PR was created in FP target repo"
c = prod.commit(pr1.head)
# TODO: add original committer (if !author) as co-author in commit message?
assert c.author['name'] == other_user['user'], "author should still be original's probably"
[IMP] forwardport: better handle authorship information 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
2019-11-20 13:43:56 +07:00
assert c.committer['name'] == other_user['user'], "committer should also still be the original's, really"
[FIX] forwardport: fix pinging on forwardport PRs - 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?
2022-11-03 17:46:31 +07:00
[REF] *: move most feedback messages to pseudo-templates Current system makes it hard to iterate feedback messages and make them clearer, this should improve things a touch. Use a bespoke model to avoid concerns with qweb rendering complexity (we just want GFM output and should not need logic). Also update fwbot test setup to always configure an fwbot name, in order to avoid ping messages closing the PRs they're talking about, that took a while to debug, and given the old message I assume I'd already hit it and just been too lazy to fix. This requires updating a bunch of tests as fwbot ping are sent *to* `fp_github_name`, but sent *from* the reference user (because that's the key we set). Note: noupdate on CSV files doesn't seem to work anymore, which isn't great. But instead set tracking on the template's templates, it's not quite as good but should be sufficient. Fixes #769
2023-06-12 19:41:42 +07:00
assert pr1.ping == "@%s @%s " % (
[FIX] forwardport: fix pinging on forwardport PRs - 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?
2022-11-03 17:46:31 +07:00
config['role_other']['user'],
config['role_reviewer']['user'],
), "ping of forward-port PR should include author and reviewer of source"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert prod.read_tree(c) == {
'f': 'c',
'g': 'b',
'x': '1'
}
with prod:
prod.post_status(pr1.head, 'success', 'ci/runbot')
prod.post_status(pr1.head, 'success', 'legal/cla')
env.run_crons()
[IMP] *: trigger-ify task queue type crons These are pretty simple to convert as they are straightforward: an item is added to a work queue (table), then a cron regularly scans through the table executing the items and deleting them. That means the cron trigger can just be added on `create` and things should work out fine. There's just two wrinkles in the port_forward cron: - It can be requeued in the future, so needs a conditional trigger-ing in `write`. - It is disabled during freeze (maybe something to change), as a result triggers don't enqueue at all, so we need to immediately trigger after freeze to force the cron re-enabling it.
2024-07-30 18:45:51 +07:00
env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
[IMP] forwardport: less spammy reminder cron * 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
2019-09-16 20:28:03 +07:00
[IMP] forwardbot: show FP PRs in reminder message When posting a reminder that there are open / waiting forward ports on a source PR, also post *which* PRs those are. While at it, move the cron code in a proper python file (so we can use stuff from odoo.tools), and fix display_name so we can straight use display_name as a github ref' ({owner}/{repo}#{number}). This impacts log-grepping but it seems like an improvement nonetheless. Closes odoo/runbot#228
2019-10-10 17:07:57 +07:00
pr0_, pr1_, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
[IMP] forwardport: less spammy reminder cron * 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
2019-09-16 20:28:03 +07:00
assert pr.comments == [
(users['reviewer'], 'hansen r+ rebase-ff'),
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
seen(env, pr, users),
[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-23 19:25:07 +07:00
(users['user'], 'Merge method set to rebase and fast-forward.'),
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
]
pr1_remote = prod.get_pr(pr1.number)
assert pr1_remote.comments == [
seen(env, pr1_remote, users),
(users['user'], """\
This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
[IMP] forwardport: don't ping *every* forwardport 98aaa910 updated the forwardport notifications system to notify on the forward ports rather than the source, to try and mitigate or at least shift some of the spam: spam the followers of the original source (which might be many people) somewhat less, at the possible cost of spamming the author and reviewer more because they get a message per forgotten forward port. This change aims to alleviate part of the latter, by only notifying on PRs which actually need to be r+'d, and not notifying on those which will implicitly "inherit" the reviews. This should cut down on redundant notifications and let users focus on the important ones.
2024-06-10 23:47:49 +07:00
""")]
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert pr0_ == pr0
assert pr1_ == pr1
[FIX] forwardbot: selection of ancestor in FP tail ping 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
2019-09-27 19:14:07 +07:00
assert pr1.parent_id == pr1.source_id == pr0
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert pr2.parent_id == pr1
assert pr2.source_id == pr0
assert not pr0.squash, "original PR has >1 commit"
assert not (pr1.squash or pr2.squash), "forward ports should also have >1 commit"
assert re.match(REF_PATTERN.format(target='c', source='hugechange'), pr2.refname), \
"check that FP PR was created in FP target repo"
assert prod.read_tree(prod.commit(pr2.head)) == {
'f': 'c',
'g': 'a',
'h': 'a',
'x': '1'
}
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
pr2_remote = prod.get_pr(pr2.number)
assert pr2_remote.comments == [
seen(env, pr2_remote, users),
[FIX] forwardport: clarify forward port message 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
2019-09-10 21:29:02 +07:00
(users['user'], """\
[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-23 19:25:07 +07:00
@%s @%s this PR targets c and is the last of the forward-port chain containing:
[IMP] forwardbot: show FP PRs in reminder message When posting a reminder that there are open / waiting forward ports on a source PR, also post *which* PRs those are. While at it, move the cron code in a proper python file (so we can use stuff from odoo.tools), and fix display_name so we can straight use display_name as a github ref' ({owner}/{repo}#{number}). This impacts log-grepping but it seems like an improvement nonetheless. Closes odoo/runbot#228
2019-10-10 17:07:57 +07:00
* %s
[FIX] forwardport: clarify forward port message 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
2019-09-10 21:29:02 +07:00
[REF] *: move most feedback messages to pseudo-templates Current system makes it hard to iterate feedback messages and make them clearer, this should improve things a touch. Use a bespoke model to avoid concerns with qweb rendering complexity (we just want GFM output and should not need logic). Also update fwbot test setup to always configure an fwbot name, in order to avoid ping messages closing the PRs they're talking about, that took a while to debug, and given the old message I assume I'd already hit it and just been too lazy to fix. This requires updating a bunch of tests as fwbot ping are sent *to* `fp_github_name`, but sent *from* the reference user (because that's the key we set). Note: noupdate on CSV files doesn't seem to work anymore, which isn't great. But instead set tracking on the template's templates, it's not quite as good but should be sufficient. Fixes #769
2023-06-12 19:41:42 +07:00
To merge the full chain, use
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
> @hansen r+
[FIX] forwardport: clarify forward port message 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
2019-09-10 21:29:02 +07:00
[FIX] forwardport: fix forward-port Odoo wiki link
2020-03-09 20:20:59 +07:00
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
[FIX] forwardbot: selection of ancestor in FP tail ping 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
2019-09-27 19:14:07 +07:00
""" % (
users['other'], users['reviewer'],
[IMP] forwardbot: show FP PRs in reminder message When posting a reminder that there are open / waiting forward ports on a source PR, also post *which* PRs those are. While at it, move the cron code in a proper python file (so we can use stuff from odoo.tools), and fix display_name so we can straight use display_name as a github ref' ({owner}/{repo}#{number}). This impacts log-grepping but it seems like an improvement nonetheless. Closes odoo/runbot#228
2019-10-10 17:07:57 +07:00
pr1.display_name,
[FIX] forwardbot: selection of ancestor in FP tail ping 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
2019-09-27 19:14:07 +07:00
)),
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
(users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % (
users['other'],
users['reviewer'],
pr0.display_name,
))
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
]
with prod:
prod.post_status(pr2.head, 'success', 'ci/runbot')
prod.post_status(pr2.head, 'success', 'legal/cla')
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
pr2_remote.post_comment('hansen r+', config['role_reviewer']['token'])
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
assert pr1.staging_id
assert pr2.staging_id
# two branches so should have two stagings
assert pr1.staging_id != pr2.staging_id
# validate
with prod:
prod.post_status('staging.b', 'success', 'ci/runbot')
prod.post_status('staging.b', 'success', 'legal/cla')
prod.post_status('staging.c', 'success', 'ci/runbot')
prod.post_status('staging.c', 'success', 'legal/cla')
# and trigger merge
env.run_crons()
# apparently github strips out trailing newlines when fetching through the
# API...
message_template = MESSAGE_TEMPLATE.format(
message='p_1',
repo=prod.name,
number='%s',
headers='X-original-commit: {}\n'.format(p_1_merged.id),
name=reviewer_name,
[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-06 18:06:53 +07:00
email=config['role_reviewer']['email'],
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
)
old_b = prod.read_tree(b_head)
head_b = prod.commit('b')
assert head_b.message == message_template % pr1.number
[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-09 12:55:38 +07:00
assert prod.commit(head_b.parents[0]).message == part_of(f'p_0\n\nX-original-commit: {p_0_merged}', pr1, separator='\n')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
b_tree = prod.read_tree(head_b)
assert b_tree == {
**old_b,
'x': '1',
}
old_c = prod.read_tree(c_head)
head_c = prod.commit('c')
assert head_c.message == message_template % pr2.number
[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-09 12:55:38 +07:00
assert prod.commit(head_c.parents[0]).message == part_of(f'p_0\n\nX-original-commit: {p_0_merged}', pr2, separator='\n')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
c_tree = prod.read_tree(head_c)
assert c_tree == {
**old_c,
'x': '1',
}
# check that we didn't just smash the original trees
assert prod.read_tree(prod.commit('a')) != b_tree != c_tree
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
prs = env['forwardport.branch_remover'].search([]).mapped('pr_id')
assert prs == pr0 | pr1 | pr2, "pr1 and pr2 should be slated for branch deletion"
env.run_crons('forwardport.remover', context={'forwardport_merged_before': FAKE_PREV_WEEK})
# should not have deleted the base branch (wrong repo)
assert other_user_repo.get_ref(pr.ref) == p_1
# should have deleted all PR branches
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
pr1_ref = pr1_remote.ref
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
with pytest.raises(AssertionError, match='Not Found'):
other.get_ref(pr1_ref)
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
pr2_ref = pr2_remote.ref
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
with pytest.raises(AssertionError, match="Not Found"):
other.get_ref(pr2_ref)
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
def test_empty(env, config, make_repo, users):
""" Cherrypick of an already cherrypicked (or separately implemented)
[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-11 16:36:35 +07:00
commit -> conflicting pr.
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
"""
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
prod, other = make_basic(env, config, make_repo, statuses="default")
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# merge change to b
with prod:
[p_0] = prod.make_commits(
'b', Commit('p', tree={'x': '0'}),
ref='heads/early'
)
pr0 = prod.make_pr(target='b', head='early')
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
prod.post_status(p_0, 'success')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
pr0.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
prod.post_status('staging.b', 'success')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# merge same change to a afterwards
with prod:
[p_1] = prod.make_commits(
'a', Commit('p_0', tree={'x': '0'}),
ref='heads/late'
)
pr1 = prod.make_pr(target='a', head='late')
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
prod.post_status(p_1, 'success')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
prod.post_status('staging.a', 'success')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert prod.read_tree(prod.commit('a')) == {
'f': 'e',
'x': '0',
}
assert prod.read_tree(prod.commit('b')) == {
'f': 'c',
'g': 'b',
'x': '0',
}
# should have 4 PRs:
# PR 0
# FP of PR 0 to C
# PR 1
# failed FP of PR1 to B
prs = env['runbot_merge.pull_requests'].search([], order='number')
assert len(prs) == 4
pr0_id = prs.filtered(lambda p: p.number == pr0.number)
pr1_id = prs.filtered(lambda p: p.number == pr1.number)
fp_id = prs.filtered(lambda p: p.parent_id == pr0_id)
fail_id = prs - (pr0_id | pr1_id | fp_id)
assert fp_id
assert fail_id
# unlinked from parent since cherrypick failed
assert not fail_id.parent_id
# the tree should be clean...
assert prod.read_tree(prod.commit(fail_id.head)) == {
'f': 'c',
'g': 'b',
'x': '0',
}
with prod:
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
prod.post_status(fp_id.head, 'success')
prod.post_status(fail_id.head, 'success')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
# should not have created any new PR
assert env['runbot_merge.pull_requests'].search([], order='number') == prs
[IMP] runbot_merge, forwardport: variable-user feedback Having all the feedback be sent by the mergebot user (github_token) is confusing. Add a way to specify which field of project should be used to source the token used when sending feedback. Fixes #190
2019-09-18 20:37:14 +07:00
# change FP token to see if the feedback comes from the proper user
project = env['runbot_merge.project'].search([])
[REF] *: move most feedback messages to pseudo-templates Current system makes it hard to iterate feedback messages and make them clearer, this should improve things a touch. Use a bespoke model to avoid concerns with qweb rendering complexity (we just want GFM output and should not need logic). Also update fwbot test setup to always configure an fwbot name, in order to avoid ping messages closing the PRs they're talking about, that took a while to debug, and given the old message I assume I'd already hit it and just been too lazy to fix. This requires updating a bunch of tests as fwbot ping are sent *to* `fp_github_name`, but sent *from* the reference user (because that's the key we set). Note: noupdate on CSV files doesn't seem to work anymore, which isn't great. But instead set tracking on the template's templates, it's not quite as good but should be sufficient. Fixes #769
2023-06-12 19:41:42 +07:00
project.write({
'fp_github_name': False,
'fp_github_token': config['role_other']['token'],
})
[IMP] runbot_merge, forwardport: variable-user feedback Having all the feedback be sent by the mergebot user (github_token) is confusing. Add a way to specify which field of project should be used to source the token used when sending feedback. Fixes #190
2019-09-18 20:37:14 +07:00
assert project.fp_github_name == users['other']
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# check reminder
[IMP] *: trigger-ify task queue type crons These are pretty simple to convert as they are straightforward: an item is added to a work queue (table), then a cron regularly scans through the table executing the items and deleting them. That means the cron trigger can just be added on `create` and things should work out fine. There's just two wrinkles in the port_forward cron: - It can be requeued in the future, so needs a conditional trigger-ing in `write`. - It is disabled during freeze (maybe something to change), as a result triggers don't enqueue at all, so we need to immediately trigger after freeze to force the cron re-enabling it.
2024-07-30 18:45:51 +07:00
env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
[IMP] forwardport: less spammy reminder cron * 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
2019-09-16 20:28:03 +07:00
[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-23 19:25:07 +07:00
awaiting = (
users['other'],
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
'@%s @%s this forward port of %s is awaiting action (not merged or closed).' % (
[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-23 19:25:07 +07:00
users['user'], users['reviewer'],
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
pr1_id.display_name
[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-23 19:25:07 +07:00
)
)
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
conflict = (users['user'], matches(
f"""@{users['user']} @{users['reviewer']} cherrypicking of pull request {pr1_id.display_name} failed.
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
stdout:
```
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
$$
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
```
stderr:
```
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
$$
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
```
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
In the former case, you may want to edit this PR message as well.
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}.
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
[REF] forwardport: update re_matches to not use a regex Using a regex as the pattern is quite frustrating due to all the escaping necessary, which in this refactoring I found out I'd missed, multiple times. Convert the pattern to something bespoke but not too complicated, we may want to add anchoring support and a bit more finesse and the future but for now straightforward "holes" seem to work well. I've added support for capturing and even named groups even if this as yet unnecessary and unused. Fixes #861 [^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
2024-06-04 17:15:29 +07:00
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
"""))
[IMP] forwardport: less spammy reminder cron * 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
2019-09-16 20:28:03 +07:00
assert pr1.comments == [
(users['reviewer'], 'hansen r+'),
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
seen(env, pr1, users),
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
]
fail_pr = prod.get_pr(fail_id.number)
assert fail_pr.comments == [
seen(env, fail_pr, users),
conflict,
[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-23 19:25:07 +07:00
awaiting,
awaiting,
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
], "each cron run should trigger a new message"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# check that this stops if we close the PR
with prod:
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
fail_pr.close()
[IMP] *: trigger-ify task queue type crons These are pretty simple to convert as they are straightforward: an item is added to a work queue (table), then a cron regularly scans through the table executing the items and deleting them. That means the cron trigger can just be added on `create` and things should work out fine. There's just two wrinkles in the port_forward cron: - It can be requeued in the future, so needs a conditional trigger-ing in `write`. - It is disabled during freeze (maybe something to change), as a result triggers don't enqueue at all, so we need to immediately trigger after freeze to force the cron re-enabling it.
2024-07-30 18:45:51 +07:00
env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
[IMP] forwardport: less spammy reminder cron * 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
2019-09-16 20:28:03 +07:00
assert pr1.comments == [
(users['reviewer'], 'hansen r+'),
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
seen(env, pr1, users),
[CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836
2024-06-03 20:29:47 +07:00
]
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
assert fail_pr.comments[2:] == [awaiting]*2,\
"message should not be triggered on closed PR"
with prod:
fail_pr.open()
with prod:
prod.post_status(fail_id.head, 'success')
assert fail_id.state == 'validated'
env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
assert fail_pr.comments[2:] == [awaiting]*3, "check that message triggers again"
with prod:
fail_pr.post_comment('hansen r+', config['role_reviewer']['token'])
assert fail_id.state == 'ready'
env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
assert fail_pr.comments[2:] == [
[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-23 19:25:07 +07:00
awaiting,
awaiting,
[FIX] forwardport: don't post reminder on PRs waiting to be staged The FW reminder is useful to remind people of the outstanding forward ports they need to process, as taking too long can be an issue. They are, however, not useful if the developer has already done everything, the PR is ready and unblocked, but the mergebot has fallen behind and has a hard time catching up. In that case there is nothing for the developer to do, so pinging them is not productive, it's only frustrating. Fixes #930
2024-09-06 17:33:51 +07:00
awaiting,
(users['reviewer'], "hansen r+"),
],"if a PR is ready (unblocked), the reminder should not trigger as "\
"we're likely waiting for the mergebot"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
def test_partially_empty(env, config, make_repo):
""" Check what happens when only some commits of the PR are now empty
"""
prod, other = make_basic(env, config, make_repo)
# merge change to b
with prod:
[p_0] = prod.make_commits(
'b', Commit('p', tree={'x': '0'}),
ref='heads/early'
)
pr0 = prod.make_pr(target='b', head='early')
prod.post_status(p_0, 'success', 'legal/cla')
prod.post_status(p_0, 'success', 'ci/runbot')
pr0.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.b', 'success', 'legal/cla')
prod.post_status('staging.b', 'success', 'ci/runbot')
# merge same change to a afterwards
with prod:
[*_, p_1] = prod.make_commits(
'a',
Commit('p_0', tree={'w': '0'}),
Commit('p_1', tree={'x': '0'}),
Commit('p_2', tree={'y': '0'}),
ref='heads/late'
)
pr1 = prod.make_pr(target='a', head='late')
prod.post_status(p_1, 'success', 'legal/cla')
prod.post_status(p_1, 'success', 'ci/runbot')
pr1.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
assert prod.read_tree(prod.commit('a')) == {
'f': 'e',
'w': '0',
'x': '0',
'y': '0',
}
assert prod.read_tree(prod.commit('b')) == {
'f': 'c',
'g': 'b',
'x': '0',
}
fail_id = env['runbot_merge.pull_requests'].search([
('number', 'not in', [pr0.number, pr1.number]),
('parent_id', '=', False),
], order='number')
assert fail_id
# unlinked from parent since cherrypick failed
assert not fail_id.parent_id
# the tree should be clean...
assert prod.read_tree(prod.commit(fail_id.head)) == {
'f': 'c',
'g': 'b',
'w': '0',
'x': '0',
'y': '0',
}
Case = collections.namedtuple('Case', 'author reviewer delegate success')
ACL = [
[CHG] forwardport: automatically delegate original PR author on FP PRs This way the original author can r+ the forward ports if they succeed (and probably requires no attention). Closes #195
2019-09-17 19:43:21 +07:00
Case('reviewer', 'reviewer', None, True),
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
Case('reviewer', 'self_reviewer', None, False),
Case('reviewer', 'other', None, False),
Case('reviewer', 'other', 'other', True),
Case('self_reviewer', 'reviewer', None, True),
Case('self_reviewer', 'self_reviewer', None, True),
Case('self_reviewer', 'other', None, False),
Case('self_reviewer', 'other', 'other', True),
Case('other', 'reviewer', None, True),
Case('other', 'self_reviewer', None, False),
[CHG] forwardport: automatically delegate original PR author on FP PRs This way the original author can r+ the forward ports if they succeed (and probably requires no attention). Closes #195
2019-09-17 19:43:21 +07:00
Case('other', 'other', None, True),
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
Case('other', 'other', 'other', True),
]
@pytest.mark.parametrize(Case._fields, ACL)
def test_access_rights(env, config, make_repo, users, author, reviewer, delegate, success):
[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-06 18:06:53 +07:00
"""Validates the review rights *for the forward-port sequence*, the original
PR is always reviewed by `user`.
"""
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
prod, other = make_basic(env, config, make_repo)
project = env['runbot_merge.project'].search([])
# create a partner for `user`
[IMP] runbot_merge: make review rights repo-dependent As the odds of having more projects or more repos with different requirements in the same project, the need to have different sets of reviewers for different repositories increases. As a result, rather than be trivial boolean flags the review info should probably depend on the user / partner and the repo. Turns out the permission checks had already been extracted into their own function so most of the mess comes from testing utilities which went and configured their review rights as needed. Incidentally it might be that the test suite could just use something like a sequence of commoditized accounts which get configured as needed and not even looked at unless they're used.
2020-02-10 21:05:08 +07:00
c = env['res.partner'].create({
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
'name': users['user'],
'github_login': users['user'],
[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-06 18:06:53 +07:00
'email': 'user@example.org',
[IMP] runbot_merge: make review rights repo-dependent As the odds of having more projects or more repos with different requirements in the same project, the need to have different sets of reviewers for different repositories increases. As a result, rather than be trivial boolean flags the review info should probably depend on the user / partner and the repo. Turns out the permission checks had already been extracted into their own function so most of the mess comes from testing utilities which went and configured their review rights as needed. Incidentally it might be that the test suite could just use something like a sequence of commoditized accounts which get configured as needed and not even looked at unless they're used.
2020-02-10 21:05:08 +07:00
})
c.write({
'review_rights': [
(0, 0, {'repository_id': repo.id, 'review': True})
for repo in project.repo_ids
]
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
})
[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-06 18:06:53 +07:00
# create a partner for `other` so we can put an email on it
env['res.partner'].create({
'name': users['other'],
'github_login': users['other'],
'email': 'other@example.org',
})
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
author_token = config['role_' + author]['token']
fork = prod.fork(token=author_token)
with prod, fork:
[c] = fork.make_commits('a', Commit('c_0', tree={'y': '0'}), ref='heads/accessrights')
pr = prod.make_pr(
target='a', title='my change',
head=users[author] + ':accessrights',
token=author_token,
)
prod.post_status(c, 'success', 'legal/cla')
prod.post_status(c, 'success', 'ci/runbot')
pr.post_comment('hansen r+', token=config['github']['token'])
if delegate:
pr.post_comment('hansen delegate=%s' % users[delegate], token=config['github']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
assert pr0.state == 'merged'
with prod:
prod.post_status(pr1.head, 'success', 'ci/runbot')
prod.post_status(pr1.head, 'success', 'legal/cla')
env.run_crons()
_, _, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
with prod:
prod.post_status(pr2.head, 'success', 'ci/runbot')
prod.post_status(pr2.head, 'success', 'legal/cla')
prod.get_pr(pr2.number).post_comment(
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
'hansen r+',
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
token=config['role_' + reviewer]['token']
)
env.run_crons()
if success:
assert pr1.staging_id and pr2.staging_id,\
"%s should have approved FP of PRs by %s" % (reviewer, author)
[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-02 14:54:58 +07:00
st = prod.commit('staging.b')
# Should be signed-off by both original reviewer and forward port reviewer
[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-09 18:21:24 +07:00
original_signoff = signoff(config['role_user'], st.message)
forward_signoff = signoff(config['role_' + reviewer], st.message)
assert st.message.index(original_signoff) <= st.message.index(forward_signoff),\
[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-02 14:54:58 +07:00
"Check that FP approver is after original PR approver as that's " \
"the review path for the PR"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
else:
assert not (pr1.staging_id or pr2.staging_id),\
"%s should *not* have approved FP of PRs by %s" % (reviewer, author)
[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-02 14:54:58 +07:00
def signoff(conf, message):
for n in filter(None, [conf.get('name'), conf.get('user')]):
signoff = 'Signed-off-by: ' + n
if signoff in message:
return signoff
raise AssertionError("Failed to find signoff by %s in %s" % (conf, message))
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[IMP] *: add test for r- on forward ports Apparently I'd already fixed that in 286c1fdaee12bce073c28a48e4e415e843de545f but it has yet to be deployed. While at it, add a feedback message to clarify that, unlike `r+`, `r-` on forward ports does *not* propagate. Fixes #912
2024-07-26 17:05:51 +07:00
def test_disapproval(env, config, make_repo, users):
"""The author of a source PR should be able to unapprove the forward port in
case they approved it then noticed an issue of something.
"""
# region setup
prod, _ = make_basic(env, config, make_repo, statuses='default')
env['res.partner'].create({
'name': users['other'],
'github_login': users['other'],
'email': 'other@example.org',
})
author_token = config['role_other']['token']
fork = prod.fork(token=author_token)
with prod, fork:
[c] = fork.make_commits('a', Commit('c_0', tree={'y': '0'}), ref='heads/accessrights')
pr0 = prod.make_pr(
target='a', title='my change',
head=users['other'] + ':accessrights',
token=author_token,
)
prod.post_status(c, 'success')
pr0.post_comment('hansen r+', token=config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success')
env.run_crons()
pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
assert pr1_id.source_id == pr0_id
pr1 = prod.get_pr(pr1_id.number)
assert pr0_id.state == 'merged'
with prod:
prod.post_status(pr1_id.head, 'success')
env.run_crons()
# endregion
_, _, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
pr2 = prod.get_pr(pr2_id.number)
with prod:
prod.post_status(pr2_id.head, 'success')
pr2.post_comment('hansen r+', token=config['role_other']['token'])
# no point creating staging for our needs, just propagate statuses
[IMP] *: rely only on triggers to run statuses propagation The cron had been converted to using mostly triggers in 7cd9afe7f2ff2cbaaabe0ecf6eb364b55aaf8d60 but I forgot to update the *tests* to avoid explicitly triggering it.
2024-07-30 18:42:00 +07:00
env.run_crons(None)
[IMP] *: add test for r- on forward ports Apparently I'd already fixed that in 286c1fdaee12bce073c28a48e4e415e843de545f but it has yet to be deployed. While at it, add a feedback message to clarify that, unlike `r+`, `r-` on forward ports does *not* propagate. Fixes #912
2024-07-26 17:05:51 +07:00
assert pr1_id.state == 'ready'
assert pr2_id.state == 'ready'
# oh no, pr1 has an error!
with prod:
pr1.post_comment('hansen r-', token=config['role_other']['token'])
[IMP] *: trigger-ify task queue type crons These are pretty simple to convert as they are straightforward: an item is added to a work queue (table), then a cron regularly scans through the table executing the items and deleting them. That means the cron trigger can just be added on `create` and things should work out fine. There's just two wrinkles in the port_forward cron: - It can be requeued in the future, so needs a conditional trigger-ing in `write`. - It is disabled during freeze (maybe something to change), as a result triggers don't enqueue at all, so we need to immediately trigger after freeze to force the cron re-enabling it.
2024-07-30 18:45:51 +07:00
env.run_crons(None)
[IMP] *: add test for r- on forward ports Apparently I'd already fixed that in 286c1fdaee12bce073c28a48e4e415e843de545f but it has yet to be deployed. While at it, add a feedback message to clarify that, unlike `r+`, `r-` on forward ports does *not* propagate. Fixes #912
2024-07-26 17:05:51 +07:00
assert pr1_id.state == 'validated', "pr1 should not be approved anymore"
assert pr2_id.state == 'ready', "pr2 should not be affected"
assert pr1.comments == [
seen(env, pr1, users),
(users['user'], 'This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.\n\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n'),
(users['other'], "hansen r-"),
(users['user'], "Note that only this forward-port has been unapproved, "
"sibling forward ports may have to be unapproved "
"individually."),
]
[FIX] *: unnecessary warning on r- of forward port Reminding users that `r-` on a forward port only unreviews *that* forwardport is useful, as `r+;r-` is not a no-op (all preceding siblings are still reviewed). However it is useless if all siblings are not approved or already merged. So avoid sending the warning in that case. Fixes #934
2024-09-06 18:04:13 +07:00
with prod:
pr2.post_comment('hansen r-', token=config['role_other']['token'])
env.run_crons(None)
assert pr2_id.state == 'validated'
assert pr2.comments[2:] == [
(users['other'], "hansen r+"),
(users['other'], "hansen r-"),
], "shouldn't have a warning on r- because it's the only approved PR of the chain"
[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-06 19:42:54 +07:00
[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-18 16:46:14 +07:00
def test_delegate_fw(env, config, make_repo, users):
"""If a user is delegated *on a forward port* they should be able to approve
*the followup*.
"""
prod, _ = make_basic(env, config, make_repo)
# create a partner for `other` so we can put an email on it
env['res.partner'].create({
'name': users['other'],
'github_login': users['other'],
'email': 'other@example.org',
})
author_token = config['role_self_reviewer']['token']
fork = prod.fork(token=author_token)
with prod, fork:
[c] = fork.make_commits('a', Commit('c_0', tree={'y': '0'}), ref='heads/accessrights')
pr = prod.make_pr(
target='a', title='my change',
head=users['self_reviewer'] + ':accessrights',
token=author_token,
)
prod.post_status(c, 'success', 'legal/cla')
prod.post_status(c, 'success', 'ci/runbot')
pr.post_comment('hansen r+', token=config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
# ensure pr1 has to be approved to be forward-ported
_, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
# detatch from source
[IMP] forwardport: store and display detachment reason 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
2023-01-19 20:23:41 +07:00
pr1_id.write({
'parent_id': False,
'detach_reason': "Detached for testing.",
})
[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-18 16:46:14 +07:00
with prod:
prod.post_status(pr1_id.head, 'success', 'legal/cla')
prod.post_status(pr1_id.head, 'success', 'ci/runbot')
env.run_crons()
pr1 = prod.get_pr(pr1_id.number)
# delegate review to "other" consider PR fixed, and have "other" approve it
with prod:
pr1.post_comment('hansen delegate=' + users['other'],
token=config['role_reviewer']['token'])
prod.post_status(pr1_id.head, 'success', 'ci/runbot')
pr1.post_comment('hansen r+', token=config['role_other']['token'])
env.run_crons()
with prod:
prod.post_status('staging.b', 'success', 'legal/cla')
prod.post_status('staging.b', 'success', 'ci/runbot')
env.run_crons()
_, _, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
pr2 = prod.get_pr(pr2_id.number)
# make "other" also approve this one
with prod:
prod.post_status(pr2_id.head, 'success', 'ci/runbot')
prod.post_status(pr2_id.head, 'success', 'legal/cla')
pr2.post_comment('hansen r+', token=config['role_other']['token'])
env.run_crons()
assert pr2.comments == [
seen(env, pr2, users),
[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-23 19:25:07 +07:00
(users['user'], '''@{self_reviewer} @{reviewer} this PR targets c and is the last of the forward-port chain.
[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-18 16:46:14 +07:00
[REF] *: move most feedback messages to pseudo-templates Current system makes it hard to iterate feedback messages and make them clearer, this should improve things a touch. Use a bespoke model to avoid concerns with qweb rendering complexity (we just want GFM output and should not need logic). Also update fwbot test setup to always configure an fwbot name, in order to avoid ping messages closing the PRs they're talking about, that took a while to debug, and given the old message I assume I'd already hit it and just been too lazy to fix. This requires updating a bunch of tests as fwbot ping are sent *to* `fp_github_name`, but sent *from* the reference user (because that's the key we set). Note: noupdate on CSV files doesn't seem to work anymore, which isn't great. But instead set tracking on the template's templates, it's not quite as good but should be sufficient. Fixes #769
2023-06-12 19:41:42 +07:00
To merge the full chain, use
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
> @hansen r+
[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-18 16:46:14 +07:00
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
'''.format_map(users)),
[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-18 16:46:14 +07:00
(users['other'], 'hansen r+')
]
[FIX] runbot_merge: missing feedback on fw r+ In some cases, feedback to the PR author that an r+ is redundant went missing. This turns out to be due to the convolution of the handling of approval on forward-port, and the fact that the target PR is treated exactly like its ancestors: if the PR is already approved the approval is not even attempted (and so no feedback if it's incorrect). Straighten up this bit and add a special case for the PR being commented on, it should have the usual feedback if in error or already commented on. Furthermore, update `PullRequests._pr_acl` to kinda work out of the box for forward-port: if the current PR is a forward port, `is_reviewer` should check delegation on all ancestors, there doesn't seem to be any reason to split "source_reviewer", "parent_reviewer", and "is_reviewer". Fixes #939
2024-09-05 18:02:10 +07:00
assert pr2_id.reviewed_by
[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-18 16:46:14 +07:00
[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-06 19:42:54 +07:00
def test_redundant_approval(env, config, make_repo, users):
"""If a forward port sequence has been partially approved, fw-bot r+ should
not perform redundant approval as that triggers warning messages.
"""
prod, _ = make_basic(env, config, make_repo)
[project] = env['runbot_merge.project'].search([])
with prod:
prod.make_commits(
'a', Commit('p', tree={'x': '0'}),
ref='heads/early'
)
pr0 = prod.make_pr(target='a', head='early')
prod.post_status('heads/early', 'success', 'legal/cla')
prod.post_status('heads/early', 'success', 'ci/runbot')
pr0.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number asc')
with prod:
prod.post_status(pr1_id.head, 'success', 'legal/cla')
prod.post_status(pr1_id.head, 'success', 'ci/runbot')
env.run_crons()
_, _, pr2_id = env['runbot_merge.pull_requests'].search([], order='number asc')
assert pr2_id.parent_id == pr1_id
assert pr1_id.parent_id == pr0_id
pr1 = prod.get_pr(pr1_id.number)
pr2 = prod.get_pr(pr2_id.number)
with prod:
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
with prod:
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
pr2.post_comment('hansen r+', config['role_reviewer']['token'])
[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-06 19:42:54 +07:00
env.run_crons()
assert pr1.comments == [
seen(env, pr1, users),
(users['user'], 'This PR targets b and is part of the forward-port chain. '
'Further PRs will be created up to c.\n\n'
'More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n'),
(users['reviewer'], 'hansen r+'),
]
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
def test_batched(env, config, make_repo, users):
""" Tests for projects with multiple repos & sync'd branches. Batches
should be FP'd to batches
"""
main1, _ = make_basic(env, config, make_repo, reponame='main1')
main2, _ = make_basic(env, config, make_repo, reponame='main2')
main1.unsubscribe(config['role_reviewer']['token'])
main2.unsubscribe(config['role_reviewer']['token'])
friendo = config['role_other']
other1 = main1.fork(token=friendo['token'])
other2 = main2.fork(token=friendo['token'])
with main1, other1:
[c1] = other1.make_commits(
'a', Commit('commit repo 1', tree={'1': 'a'}),
ref='heads/contribution'
)
pr1 = main1.make_pr(
target='a', title="My contribution",
head=friendo['user'] + ':contribution',
token=friendo['token']
)
# we can ack it directly as it should not be taken in account until
# we run crons
validate_all([main1], [c1])
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
with main2, other2:
[c2] = other2.make_commits(
'a', Commit('commit repo 2', tree={'2': 'a'}),
ref='heads/contribution' # use same ref / label as pr1
)
pr2 = main2.make_pr(
target='a', title="Main2 part of my contribution",
head=friendo['user'] + ':contribution',
token=friendo['token']
)
validate_all([main2], [c2])
pr2.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
# sanity check: this should have created a staging with 1 batch with pr1 and pr2
stagings = env['runbot_merge.stagings'].search([])
assert len(stagings) == 1
assert stagings.target.name == 'a'
assert len(stagings.batch_ids) == 1
assert stagings.mapped('batch_ids.prs.number') == [pr1.number, pr2.number]
with main1, main2:
validate_all([main1, main2], ['staging.a'])
env.run_crons()
PullRequests = env['runbot_merge.pull_requests']
# created the first forward port, need to validate it so the second one is
# triggered (FP only goes forward on CI+) (?)
pr1b = PullRequests.search([
('source_id', '!=', False),
('repository.name', '=', main1.name),
])
pr2b = PullRequests.search([
('source_id', '!=', False),
('repository.name', '=', main2.name),
])
# check that relevant users were pinged
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
ping = (users['user'], """\
[FIX] forwardport: clarify forward port message 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
2019-09-10 21:29:02 +07:00
This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
[FIX] forwardport: fix forward-port Odoo wiki link
2020-03-09 20:20:59 +07:00
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
""")
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
pr_remote_1b = main1.get_pr(pr1b.number)
pr_remote_2b = main2.get_pr(pr2b.number)
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
assert pr_remote_1b.comments == [seen(env, pr_remote_1b, users), ping]
assert pr_remote_2b.comments == [seen(env, pr_remote_2b, users), ping]
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with main1, main2:
validate_all([main1], [pr1b.head])
validate_all([main2], [pr2b.head])
env.run_crons() # process updated statuses -> generate followup FP
# should have created two PRs whose source is p1 and two whose source is p2
pr1a, pr1b, pr1c = PullRequests.search([
('repository.name', '=', main1.name),
], order='number')
pr2a, pr2b, pr2c = PullRequests.search([
('repository.name', '=', main2.name),
], order='number')
assert pr1a.number == pr1.number
assert pr2a.number == pr2.number
assert pr1a.state == pr2a.state == 'merged'
assert pr1b.label == pr2b.label, "batched source should yield batched FP"
assert pr1c.label == pr2c.label, "batched source should yield batched FP"
assert pr1b.label != pr1c.label
project = env['runbot_merge.project'].search([])
# ok main1 PRs
with main1:
validate_all([main1], [pr1c.head])
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
main1.get_pr(pr1c.number).post_comment('hansen r+', config['role_reviewer']['token'])
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
# check that the main1 PRs are ready but blocked on the main2 PRs
assert pr1b.state == 'ready'
assert pr1c.state == 'ready'
assert pr1b.blocked
assert pr1c.blocked
# ok main2 PRs
with main2:
validate_all([main2], [pr2c.head])
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
main2.get_pr(pr2c.number).post_comment('hansen r+', config['role_reviewer']['token'])
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
[REM] forwardport: fp_sequence field 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
2022-12-08 16:42:12 +07:00
env['runbot_merge.stagings'].search([]).mapped('target.display_name')
env['runbot_merge.stagings'].search([], order='target').mapped('target.display_name')
stc, stb = env['runbot_merge.stagings'].search([], order='target')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert stb.target.name == 'b'
assert stc.target.name == 'c'
with main1, main2:
validate_all([main1, main2], ['staging.b', 'staging.c'])
class TestClosing:
def test_closing_before_fp(self, env, config, make_repo, users):
""" Closing a PR should preclude its forward port
"""
prod, other = make_basic(env, config, make_repo)
with prod:
[p_1] = prod.make_commits(
'a',
Commit('p_0', tree={'x': '0'}),
ref='heads/hugechange'
)
pr = prod.make_pr(target='a', head='hugechange')
prod.post_status(p_1, 'success', 'legal/cla')
prod.post_status(p_1, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
# should merge the staging then create the FP PR
env.run_crons()
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# close the FP PR then have CI validate it
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
pr1 = prod.get_pr(pr1_id.number)
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with prod:
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
pr1.close()
assert pr1_id.state == 'closed'
assert not pr1_id.parent_id, "closed PR should should be detached from its parent"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with prod:
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
prod.post_status(pr1_id.head, 'success', 'legal/cla')
prod.post_status(pr1_id.head, 'success', 'ci/runbot')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
env.run_crons()
[IMP] *: trigger-ify task queue type crons These are pretty simple to convert as they are straightforward: an item is added to a work queue (table), then a cron regularly scans through the table executing the items and deleting them. That means the cron trigger can just be added on `create` and things should work out fine. There's just two wrinkles in the port_forward cron: - It can be requeued in the future, so needs a conditional trigger-ing in `write`. - It is disabled during freeze (maybe something to change), as a result triggers don't enqueue at all, so we need to immediately trigger after freeze to force the cron re-enabling it.
2024-07-30 18:45:51 +07:00
env.run_crons('forwardport.reminder')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
"closing the PR should suppress the FP sequence"
[CHG] runbot_merge, forwardport: remove labelling Because github materialises every labels change in the timeline (interspersed with comments), the increasing labels churn contributes to PRs being difficult to read and review. This change removes the update of labels on PRs, instead the mergebot will automatically send a comment to created PRs serving as a notification that the PR was noticed & providing a link to the mergebot's dashboard for that PR where users should be able to see the PR state in detail in case they wonder what's what. Lots of tests had to be edited to: - remove any check on the labels of the PR - add checks on the PR dashboard (to ensure that they're at least on the correct "view") - add a helper to handle the comment now added to every PR by the 'bot - since that helper is needed by both mergebot and forwardbot, the utils modules were unified and moved out of the odoo modules Probably relevant note: no test was added for the dashboard ACL, though since I had to explicitly unset the group on the repo used for tests for things to work it looks to me like it at least excludes people just fine. Fixes #419
2020-11-17 21:21:21 +07:00
assert pr1.comments == [
seen(env, pr1, users),
[FIX] forwardport: clarify forward port message 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
2019-09-10 21:29:02 +07:00
(users['user'], """\
This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
[FIX] forwardport: fix forward-port Odoo wiki link
2020-03-09 20:20:59 +07:00
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
[FIX] forwardport: clarify forward port message 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
2019-09-10 21:29:02 +07:00
""")
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
]
[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-29 15:58:13 +07:00
def test_closing_after_fp(self, env, config, make_repo, users):
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
""" Closing a PR which has been forward-ported should not touch the
followups
"""
prod, other = make_basic(env, config, make_repo)
[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-29 15:58:13 +07:00
project = env['runbot_merge.project'].search([])
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with prod:
[p_1] = prod.make_commits(
'a',
Commit('p_0', tree={'x': '0'}),
ref='heads/hugechange'
)
pr = prod.make_pr(target='a', head='hugechange')
prod.post_status(p_1, 'success', 'legal/cla')
prod.post_status(p_1, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
# should merge the staging then create the FP PR
env.run_crons()
[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-29 15:58:13 +07:00
pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with prod:
[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-29 15:58:13 +07:00
prod.post_status(pr1_id.head, 'success', 'legal/cla')
prod.post_status(pr1_id.head, 'success', 'ci/runbot')
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
# should create the second staging
env.run_crons()
[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-29 15:58:13 +07:00
pr0_id2, pr1_id2, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
assert pr0_id2 == pr0_id
assert pr1_id2 == pr1_id
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[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-29 15:58:13 +07:00
pr1 = prod.get_pr(pr1_id.number)
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with prod:
[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-29 15:58:13 +07:00
pr1.close()
assert pr1_id.state == 'closed'
assert not pr1_id.parent_id
assert pr2_id.state == 'opened'
assert not pr2_id.parent_id, \
"the descendant of a closed PR doesn't really make sense, maybe?"
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[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-29 15:58:13 +07:00
with prod:
pr1.open()
assert pr1_id.state == 'validated'
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
assert not pr1_id.parent_id
assert not pr2_id.parent_id
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[FIX] forwardport: suppress warning when closing unmanaged PR Continuation of 327500bc8356d242be655af1ae07f916c80efed6 for an other edge case of closing a PR to a detached branch with a merged descendant. The mergebot would: - warn on the parent about it being detached due to being closed - then warn on the child about it being detached due to the parent being closed (despite it being merged already) - then warn the parent *again* due to the child being detached At least some of those messages were still produced by the test case, stop them. Issue was noticed on odoo/odoo#145969 and odoo/odoo#145984 due to 16.2 being deactivated.
2024-03-19 17:46:36 +07:00
def test_close_disabled(self, env, make_repo, users, config):
""" If an fwport's target is disabled and its branch is closed, it
should not be notified (multiple times), also its descendant should not
be nodified if already merged, also there should not be recursive
notifications (odoo/odoo#145969, odoo/odoo#145984)
"""
repo, _ = make_basic(env, config, make_repo)
env['runbot_merge.repository'].search([]).required_statuses = 'default'
# prep: merge PR, create two forward ports
with repo:
[c1] = repo.make_commits('a', Commit('first', tree={'m': 'c1'}))
pr1 = repo.make_pr(title='title', body='body', target='a', head=c1)
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
repo.post_status(c1, 'success')
env.run_crons()
pr1_id = to_pr(env, pr1)
assert pr1_id.state == 'ready', pr1_id.blocked
with repo:
repo.post_status('staging.a', 'success')
env.run_crons()
pr1_id_, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
assert pr1_id_ == pr1_id
with repo:
repo.post_status(pr2_id.head, 'success')
env.run_crons()
_, _, pr3_id = env['runbot_merge.pull_requests'].search([], order='number')
# disable second branch
pr2_id.target.active = False
env.run_crons()
pr2 = repo.get_pr(pr2_id.number)
assert pr2.comments == [
seen(env, pr2, users),
(users['user'], "This PR targets b and is part of the forward-port chain. "
"Further PRs will be created up to c.\n\n"
"More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n"),
(users['user'], "@{user} @{reviewer} the target branch 'b' has been disabled, you may want to close this PR.".format_map(
users
)),
]
pr3 = repo.get_pr(pr3_id.number)
assert pr3.comments == [
seen(env, pr3, users),
(users['user'], """\
@{user} @{reviewer} this PR targets c and is the last of the forward-port chain containing:
* {pr2_id.display_name}
To merge the full chain, use
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
> @hansen r+
[FIX] forwardport: suppress warning when closing unmanaged PR Continuation of 327500bc8356d242be655af1ae07f916c80efed6 for an other edge case of closing a PR to a detached branch with a merged descendant. The mergebot would: - warn on the parent about it being detached due to being closed - then warn on the child about it being detached due to the parent being closed (despite it being merged already) - then warn the parent *again* due to the child being detached At least some of those messages were still produced by the test case, stop them. Issue was noticed on odoo/odoo#145969 and odoo/odoo#145984 due to 16.2 being deactivated.
2024-03-19 17:46:36 +07:00
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
""".format(pr2_id=pr2_id, **users)),
]
# some time later, notice PR3 is open and merge it
with repo:
pr3.post_comment('hansen r+', config['role_reviewer']['token'])
repo.post_status(pr3.head, 'success')
env.run_crons()
with repo:
repo.post_status('staging.c', 'success')
env.run_crons()
assert pr3_id.status == 'success'
# even later, notice PR2 is still open but not mergeable anymore
with repo:
pr2.close()
env.run_crons()
assert pr2.comments[3:] == []
assert pr3.comments[2:] == [(users['reviewer'], "hansen r+")]
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
class TestBranchDeletion:
def test_delete_normal(self, env, config, make_repo):
""" Regular PRs should get their branch deleted as long as they're
created in the fp repository
"""
prod, other = make_basic(env, config, make_repo)
with prod, other:
[c] = other.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/abranch')
pr = prod.make_pr(
target='a', head='%s:abranch' % other.owner,
title="a pr",
)
prod.post_status(c, 'success', 'legal/cla')
prod.post_status(c, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
[IMP] *: modernise tests via `to_pr` The `to_pr` helper was added a *while* ago to replace the pretty verbose process of looking a PR with the right number in the right repository after a `make_pr`. However while I did some ad-hoc replacement of existing `search` calls as I had to work with existing tests I never did a full search and replace to try and excise searches from the test suite. This is now done. I've undoubtedly missed a few, but a hundred-odd lines of codes have been simplified.
2024-12-02 20:17:28 +07:00
pr_id = to_pr(env, pr)
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
assert pr_id.state == 'merged'
removers = env['forwardport.branch_remover'].search([])
to_delete_branch = removers.mapped('pr_id')
assert to_delete_branch == pr_id
env.run_crons('forwardport.remover', context={'forwardport_merged_before': FAKE_PREV_WEEK})
with pytest.raises(AssertionError, match="Not Found"):
other.get_ref('heads/abranch')
def test_not_merged(self, env, config, make_repo):
""" The branches of PRs which are still open or have been closed (rather
than merged) should not get deleted
"""
prod, other = make_basic(env, config, make_repo)
with prod, other:
[c] = other.make_commits('a', Commit('c1', tree={'1': '0'}), ref='heads/abranch')
pr1 = prod.make_pr(target='a', head='%s:abranch' % other.owner, title='a')
prod.post_status(c, 'success', 'legal/cla')
prod.post_status(c, 'success', 'ci/runbot')
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
[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 14:18:17 +07:00
other.make_commits('a', Commit('c2', tree={'2': '0'}), ref='heads/bbranch')
[ADD] forwardport: automatic branch deleter 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
2019-10-16 19:41:26 +07:00
pr2 = prod.make_pr(target='a', head='%s:bbranch' % other.owner, title='b')
pr2.close()
[c] = other.make_commits('a', Commit('c3', tree={'3': '0'}), ref='heads/cbranch')
pr3 = prod.make_pr(target='a', head='%s:cbranch' % other.owner, title='c')
prod.post_status(c, 'success', 'legal/cla')
prod.post_status(c, 'success', 'ci/runbot')
other.make_commits('a', Commit('c3', tree={'4': '0'}), ref='heads/dbranch')
pr4 = prod.make_pr(target='a', head='%s:dbranch' % other.owner, title='d')
pr4.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
PR = env['runbot_merge.pull_requests']
# check PRs are in states we expect
pr_heads = []
for p, st in [(pr1, 'ready'), (pr2, 'closed'), (pr3, 'validated'), (pr4, 'approved')]:
p_id = PR.search([
('repository.name', '=', prod.name),
('number', '=', p.number),
])
assert p_id.state == st
pr_heads.append(p_id.head)
env.run_crons('forwardport.remover', context={'forwardport_merged_before': FAKE_PREV_WEEK})
# check that the branches still exist
assert other.get_ref('heads/abranch') == pr_heads[0]
assert other.get_ref('heads/bbranch') == pr_heads[1]
assert other.get_ref('heads/cbranch') == pr_heads[2]
assert other.get_ref('heads/dbranch') == pr_heads[3]
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
def sPeNgBaB(s):
return ''.join(
l if i % 2 == 0 else l.upper()
for i, l in enumerate(s)
)
def test_spengbab():
assert sPeNgBaB("spongebob") == 'sPoNgEbOb'
class TestRecognizeCommands:
def make_pr(self, env, config, make_repo):
r, _ = make_basic(env, config, make_repo)
with r:
r.make_commits('c', Commit('p', tree={'x': '0'}), ref='heads/testbranch')
pr = r.make_pr(target='a', head='testbranch')
[IMP] *: modernise tests via `to_pr` The `to_pr` helper was added a *while* ago to replace the pretty verbose process of looking a PR with the right number in the right repository after a `make_pr`. However while I did some ad-hoc replacement of existing `search` calls as I had to work with existing tests I never did a full search and replace to try and excise searches from the test suite. This is now done. I've undoubtedly missed a few, but a hundred-odd lines of codes have been simplified.
2024-12-02 20:17:28 +07:00
return r, pr, to_pr(env, pr)
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
# FIXME: remove / merge into mergebot tests
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
def test_botname_casing(self, env, config, make_repo):
""" Test that the botname is case-insensitive as people might write
bot names capitalised or titlecased or uppercased or whatever
"""
repo, pr, pr_id = self.make_pr(env, config, make_repo)
assert pr_id.state == 'opened'
[a] = env['runbot_merge.branch'].search([
('name', '=', 'a')
])
names = [
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
"hansen",
"HANSEN",
"Hansen",
sPeNgBaB("hansen"),
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
]
for n in names:
[CHG] *: move forward-porting over to batches Thank god I have a bunch of tests because once again I forgot / missed a bunch of edge cases in doing the conversion, which the tests caught (sadly that means I almost certainly broke a few untested edge cases). Important notes: Handling of parent links ------------------------ Unlike PRs, batches don't lose their parent info ever, the link is permanent, which is convenient to trawl through a forward port (currently implemented very inefficiently, maybe we'll optimise that in the future). However this means the batch having a parent and the batch's PRs having parents are slightly different informations, one of the edge cases I missed is that of conflicting PRs, which are deparented and have to be merged by hand before being forward ported further, I had originally replaced the checks on a pr and its sibling having parents by just the batch. Batches & targets ----------------- Batches were originally concepted as being fixed to a target and PRs having that target, a PR being retargeted would move it from one batch to an other. As it turns out this does not work in the case where people retarget forward-port PRs, which I know they do because #551 (2337bd85186de000d074e5a7178cfeb110d15de7). I could not think of a good way to handle this issue as is, so scrapped the moving PRs thing, instead one of the coherence checks of a batch being ready is that all its PRs have the same target, and a batch only has a target if all its PRs have the same target. It's possible for somewhat odd effects to arise, notably if a PR is closed (removed from batch), the other PRs are retargeted, and the new PR is reopened, it will now be on a separate batch even if it also gets retargeted. This is weird. I don't quite know how I should handle it, maybe batches could merge if they have the same target and label? however batches don't currently have a label so... Improve limits -------------- Keep limits on the PRs rather than lift them on the batchL if we can add/remove PRs of batches having different limits on different PRs of the same batch is reasonable. Also leave limit unset by default: previously, the limit was eagerly set to the tip (accessible) branch. That doesn't really seem necessary, so stop doing that. Also remove completely unnecessary `max` when trying to find a PR's next target: `root` is either `self` or `self.source_id`, so it should not be possible for that to have a later target. And for now ensure the limits are consistent per batch: a PR defaults to the limit of their batch-mate if they don't have one, and if a limit is set via command it's set on all PRs of a batch. This commit does not allow differential limits via commands, they are allowed via the backend but not really tested. The issue is mostly that it's not clear what the UX should look like to have clear and not super error prone interactions. So punt on it for now, and hopefully there's no hole I missed which will create inconsistent batches.
2024-05-21 20:45:53 +07:00
assert not pr_id.limit_id
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with repo:
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
pr.post_comment(f'@{n} up to a', config['role_reviewer']['token'])
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert pr_id.limit_id == a
# reset state
[CHG] *: move forward-porting over to batches Thank god I have a bunch of tests because once again I forgot / missed a bunch of edge cases in doing the conversion, which the tests caught (sadly that means I almost certainly broke a few untested edge cases). Important notes: Handling of parent links ------------------------ Unlike PRs, batches don't lose their parent info ever, the link is permanent, which is convenient to trawl through a forward port (currently implemented very inefficiently, maybe we'll optimise that in the future). However this means the batch having a parent and the batch's PRs having parents are slightly different informations, one of the edge cases I missed is that of conflicting PRs, which are deparented and have to be merged by hand before being forward ported further, I had originally replaced the checks on a pr and its sibling having parents by just the batch. Batches & targets ----------------- Batches were originally concepted as being fixed to a target and PRs having that target, a PR being retargeted would move it from one batch to an other. As it turns out this does not work in the case where people retarget forward-port PRs, which I know they do because #551 (2337bd85186de000d074e5a7178cfeb110d15de7). I could not think of a good way to handle this issue as is, so scrapped the moving PRs thing, instead one of the coherence checks of a batch being ready is that all its PRs have the same target, and a batch only has a target if all its PRs have the same target. It's possible for somewhat odd effects to arise, notably if a PR is closed (removed from batch), the other PRs are retargeted, and the new PR is reopened, it will now be on a separate batch even if it also gets retargeted. This is weird. I don't quite know how I should handle it, maybe batches could merge if they have the same target and label? however batches don't currently have a label so... Improve limits -------------- Keep limits on the PRs rather than lift them on the batchL if we can add/remove PRs of batches having different limits on different PRs of the same batch is reasonable. Also leave limit unset by default: previously, the limit was eagerly set to the tip (accessible) branch. That doesn't really seem necessary, so stop doing that. Also remove completely unnecessary `max` when trying to find a PR's next target: `root` is either `self` or `self.source_id`, so it should not be possible for that to have a later target. And for now ensure the limits are consistent per batch: a PR defaults to the limit of their batch-mate if they don't have one, and if a limit is set via command it's set on all PRs of a batch. This commit does not allow differential limits via commands, they are allowed via the backend but not really tested. The issue is mostly that it's not clear what the UX should look like to have clear and not super error prone interactions. So punt on it for now, and hopefully there's no hole I missed which will create inconsistent batches.
2024-05-21 20:45:53 +07:00
pr_id.limit_id = False
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
# FIXME: remove / merge into mergebot tests
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
@pytest.mark.parametrize('indent', ['', '\N{SPACE}', '\N{SPACE}'*4, '\N{TAB}'])
def test_botname_indented(self, env, config, make_repo, indent):
""" matching botname should ignore leading whitespaces
"""
repo, pr, pr_id = self.make_pr(env, config, make_repo)
assert pr_id.state == 'opened'
[a] = env['runbot_merge.branch'].search([
('name', '=', 'a')
])
[CHG] *: move forward-porting over to batches Thank god I have a bunch of tests because once again I forgot / missed a bunch of edge cases in doing the conversion, which the tests caught (sadly that means I almost certainly broke a few untested edge cases). Important notes: Handling of parent links ------------------------ Unlike PRs, batches don't lose their parent info ever, the link is permanent, which is convenient to trawl through a forward port (currently implemented very inefficiently, maybe we'll optimise that in the future). However this means the batch having a parent and the batch's PRs having parents are slightly different informations, one of the edge cases I missed is that of conflicting PRs, which are deparented and have to be merged by hand before being forward ported further, I had originally replaced the checks on a pr and its sibling having parents by just the batch. Batches & targets ----------------- Batches were originally concepted as being fixed to a target and PRs having that target, a PR being retargeted would move it from one batch to an other. As it turns out this does not work in the case where people retarget forward-port PRs, which I know they do because #551 (2337bd85186de000d074e5a7178cfeb110d15de7). I could not think of a good way to handle this issue as is, so scrapped the moving PRs thing, instead one of the coherence checks of a batch being ready is that all its PRs have the same target, and a batch only has a target if all its PRs have the same target. It's possible for somewhat odd effects to arise, notably if a PR is closed (removed from batch), the other PRs are retargeted, and the new PR is reopened, it will now be on a separate batch even if it also gets retargeted. This is weird. I don't quite know how I should handle it, maybe batches could merge if they have the same target and label? however batches don't currently have a label so... Improve limits -------------- Keep limits on the PRs rather than lift them on the batchL if we can add/remove PRs of batches having different limits on different PRs of the same batch is reasonable. Also leave limit unset by default: previously, the limit was eagerly set to the tip (accessible) branch. That doesn't really seem necessary, so stop doing that. Also remove completely unnecessary `max` when trying to find a PR's next target: `root` is either `self` or `self.source_id`, so it should not be possible for that to have a later target. And for now ensure the limits are consistent per batch: a PR defaults to the limit of their batch-mate if they don't have one, and if a limit is set via command it's set on all PRs of a batch. This commit does not allow differential limits via commands, they are allowed via the backend but not really tested. The issue is mostly that it's not clear what the UX should look like to have clear and not super error prone interactions. So punt on it for now, and hopefully there's no hole I missed which will create inconsistent batches.
2024-05-21 20:45:53 +07:00
assert not pr_id.limit_id
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
with repo:
[CHG] *: rewrite commands set, rework status management This commit revisits the commands set in order to make it more regular, and limit inconsistent command-sets, although it includes pseudo-command aliases for common tasks now removed from the core set. Hard Errors =========== The previous iteration of the commands set would ignore any non-command term in a command line. This has been changed to hard error (and ignoring the entire thing) if any command is unknown or invalid. This fixes inconsistent / unexpected interpretations where a user sends a command, then writes a novel on the same line some words of which happen to *also* be commands, leading to merge states they did not expect. They should now be told to fuck off. Priority Restructuring ---------------------- The numerical priority system was pretty messy in that it confused "staging priority" (in ways which were not entirely straightforward) with overrides to other concerns. This has now being split along all the axis, with separate command subsets for: - staging prioritisation, now separated between `default`, `priority`, and `alone`, - `default` means PRs are picked by an unspecified order when creating a staging, if nothing better is available - `priority` means PRs are picked first when staging, however if `priority` PRs don't fill the staging the rest will be filled with `default`, this mode did not previously exist - `alone` means the PRs are picked first, before splits, and only `alone` PRs can be part of the staging (which usually matches the modename) - `skipchecks` overrides both statuses and approval checks, for the batch, something previously implied in `p=0`, but now independent. Setting `skipchecks` basically makes the entire batch `ready`. For consistency this also sets the reviewer implicitly: since skipchecks overrides both statuses *and approval*, whoever enables this mode is essentially the reviewer. - `cancel` cancels any ongoing staging when the marked PR becomes ready again, previously this was also implied (in a more restricted form) by setting `p=0` FWBot removal ============= While the "forwardport bot" still exists as an API level (to segregate access rights between tokens) it has been removed as an interaction point, as part of the modules merge plan. As a result, fwbot stops responding ---------------------- Feedback messages are now always sent by the mergebot, the forward-porting bot should not send any message or notification anymore. commands moved to the merge bot ------------------------------- - `ignore`/`up to` simply changes bot - `close` as well - `skipci` is now a choice / flag of an `fw` command, which denotes the forward-port policy, - `fw=default` is the old `ci` and resets the policy to default, that is wait for the PR to be merged to create forward ports, and for the required statuses on each forward port to be received before creating the next - `fw=skipci` is the old `skipci`, it waits for the merge of the base PR but then creates all the forward ports immediately (unless it gets a conflict) - `fw=skipmerge` immediately creates all the forward ports, without even waiting for the PR to be merged This is a completely new mode, and may be rather broken as until now the 'bot has always assumed the source PR had been merged. approval rework --------------- Because of the previous section, there is no distinguishing feature between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge this PR and all its parent with different access rights". As a result, the two have been merged under a single `mergebot r+` with heuristics attempting to provide the best experience: - if approving a non-forward port, the behavior does not change - else, with review rights on the source, all ancestors are approved - else, as author of the original, approves all ancestors which descend from a merged PR - else, approves all ancestors up to and including the oldest ancestor to which we have review rights Most notably, the source's author is not delegated on the source or any of its descendants anymore. This might need to be revisited if it provides too restrictive. For the very specialized need of approving a forward-port *and none of its ancestors*, `review=` can now take a comma (`,`) separated list of pull request numbers (github numbers, not mergebot ids). Computed State ============== The `state` field of pull requests is now computed. Hopefully this makes the status more consistent and predictable in the long run, and importantly makes status management more reliable (because reference datum get updated naturally flowing to the state). For now however it makes things more complicated as some of the states have to be separately signaled or updated: - `closed` and `error` are now separate flags - `merge_date` is pulled down from forwardport and becomes the transition signal for ready -> merged - `reviewed_by` becomes the transition signal for approval (might be a good idea to rename it...) - `status` is computed from the head's statuses and overrides, and *that* becomes the validation state Ideally, batch-level flags like `skipchecks` should be on, well, the batch, and `state` should have a dependency on the batch. However currently the batch is not a durable / permanent member of the system, so it's a PR-level flag and a messy pile. On notable change is that *forcing* the state to `ready` now does that but also sets the reviewer, `skipchecks`, and overrides to ensure the API-mediated readying does not get rolled back by e.g. the runbot sending a status. This is useful for a few types of automated / programmatic PRs e.g. translation exports, where we set the state programmatically to limit noise. recursive dependency hack ------------------------- Given a sequence of PRs with an override of the source, if one of the PRs is updated its descendants should not have the override anymore. However if the updated PR gets overridden, its descendants should have *that* override. This requires some unholy manipulations via an override of `modified`, as the ORM supports recursive fields but not recursive dependencies (on a different field). unconditional followup scheduling --------------------------------- Previously scheduling forward-port followup was contigent on the FW policy, but it's not actually correct if the new PR is *immediately* validated (which can happen now that the field is computed, if there are no required statuses *or* all of the required statuses are overridden by an ancestor) as nothing will trigger the state change and thus scheduling of the fp followup. The followup function checks all the properties of the batch to port, so this should not result on incorrect ports. Although it's a bit more expensive, and will lead to more spam. Previously this would not happen because on creation of a PR the validation task (commit -> PR) would still have to execute. Misc Changes ============ - If a PR is marked as overriding / canceling stagings, it now does so on retry not just when setting initially. This was not handled at all previously, so a PR in P0 going into error due to e.g. a non-deterministic bug would be retried and still p=0, but a current staging would not get cancelled. Same when a PR in p=0 goes into error because something was failed, then is updated with a fix. - Add tracking to a bunch of relevant PR fields. Post-mortem analysis currently generally requires going through the text logs to see what happened, which is annoying. There is a nondeterminism / inconsistency in the tracking which sometimes leads the admin user to trigger tracking before the bot does, leading to the staging tracking being attributed to them during tests, shove under the carpet by ignoring the user to whom that tracking is attributed. When multiple users update tracked fields in the same transaction all the changes are attributed to the first one having triggered tracking (?), I couldn't find why the admin sometimes takes over. - added and leveraged support for enum-backed selection fields - moved variuous fields from forwardport to runbot_merge - fix a migration which had never worked and which never run (because I forgot to bump the version on the module) - remove some unnecessary intermediate de/serialisation fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
pr.post_comment(f'{indent}@hansen up to a', config['role_reviewer']['token'])
[ADD] forwardbot * Cherrypicking is handrolled because there seems to be no easy way to programmatically edit commit messages during the cherrypicking sequence: `-n` basically squashes all commits and `-e` invokes a subprocess. `-e` with `VISUAL=false` kinda sorta works (in that it interrupts the process before each commit), however there doesn't seem to be clean status codes so it's difficult to know if the cherrypick failed or if it's just waiting for a commit of this step. Instead, cherrypick commits individually then edit / rewrite their commit messages: * add a reference to the original commit * convert signed-off-by to something else as the original commit was signed off but not necessarily this one * Can't assign users when creating PRs: only repository collaborators or people who commented on the issue / PR (which we're in the process of creating) can be assigned. PR authors are as likely to be collaborators as not, and we can have non-collaborator reviewers. So pinging via a regular comment seems less fraught as a way to notify users.
2019-08-23 21:16:30 +07:00
assert pr_id.limit_id == a
Reference in New Issue Copy Permalink