2021-02-24 16:06:27 +07:00
|
|
|
"""
|
|
|
|
Test cases for updating PRs during after the forward-porting process after the
|
|
|
|
initial merge has succeeded (and forward-porting has started)
|
|
|
|
"""
|
2024-03-12 20:57:50 +07:00
|
|
|
import pytest
|
|
|
|
|
2024-06-04 17:15:29 +07:00
|
|
|
from utils import seen, matches, Commit, make_basic, to_pr
|
2021-07-27 14:17:18 +07:00
|
|
|
|
2021-02-24 16:06:27 +07:00
|
|
|
|
2024-03-12 20:57:50 +07:00
|
|
|
@pytest.mark.parametrize("merge_parent", [False, True])
|
|
|
|
def test_update_pr(env, config, make_repo, users, merge_parent) -> None:
|
2021-02-24 16:06:27 +07:00
|
|
|
""" Even for successful cherrypicks, it's possible that e.g. CI doesn't
|
|
|
|
pass or the reviewer finds out they need to update the code.
|
|
|
|
|
|
|
|
In this case, all following forward ports should... be detached? Or maybe
|
|
|
|
only this one and its dependent should be updated?
|
|
|
|
"""
|
2025-02-06 20:33:40 +07:00
|
|
|
prod, _ = make_basic(env, config, make_repo, statuses='ci/runbot,legal/cla')
|
2023-08-29 20:59:05 +07:00
|
|
|
# create a branch d from c so we can have 3 forward ports PRs, not just 2,
|
|
|
|
# for additional checks
|
|
|
|
env['runbot_merge.project'].search([]).write({
|
|
|
|
'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})]
|
|
|
|
})
|
|
|
|
with prod:
|
|
|
|
prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d')
|
[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
|
|
|
|
2021-02-24 16:06:27 +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')
|
|
|
|
pr.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
|
[FIX] *: re-enable notification on status failure
If a PR gets approved *then* fails CI, there should be a notification
warning the author & reviewer since
48e08b657b23760e6d45f397cf59e7710bc7f89a, it even has a test, which
passes (in fact it has *two*, one of which is redundant, so merge
`test_ci_failure_after_review` into the later `test_ci_approved`).
*However* this is in runbot_merge, turns out in
fafa7ef437d75d72e34be91af72e16c52135b7fa some refactoring was done in
order to override the notification and customise it for *forward
ports* with a failed status... except that override never called its
`super()`, so as soon as forwardport is installed the base
notification stops working, and that's been that since October
2019 (had been added in March that year, ignoring deployment lag).
This can be revealed by adding the corresponding check in the
*forwardport* tests, revealing the failure.
This was a pain to track down, thankfully it reproduced relatively
easily locally.
While this could be resolved in the override, might as well fold it
into the base method in furtherance of #789: the mergebot is only
used by odoo, and only with both modules combined, so splitting them
is not useful. And furthermore it things should work fine with the
forwardport installed but unused.
Fixes #894
2024-06-21 15:27:01 +07:00
|
|
|
prod.post_status(p_1, 'success', 'legal/cla')
|
|
|
|
prod.post_status(p_1, 'failure', 'ci/runbot')
|
2021-02-24 16:06:27 +07:00
|
|
|
env.run_crons()
|
[FIX] *: re-enable notification on status failure
If a PR gets approved *then* fails CI, there should be a notification
warning the author & reviewer since
48e08b657b23760e6d45f397cf59e7710bc7f89a, it even has a test, which
passes (in fact it has *two*, one of which is redundant, so merge
`test_ci_failure_after_review` into the later `test_ci_approved`).
*However* this is in runbot_merge, turns out in
fafa7ef437d75d72e34be91af72e16c52135b7fa some refactoring was done in
order to override the notification and customise it for *forward
ports* with a failed status... except that override never called its
`super()`, so as soon as forwardport is installed the base
notification stops working, and that's been that since October
2019 (had been added in March that year, ignoring deployment lag).
This can be revealed by adding the corresponding check in the
*forwardport* tests, revealing the failure.
This was a pain to track down, thankfully it reproduced relatively
easily locally.
While this could be resolved in the override, might as well fold it
into the base method in furtherance of #789: the mergebot is only
used by odoo, and only with both modules combined, so splitting them
is not useful. And furthermore it things should work fine with the
forwardport installed but unused.
Fixes #894
2024-06-21 15:27:01 +07:00
|
|
|
|
|
|
|
assert pr.comments == [
|
|
|
|
(users['reviewer'], 'hansen r+'),
|
|
|
|
seen(env, pr, users),
|
|
|
|
(users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)),
|
|
|
|
]
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
prod.post_status(p_1, 'success', 'ci/runbot')
|
|
|
|
env.run_crons()
|
|
|
|
|
2021-02-24 16:06:27 +07:00
|
|
|
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()
|
|
|
|
|
|
|
|
pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
|
|
|
|
fp_intermediate = (users['user'], '''\
|
2023-08-29 20:59:05 +07:00
|
|
|
This PR targets b and is part of the forward-port chain. Further PRs will be created up to d.
|
2021-02-24 16:06:27 +07:00
|
|
|
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
|
|
''')
|
2022-06-23 19:25:07 +07:00
|
|
|
ci_warning = (users['user'], '@%(user)s @%(reviewer)s ci/runbot failed on this forward-port PR' % users)
|
2021-02-24 16:06:27 +07:00
|
|
|
|
|
|
|
# oh no CI of the first FP PR failed!
|
|
|
|
# simulate status being sent multiple times (e.g. on multiple repos) with
|
|
|
|
# some delivery lag allowing for the cron to run between each delivery
|
|
|
|
for st, ctx in [('failure', 'ci/runbot'), ('failure', 'ci/runbot'), ('success', 'legal/cla'), ('success', 'legal/cla')]:
|
|
|
|
with prod:
|
|
|
|
prod.post_status(pr1_id.head, st, ctx)
|
|
|
|
env.run_crons()
|
|
|
|
with prod: # should be ignored because the description doesn't matter
|
|
|
|
prod.post_status(pr1_id.head, 'failure', 'ci/runbot', description="HAHAHAHAHA")
|
|
|
|
env.run_crons()
|
|
|
|
# check that FP did not resume & we have a ping on the PR
|
|
|
|
assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
|
|
|
|
"forward port should not continue on CI failure"
|
|
|
|
pr1_remote = prod.get_pr(pr1_id.number)
|
|
|
|
assert pr1_remote.comments == [seen(env, pr1_remote, users), fp_intermediate, ci_warning]
|
|
|
|
|
|
|
|
# it was a false positive, rebuild... it fails again!
|
|
|
|
with prod:
|
|
|
|
prod.post_status(pr1_id.head, 'failure', 'ci/runbot', target_url='http://example.org/4567890')
|
|
|
|
env.run_crons()
|
|
|
|
# check that FP did not resume & we have a ping on the PR
|
|
|
|
assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
|
|
|
|
"ensure it still hasn't restarted"
|
|
|
|
assert pr1_remote.comments == [seen(env, pr1_remote, users), fp_intermediate, ci_warning, ci_warning]
|
|
|
|
|
|
|
|
# nb: updating the head would detach the PR and not put it in the warning
|
|
|
|
# path anymore
|
|
|
|
|
|
|
|
# rebuild again, finally passes
|
|
|
|
with prod:
|
|
|
|
prod.post_status(pr1_id.head, 'success', 'ci/runbot')
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
assert pr1_id.parent_id == pr0_id
|
|
|
|
assert pr2_id.parent_id == pr1_id
|
|
|
|
pr1_head = pr1_id.head
|
|
|
|
pr2_head = pr2_id.head
|
|
|
|
|
|
|
|
# turns out branch b is syntactically but not semantically compatible! It
|
|
|
|
# needs x to be 5!
|
|
|
|
pr_repo, pr_ref = prod.get_pr(pr1_id.number).branch
|
|
|
|
with pr_repo:
|
|
|
|
# force-push correct commit to PR's branch
|
|
|
|
[new_c] = pr_repo.make_commits(
|
2025-02-06 18:35:55 +07:00
|
|
|
prod.commit(pr1_id.target.name).id,
|
2021-02-24 16:06:27 +07:00
|
|
|
Commit('whop whop', tree={'x': '5'}),
|
|
|
|
ref='heads/%s' % pr_ref,
|
|
|
|
make=False
|
|
|
|
)
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
assert pr1_id.head == new_c != pr1_head, "the FP PR should be updated"
|
|
|
|
assert not pr1_id.parent_id, "the FP PR should be detached from the original"
|
|
|
|
# NOTE: should the followup PR wait for pr1 CI or not?
|
|
|
|
assert pr2_id.head != pr2_head
|
|
|
|
assert pr2_id.parent_id == pr1_id, "the followup PR should still be linked"
|
|
|
|
|
|
|
|
assert prod.read_tree(prod.commit(pr1_id.head)) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'b',
|
|
|
|
'x': '5'
|
|
|
|
}, "the FP PR should have the new code"
|
|
|
|
assert prod.read_tree(prod.commit(pr2_id.head)) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'a',
|
|
|
|
'h': 'a',
|
|
|
|
'x': '5'
|
|
|
|
}, "the followup FP should also have the update"
|
[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
|
|
|
|
2023-08-29 20:59:05 +07:00
|
|
|
with prod:
|
|
|
|
prod.post_status(pr2_id.head, 'success', 'ci/runbot')
|
|
|
|
prod.post_status(pr2_id.head, 'success', 'legal/cla')
|
|
|
|
env.run_crons()
|
2024-03-12 20:57:50 +07:00
|
|
|
|
|
|
|
pr2 = prod.get_pr(pr2_id.number)
|
|
|
|
if merge_parent:
|
|
|
|
with prod:
|
|
|
|
pr2.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
with prod:
|
|
|
|
prod.post_status('staging.c', 'success', 'ci/runbot')
|
|
|
|
prod.post_status('staging.c', 'success', 'legal/cla')
|
|
|
|
env.run_crons()
|
|
|
|
assert pr2_id.state == 'merged'
|
|
|
|
|
2023-08-29 20:59:05 +07:00
|
|
|
_0, _1, _2, pr3_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
assert pr3_id.parent_id == pr2_id
|
|
|
|
# don't bother updating heads (?)
|
|
|
|
pr3_id.write({'parent_id': False, 'detach_reason': "testing"})
|
|
|
|
# pump feedback messages
|
|
|
|
env.run_crons()
|
[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
|
|
|
|
2023-08-29 20:59:05 +07:00
|
|
|
pr3 = prod.get_pr(pr3_id.number)
|
|
|
|
assert pr3.comments == [
|
|
|
|
seen(env, pr3, users),
|
|
|
|
(users['user'], f"""\
|
|
|
|
@{users['user']} @{users['reviewer']} this PR targets d 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+
|
2023-08-29 20:59:05 +07:00
|
|
|
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
|
|
"""),
|
|
|
|
(users['user'], f"@{users['user']} @{users['reviewer']} this PR was "
|
|
|
|
f"modified / updated and has become a normal PR. It "
|
[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
|
|
|
f"must be merged directly."
|
2023-08-29 20:59:05 +07:00
|
|
|
)
|
|
|
|
]
|
2024-03-12 20:57:50 +07:00
|
|
|
|
|
|
|
assert pr2.comments[:2] == [
|
2023-08-29 20:59:05 +07:00
|
|
|
seen(env, pr2, users),
|
|
|
|
(users['user'], """\
|
|
|
|
This PR targets c and is part of the forward-port chain. Further PRs will be created up to d.
|
|
|
|
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
|
|
"""),
|
|
|
|
]
|
2024-03-12 20:57:50 +07:00
|
|
|
|
|
|
|
if merge_parent:
|
|
|
|
assert pr2.comments[2:] == [
|
|
|
|
(users['reviewer'], "hansen r+"),
|
|
|
|
]
|
|
|
|
else:
|
|
|
|
assert pr2.comments[2:] == [
|
|
|
|
(users['user'], f"@{users['user']} @{users['reviewer']} child PR "
|
|
|
|
f"{pr3_id.display_name} was modified / updated and has "
|
|
|
|
f"become a normal PR. This PR (and any of its parents) "
|
|
|
|
f"will need to be merged independently as approvals "
|
|
|
|
f"won't cross."),
|
|
|
|
]
|
|
|
|
|
2021-02-24 16:06:27 +07:00
|
|
|
def test_update_merged(env, make_repo, config, users):
|
|
|
|
""" Strange things happen when an FP gets closed / merged but then its
|
|
|
|
parent is modified and the forwardport tries to update the (now merged)
|
|
|
|
child.
|
|
|
|
|
|
|
|
Turns out the issue is the followup: given a PR a and forward port targets
|
|
|
|
B -> C -> D. When a is merged we get b, c and d. If c gets merged *then*
|
|
|
|
b gets updated, the fwbot will update c in turn, then it will look for the
|
|
|
|
head of the updated c in order to create d.
|
|
|
|
|
|
|
|
However it *will not* find that head, as update events don't get propagated
|
|
|
|
on closed PRs (this is generally a good thing). As a result, the sanity
|
|
|
|
check when trying to port c to d will fail.
|
|
|
|
|
|
|
|
After checking with nim, the safest behaviour seems to be:
|
|
|
|
|
|
|
|
* stop at the update of the first closed or merged PR
|
|
|
|
* signal on that PR that something fucky happened
|
|
|
|
* also maybe disable or exponentially backoff the update job after some
|
|
|
|
number of attempts?
|
|
|
|
"""
|
2025-02-06 20:33:40 +07:00
|
|
|
prod, _ = make_basic(env, config, make_repo, statuses='default')
|
2021-02-24 16:06:27 +07:00
|
|
|
# add a 4th branch
|
|
|
|
with prod:
|
|
|
|
prod.make_ref('heads/d', prod.commit('c').id)
|
|
|
|
env['runbot_merge.project'].search([]).write({
|
2023-06-08 13:19:43 +07:00
|
|
|
'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})]
|
2021-02-24 16:06:27 +07:00
|
|
|
})
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
[c] = prod.make_commits('a', Commit('p_0', tree={'0': '0'}), ref='heads/hugechange')
|
|
|
|
pr = prod.make_pr(target='a', head='hugechange')
|
2025-02-06 20:33:40 +07:00
|
|
|
prod.post_status(c, 'success')
|
2021-02-24 16:06:27 +07:00
|
|
|
pr.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
with prod:
|
2025-02-06 20:33:40 +07:00
|
|
|
prod.post_status('staging.a', 'success')
|
2021-02-24 16:06:27 +07:00
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
_, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
with prod:
|
2025-02-06 20:33:40 +07:00
|
|
|
prod.post_status(pr1_id.head, 'success')
|
2021-02-24 16:06:27 +07:00
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
pr2 = prod.get_pr(pr2_id.number)
|
|
|
|
with prod:
|
|
|
|
pr2.post_comment('hansen r+', config['role_reviewer']['token'])
|
2025-02-06 20:33:40 +07:00
|
|
|
prod.post_status(pr2_id.head, 'success')
|
2021-02-24 16:06:27 +07:00
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
assert pr2_id.staging_id
|
|
|
|
with prod:
|
2025-02-06 20:33:40 +07:00
|
|
|
prod.post_status('staging.c', 'success')
|
2021-02-24 16:06:27 +07:00
|
|
|
env.run_crons()
|
|
|
|
assert pr2_id.state == 'merged'
|
|
|
|
assert pr2.state == 'closed'
|
|
|
|
|
|
|
|
# now we can try updating pr1 and see what happens
|
|
|
|
repo, ref = prod.get_pr(pr1_id.number).branch
|
|
|
|
with repo:
|
|
|
|
repo.make_commits(
|
2025-02-06 18:35:55 +07:00
|
|
|
prod.commit(pr1_id.target.name).id,
|
2021-02-24 16:06:27 +07:00
|
|
|
Commit('2', tree={'0': '0', '1': '1'}),
|
|
|
|
ref='heads/%s' % ref,
|
|
|
|
make=False
|
|
|
|
)
|
|
|
|
updates = env['forwardport.updates'].search([])
|
|
|
|
assert updates
|
|
|
|
assert updates.original_root == pr0_id
|
|
|
|
assert updates.new_root == pr1_id
|
|
|
|
env.run_crons()
|
|
|
|
assert not pr1_id.parent_id
|
|
|
|
assert not env['forwardport.updates'].search([])
|
|
|
|
|
|
|
|
assert pr2.comments == [
|
|
|
|
seen(env, pr2, users),
|
|
|
|
(users['user'], '''This PR targets c and is part of the forward-port chain. Further PRs will be created up to d.
|
|
|
|
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
|
|
'''),
|
|
|
|
(users['reviewer'], 'hansen r+'),
|
2022-06-23 19:25:07 +07:00
|
|
|
(users['user'], """@%s @%s ancestor PR %s has been updated but this PR is merged and can't be updated to match.
|
2021-02-24 16:06:27 +07:00
|
|
|
|
2022-06-23 19:25:07 +07:00
|
|
|
You may want or need to manually update any followup PR.""" % (
|
|
|
|
users['user'],
|
|
|
|
users['reviewer'],
|
|
|
|
pr1_id.display_name,
|
|
|
|
))
|
2021-02-24 16:06:27 +07:00
|
|
|
]
|
2021-03-01 16:35:09 +07:00
|
|
|
|
|
|
|
def test_duplicate_fw(env, make_repo, setreviewers, config, users):
|
|
|
|
""" Test for #451
|
|
|
|
"""
|
|
|
|
# 0 - 1 - 2 - 3 - 4 master
|
|
|
|
# \ - 31 v3
|
|
|
|
# \ - 21 v2
|
|
|
|
# \ - 11 v1
|
|
|
|
repo = make_repo('proj')
|
|
|
|
with repo:
|
|
|
|
_, c1, c2, c3, _ = repo.make_commits(
|
|
|
|
None,
|
|
|
|
Commit('0', tree={'f': 'a'}),
|
|
|
|
Commit('1', tree={'f': 'b'}),
|
|
|
|
Commit('2', tree={'f': 'c'}),
|
|
|
|
Commit('3', tree={'f': 'd'}),
|
|
|
|
Commit('4', tree={'f': 'e'}),
|
|
|
|
ref='heads/master'
|
|
|
|
)
|
|
|
|
repo.make_commits(c1, Commit('11', tree={'g': 'a'}), ref='heads/v1')
|
|
|
|
repo.make_commits(c2, Commit('21', tree={'h': 'a'}), ref='heads/v2')
|
|
|
|
repo.make_commits(c3, Commit('31', tree={'i': 'a'}), ref='heads/v3')
|
|
|
|
|
|
|
|
proj = env['runbot_merge.project'].create({
|
|
|
|
'name': 'a project',
|
|
|
|
'github_token': config['github']['token'],
|
|
|
|
'github_prefix': 'hansen',
|
2024-11-29 15:04:06 +07:00
|
|
|
'github_name': config['github']['name'],
|
|
|
|
'github_email': "foo@example.org",
|
2021-03-01 16:35:09 +07:00
|
|
|
'fp_github_token': config['github']['token'],
|
2023-06-12 19:41:42 +07:00
|
|
|
'fp_github_name': 'herbert',
|
2021-03-01 16:35:09 +07:00
|
|
|
'branch_ids': [
|
2023-06-08 13:19:43 +07:00
|
|
|
(0, 0, {'name': 'master', 'sequence': 0}),
|
|
|
|
(0, 0, {'name': 'v3', 'sequence': 1}),
|
|
|
|
(0, 0, {'name': 'v2', 'sequence': 2}),
|
|
|
|
(0, 0, {'name': 'v1', 'sequence': 3}),
|
2021-03-01 16:35:09 +07:00
|
|
|
],
|
|
|
|
'repo_ids': [
|
|
|
|
(0, 0, {
|
|
|
|
'name': repo.name,
|
|
|
|
'required_statuses': 'ci',
|
|
|
|
'fp_remote_target': repo.name,
|
|
|
|
})
|
|
|
|
]
|
|
|
|
})
|
|
|
|
setreviewers(*proj.repo_ids)
|
[ADD] *: per-repository webhook secret
Currently webhook secrets are configured per *project* which is an
issue both because different repositories may have different
administrators and thus creates safety concerns, and because multiple
repositories can feed into different projects (e.g. on mergebot,
odoo-dev/odoo is both an ancillary repository to the main RD project,
and the main repository to the minor / legacy master-wowl
project). This means it can be necessary to have multiple projects
share the same secret as well, this then mandates the secret for more
repositories per (1).
This is a pain in the ass, so just detach secrets from projects and
link them *only* to repositories, it's cleaner and easier to manage
and set up progressively.
This requires a lot of changes to the tests, as they all need to
correctly configure the signaling.
For `runbot_merge` there was *some* setup sharing already via the
module-level `repo` fixtures`, those were merged into a conftest-level
fixture which could handle the signaling setup. A few tests which
unnecessarily set up repositories ad-hoc were also moved to the
fixture. But for most of the ad-hoc setup in `runbot_merge`, as well
as `forwardport` where it's all ad-hoc, events sources setup was just
appended as is. This should probably be cleaned up at one point, with
the various requirements collected and organised into a small set of
fixtures doing the job more uniformly.
Fixes #887
2024-06-06 16:07:57 +07:00
|
|
|
env['runbot_merge.events_sources'].create({'repository': repo.name})
|
2021-03-01 16:35:09 +07:00
|
|
|
|
|
|
|
# create a PR in v1, merge it, then create all 3 ports
|
|
|
|
with repo:
|
|
|
|
repo.make_commits('v1', Commit('c0', tree={'z': 'a'}), ref='heads/hugechange')
|
|
|
|
prv1 = repo.make_pr(target='v1', head='hugechange')
|
|
|
|
repo.post_status('hugechange', 'success', 'ci')
|
|
|
|
prv1.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
PRs = env['runbot_merge.pull_requests']
|
|
|
|
prv1_id = PRs.search([
|
|
|
|
('repository.name', '=', repo.name),
|
|
|
|
('number', '=', prv1.number),
|
|
|
|
])
|
|
|
|
assert prv1_id.state == 'ready'
|
|
|
|
with repo:
|
|
|
|
repo.post_status('staging.v1', 'success', 'ci')
|
|
|
|
env.run_crons()
|
|
|
|
assert prv1_id.state == 'merged'
|
|
|
|
|
|
|
|
parent = prv1_id
|
|
|
|
while True:
|
|
|
|
child = PRs.search([('parent_id', '=', parent.id)])
|
|
|
|
if not child:
|
|
|
|
break
|
|
|
|
|
|
|
|
assert child.state == 'opened'
|
|
|
|
with repo:
|
|
|
|
repo.post_status(child.head, 'success', 'ci')
|
|
|
|
env.run_crons()
|
|
|
|
parent = child
|
|
|
|
pr_ids = _, prv2_id, prv3_id, prmaster_id = PRs.search([], order='number')
|
2025-02-06 20:33:40 +07:00
|
|
|
_, prv2, _prv3, _prmaster = [repo.get_pr(p.number) for p in pr_ids]
|
2021-03-01 16:35:09 +07:00
|
|
|
assert pr_ids.mapped('target.name') == ['v1', 'v2', 'v3', 'master']
|
|
|
|
assert pr_ids.mapped('state') == ['merged', 'validated', 'validated', 'validated']
|
|
|
|
assert repo.read_tree(repo.commit(prmaster_id.head)) == {'f': 'e', 'z': 'a'}
|
|
|
|
|
|
|
|
with repo:
|
|
|
|
repo.make_commits('v2', Commit('c0', tree={'z': 'b'}), ref=prv2.ref, make=False)
|
|
|
|
env.run_crons()
|
[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 pr_ids.mapped('state') == ['merged', 'opened', 'opened', 'opened']
|
2021-03-01 16:35:09 +07:00
|
|
|
assert repo.read_tree(repo.commit(prv2_id.head)) == {'f': 'c', 'h': 'a', 'z': 'b'}
|
|
|
|
assert repo.read_tree(repo.commit(prv3_id.head)) == {'f': 'd', 'i': 'a', 'z': 'b'}
|
|
|
|
assert repo.read_tree(repo.commit(prmaster_id.head)) == {'f': 'e', 'z': 'b'}
|
|
|
|
|
|
|
|
assert prv2_id.source_id == prv1_id
|
|
|
|
assert not prv2_id.parent_id
|
|
|
|
|
|
|
|
env.run_crons()
|
|
|
|
assert PRs.search([], order='number') == pr_ids
|
|
|
|
|
|
|
|
with repo:
|
|
|
|
repo.post_status(prv2.head, 'success', 'ci')
|
|
|
|
prv2.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
with repo:
|
|
|
|
repo.post_status('staging.v2', 'success', 'ci')
|
|
|
|
env.run_crons()
|
|
|
|
# env.run_crons()
|
|
|
|
assert PRs.search([], order='number') == pr_ids
|
2021-07-27 14:17:18 +07:00
|
|
|
|
|
|
|
def test_subsequent_conflict(env, make_repo, config, users):
|
|
|
|
""" Test for updating an fw PR in the case where it produces a conflict in
|
|
|
|
the followup. Cf #467.
|
|
|
|
"""
|
2025-02-06 20:33:40 +07:00
|
|
|
repo, fork = make_basic(env, config, make_repo, statuses='default')
|
2021-07-27 14:17:18 +07:00
|
|
|
|
|
|
|
# create a PR in branch A which adds a new file
|
|
|
|
with repo:
|
|
|
|
repo.make_commits('a', Commit('newfile', tree={'x': '0'}), ref='heads/pr1')
|
|
|
|
pr_1 = repo.make_pr(target='a', head='pr1')
|
2025-02-06 20:33:40 +07:00
|
|
|
repo.post_status('pr1', 'success')
|
2021-07-27 14:17:18 +07:00
|
|
|
pr_1.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
with repo:
|
2025-02-06 20:33:40 +07:00
|
|
|
repo.post_status('staging.a', 'success')
|
2021-07-27 14:17:18 +07:00
|
|
|
env.run_crons()
|
|
|
|
pr1_id = to_pr(env, pr_1)
|
|
|
|
assert pr1_id.state == 'merged'
|
|
|
|
|
|
|
|
pr2_id = env['runbot_merge.pull_requests'].search([('source_id', '=', pr1_id.id)])
|
|
|
|
assert pr2_id
|
|
|
|
with repo:
|
2025-02-06 20:33:40 +07:00
|
|
|
repo.post_status(pr2_id.head, 'success')
|
2021-07-27 14:17:18 +07:00
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
pr3_id = env['runbot_merge.pull_requests'].search([('parent_id', '=', pr2_id.id)])
|
|
|
|
assert pr3_id
|
|
|
|
assert repo.read_tree(repo.commit(pr3_id.head)) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'a',
|
|
|
|
'h': 'a',
|
|
|
|
'x': '0',
|
|
|
|
}
|
|
|
|
|
|
|
|
# update pr2: add a file "h"
|
|
|
|
pr2 = repo.get_pr(pr2_id.number)
|
|
|
|
t = {**repo.read_tree(repo.commit(pr2_id.head)), 'h': 'conflict!'}
|
|
|
|
with fork:
|
2025-02-06 18:35:55 +07:00
|
|
|
fork.make_commits(repo.commit(pr2_id.target.name).id, Commit('newfiles', tree=t), ref=pr2.ref, make=False)
|
2021-07-27 14:17:18 +07:00
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
assert repo.read_tree(repo.commit(pr3_id.head)) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'a',
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
'h': matches('''<<<\x3c<<< $$
|
2021-07-27 14:17:18 +07:00
|
|
|
a
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
||||||| $$
|
2021-07-27 14:17:18 +07:00
|
|
|
=======
|
|
|
|
conflict!
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
>>>\x3e>>> $$
|
2021-07-27 14:17:18 +07:00
|
|
|
'''),
|
|
|
|
'x': '0',
|
|
|
|
}
|
|
|
|
# skip comments:
|
|
|
|
# 1. link to mergebot status page
|
|
|
|
# 2. "forward port chain" bit
|
|
|
|
# 3. updated / modified & got detached
|
|
|
|
assert pr2.comments[3:] == [
|
2022-11-03 17:46:31 +07:00
|
|
|
(users['user'], f"@{users['user']} @{users['reviewer']} WARNING: the latest change ({pr2_id.head}) triggered "
|
2021-07-27 14:17:18 +07:00
|
|
|
f"a conflict when updating the next forward-port "
|
|
|
|
f"({pr3_id.display_name}), and has been ignored.\n\n"
|
|
|
|
f"You will need to update this pull request "
|
|
|
|
f"differently, or fix the issue by hand on "
|
|
|
|
f"{pr3_id.display_name}.")
|
|
|
|
]
|
|
|
|
# skip comments:
|
|
|
|
# 1. link to status page
|
|
|
|
# 2. forward-port chain thing
|
|
|
|
assert repo.get_pr(pr3_id.number).comments[2:] == [
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
(users['user'], f'''\
|
2022-11-03 17:46:31 +07:00
|
|
|
@{users['user']} @{users['reviewer']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \
|
2021-07-27 14:17:18 +07:00
|
|
|
conflict in this pull request, data may have been lost.
|
|
|
|
|
|
|
|
stdout:
|
2024-06-04 17:15:29 +07:00
|
|
|
```
|
|
|
|
Auto-merging h
|
|
|
|
CONFLICT (add/add): Merge conflict in h
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
```'''),
|
2021-07-27 14:17:18 +07:00
|
|
|
]
|