NextERP/runbot
5
0
Fork 0
mirror of https://github.com/odoo/runbot.git synced 2025-03-16 16:05:42 +07:00
Code Issues Actions Packages Projects Releases Wiki Activity
18.0-fix-xdo
runbot/forwardport/models/forwardport.py

437 lines
17 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 -*-
[IMP] *: crons tests running for better triggered compatibility Mergebot / forwardport crons need to run in a specific ordering in order to flow into one another correctly. The default ordering being unspecified, it was not possible to use the normal cron runner (instead of the external driver running crons in sequence one at a time). This can be fixed by setting *sequences* on crons, as the cron runner (`_process_jobs`) will use that order to acquire and run crons. Also override `_process_jobs` however: the built-in cron runner fetches a static list of ready crons, then runs that. This is fine for normal situation where the cron runner runs in a loop anyway but it's any issue for the tests, as we expect that cron A can trigger cron B, and we want cron B to run *right now* even if it hadn't been triggered before cron A ran. We can replace `_process_job` with a cut down version which does that (cut down because we don't need most of the error handling / resilience, there's no concurrent workers, there's no module being installed, versions must match, ...). This allows e.g. the cron propagating commit statuses to trigger the staging cron, and both will run within the same `run_crons` session. Something I didn't touch is that `_process_jobs` internally creates completely new environments so there is no way to pass context into the cron jobs anymore (whereas it works for `method_direct_trigger`), this means the context values have to be shunted elsewhere for that purpose which is gross. But even though I'm replacing `_process_jobs`, this seems a bit too much of a change in cron execution semantics. So left it out. While at it tho, silence the spammy `py.warnings` stuff I can't do much about.
2024-07-30 14:21:05 +07:00
import builtins
[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 logging
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +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
from contextlib import ExitStack
[IMP] forwardport: handling of missing magic refs Github can fail to create the magic refs on PRs (`pull/refs/?/head`). Since forwardport relies on these refs to fetch PR content this is an issue when it occurs, as the forward ports fail in a loop. After discussion with Github support, it turns out Github enabled `allowReachableSHA1InWant` a while back, meaning it's possible to fetch content by commit (rather than ref) as long as the content is "in network". Use this property as fallback when checking if we can see the PR head before forward porting. Also: - remove explicit configuration of GC during fetch, it doesn't disable the autogc (yet?) but that's likely going to happen anyway - update logging and logger hierarchy during forward port to make things clearer and easier to extract, although based on PR id rather than number - rate limit failing forward ports to avoid running them on every cron (~ every minute), run them every ~30mn instead, this provides higher odds of recovery with less log garbage in case of transient github failure, and if the PR is stuck it limits the log pollution Fixes #658
2022-10-28 13:03:12 +07:00
from datetime import datetime, timedelta
[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
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
import requests
[CHG] *: move repo cache from forwardbot to mergebot If the stagings are going to be created locally (via a git working copy rather than the github API), the mergebot part needs to have access to the cache, so move the cache over. Also move the maintenance cron. In an extermely minor way, this prefigures the (hopeful) eventual merging of the ~~planes~~ modules.
2023-08-16 19:37:19 +07:00
import sentry_sdk
[IMP] forwardport: batch list Since b45ecf08f955478cc0e0e76177c1223c8dfc0898 forwardport batches which fail have a delay set in order to avoid spamming. However that delay was not displayed anywhere, which made things confusing as the batch would not get run even after creating new triggers. Show the delay if it's set (to a value later than now), as a relative delta for clarity (as normally the delay is in minutes so a full blown date is difficult to read / aprehend), and allow viewing and setting it in the form view. Fixes #982
2024-11-18 20:18:25 +07:00
from babel.dates import format_timedelta
[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 dateutil import relativedelta
[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: cron create prototype Apparently when I added these to trigger the corresponding cron the lack of decorator caused them to not work at all, at least when invoked from the interface, consequently I notably wasn't able to force a forward port via creating one such task when trying to work around #953
2024-09-16 17:46:44 +07:00
from odoo import api, fields, models
[CHG] *: move repo cache from forwardbot to mergebot If the stagings are going to be created locally (via a git working copy rather than the github API), the mergebot part needs to have access to the cache, so move the cache over. Also move the maintenance cron. In an extermely minor way, this prefigures the (hopeful) eventual merging of the ~~planes~~ modules.
2023-08-16 19:37:19 +07:00
from odoo.addons.runbot_merge import git
[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 odoo.addons.runbot_merge.github import GH
[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
[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
# how long a merged PR survives
MERGE_AGE = relativedelta.relativedelta(weeks=2)
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
FOOTER = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\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
_logger = logging.getLogger(__name__)
class Queue:
[MERGE] from 13.0 Get mergebot updates from since the runbot was upgraded. NOTE: updates forwardport.models.forwardport.Queue with slots for compatibility with commit odoo/odoo@ea3e39506a2b360545ec0cd1d3235e69ad16ea6c "use slots for BaseModel", otherwise we get TypeError: __bases__ assignment: 'ForwardPortTasks' object layout differs from 'BaseModel'
2022-08-23 19:33:37 +07:00
__slots__ = ()
[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
limit = 100
[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 _process_item(self):
raise NotImplementedError
def _process(self):
[IMP] forwardport: allow updating the fw limit after merging the source Currently, once a source PR has been merged it's not possible to set or update a limit, which can be inconvenient (e.g. people might have forgotten to set it, or realise after the fact that setting one is not useful, or might realise after the fact that they should *unset* it). This PR relaxes that constraint (which is not just a relaxation as it requires a bunch of additional work and validation), it should now be possible to: - update the limit to any target, as long as that target is at or above the current forwardport tip's - with the exception of a tip being forward ported, as that can't be cancelled - resume a forward port stopped by a previously set limit (just increase it to whatever) - set a limit from any forward-port PR - set a limit from a source, post-merge The process should also provide pretty chatty feedback: - feedback on the source and closest root - feedback about adjustments if any (e.g. setting the limit to B but there's already a forward port to C, the 'bot should set the limit to C and tell you that it happened and why) Fixes #506
2023-10-06 18:19:01 +07:00
skip = 0
from_clause, where_clause, params = self._search(self._search_domain(), order='create_date, id', limit=1).get_sql()
for _ in range(self.limit):
self.env.cr.execute(f"""
SELECT id FROM {from_clause}
WHERE {where_clause or "true"}
ORDER BY create_date, id
LIMIT 1 OFFSET %s
FOR UPDATE SKIP LOCKED
""", [*params, skip])
b = self.browse(self.env.cr.fetchone())
if not b:
return
[IMP] forwardport: processing queue reliability The queue would get items to process one at a time, process, commit, and go to the next. However this is an issue if one of the item fails systematically for some reason (aka it's not just a transient failure): the cron fails, then restarts at the exact same point, and fails again with the same issue, leading to following items never getting processed. Fix by getting all the queue contents at once, processing them one by one and "skipping" any item which fails (leaving it in place so it can get re-processed later). That way, even if an item causes issues, the rest of the queue gets processed normally. The interruption was an issue following odoo/enterprise#5670 not getting properly updated in the backend (backend didn't get notified of the last two updates / force-push to the PR, so it was trying to forward-port a commit which didn't exist - and failing).
2019-10-10 13:41:33 +07:00
try:
[ADD] runbot_merge: sentry instrumentation Currently sentry is only hooked from the outside, which doesn't necessarily provide sufficiently actionable information. Add some a few hooks to (try and) report odoo / mergebot metadata: - add the user to WSGI transactions - add a transaction (with users) around crons - add the webhook event info to webhook requests - add a few spans to the long-running crons, when they cover multiple units per iteration (e.g. a span per branch being staged) Closes #544
2023-06-15 13:22:38 +07:00
with sentry_sdk.start_span(description=self._name):
b._process_item()
[IMP] forwardport: processing queue reliability The queue would get items to process one at a time, process, commit, and go to the next. However this is an issue if one of the item fails systematically for some reason (aka it's not just a transient failure): the cron fails, then restarts at the exact same point, and fails again with the same issue, leading to following items never getting processed. Fix by getting all the queue contents at once, processing them one by one and "skipping" any item which fails (leaving it in place so it can get re-processed later). That way, even if an item causes issues, the rest of the queue gets processed normally. The interruption was an issue following odoo/enterprise#5670 not getting properly updated in the backend (backend didn't get notified of the last two updates / force-push to the PR, so it was trying to forward-port a commit which didn't exist - and failing).
2019-10-10 13:41:33 +07:00
b.unlink()
self.env.cr.commit()
except Exception:
_logger.exception("Error while processing %s, skipping", b)
[FIX] forwardport: queue reliability changes Previous version would break if _process_item itself committed which was bad
2019-10-11 14:02:50 +07:00
self.env.cr.rollback()
[IMP] forwardport: allow updating the fw limit after merging the source Currently, once a source PR has been merged it's not possible to set or update a limit, which can be inconvenient (e.g. people might have forgotten to set it, or realise after the fact that setting one is not useful, or might realise after the fact that they should *unset* it). This PR relaxes that constraint (which is not just a relaxation as it requires a bunch of additional work and validation), it should now be possible to: - update the limit to any target, as long as that target is at or above the current forwardport tip's - with the exception of a tip being forward ported, as that can't be cancelled - resume a forward port stopped by a previously set limit (just increase it to whatever) - set a limit from any forward-port PR - set a limit from a source, post-merge The process should also provide pretty chatty feedback: - feedback on the source and closest root - feedback about adjustments if any (e.g. setting the limit to B but there's already a forward port to C, the 'bot should set the limit to C and tell you that it happened and why) Fixes #506
2023-10-06 18:19:01 +07:00
if b._on_failure():
skip += 1
[IMP] forwardport: handling of missing magic refs Github can fail to create the magic refs on PRs (`pull/refs/?/head`). Since forwardport relies on these refs to fetch PR content this is an issue when it occurs, as the forward ports fail in a loop. After discussion with Github support, it turns out Github enabled `allowReachableSHA1InWant` a while back, meaning it's possible to fetch content by commit (rather than ref) as long as the content is "in network". Use this property as fallback when checking if we can see the PR head before forward porting. Also: - remove explicit configuration of GC during fetch, it doesn't disable the autogc (yet?) but that's likely going to happen anyway - update logging and logger hierarchy during forward port to make things clearer and easier to extract, although based on PR id rather than number - rate limit failing forward ports to avoid running them on every cron (~ every minute), run them every ~30mn instead, this provides higher odds of recovery with less log garbage in case of transient github failure, and if the PR is stuck it limits the log pollution Fixes #658
2022-10-28 13:03:12 +07:00
self.env.cr.commit()
def _on_failure(self):
[IMP] forwardport: allow updating the fw limit after merging the source Currently, once a source PR has been merged it's not possible to set or update a limit, which can be inconvenient (e.g. people might have forgotten to set it, or realise after the fact that setting one is not useful, or might realise after the fact that they should *unset* it). This PR relaxes that constraint (which is not just a relaxation as it requires a bunch of additional work and validation), it should now be possible to: - update the limit to any target, as long as that target is at or above the current forwardport tip's - with the exception of a tip being forward ported, as that can't be cancelled - resume a forward port stopped by a previously set limit (just increase it to whatever) - set a limit from any forward-port PR - set a limit from a source, post-merge The process should also provide pretty chatty feedback: - feedback on the source and closest root - feedback about adjustments if any (e.g. setting the limit to B but there's already a forward port to C, the 'bot should set the limit to C and tell you that it happened and why) Fixes #506
2023-10-06 18:19:01 +07:00
return 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
[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
def _search_domain(self):
return []
[FIX] forwardport: don't forwardport freeze PRs The freeze wizard has support for merging freeze / release PRs on each of the newly created branches. But since this would be done by, well, merging, those PRs would get forward-ported to master, and would have to be closed there. This creates additional work for the freeze master, and noise / parasitic PRs. Obviously it's possible for the freeze master to set some nonsense `up to` (nonsense because the "real" limit doesn't exist yet at that point), but really it never makes any sense to forward port release PRs, so the wizard should do it.
2022-02-08 16:11:57 +07:00
class ForwardPortTasks(models.Model, Queue):
[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 = 'forwardport.batches'
_description = 'batches which got merged and are candidates for forward-porting'
[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
limit = 10
[IMP] forwardport: allow updating the fw limit after merging the source Currently, once a source PR has been merged it's not possible to set or update a limit, which can be inconvenient (e.g. people might have forgotten to set it, or realise after the fact that setting one is not useful, or might realise after the fact that they should *unset* it). This PR relaxes that constraint (which is not just a relaxation as it requires a bunch of additional work and validation), it should now be possible to: - update the limit to any target, as long as that target is at or above the current forwardport tip's - with the exception of a tip being forward ported, as that can't be cancelled - resume a forward port stopped by a previously set limit (just increase it to whatever) - set a limit from any forward-port PR - set a limit from a source, post-merge The process should also provide pretty chatty feedback: - feedback on the source and closest root - feedback about adjustments if any (e.g. setting the limit to B but there's already a forward port to C, the 'bot should set the limit to C and tell you that it happened and why) Fixes #506
2023-10-06 18:19:01 +07:00
batch_id = fields.Many2one('runbot_merge.batch', required=True, index=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
source = fields.Selection([
('merge', 'Merge'),
('fp', 'Forward Port Followup'),
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
('insert', 'New branch port'),
('complete', 'Complete ported batches'),
[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
], required=True)
[IMP] forwardport: handling of missing magic refs Github can fail to create the magic refs on PRs (`pull/refs/?/head`). Since forwardport relies on these refs to fetch PR content this is an issue when it occurs, as the forward ports fail in a loop. After discussion with Github support, it turns out Github enabled `allowReachableSHA1InWant` a while back, meaning it's possible to fetch content by commit (rather than ref) as long as the content is "in network". Use this property as fallback when checking if we can see the PR head before forward porting. Also: - remove explicit configuration of GC during fetch, it doesn't disable the autogc (yet?) but that's likely going to happen anyway - update logging and logger hierarchy during forward port to make things clearer and easier to extract, although based on PR id rather than number - rate limit failing forward ports to avoid running them on every cron (~ every minute), run them every ~30mn instead, this provides higher odds of recovery with less log garbage in case of transient github failure, and if the PR is stuck it limits the log pollution Fixes #658
2022-10-28 13:03:12 +07:00
retry_after = fields.Datetime(required=True, default='1900-01-01 01:01:01')
[IMP] forwardport: batch list Since b45ecf08f955478cc0e0e76177c1223c8dfc0898 forwardport batches which fail have a delay set in order to avoid spamming. However that delay was not displayed anywhere, which made things confusing as the batch would not get run even after creating new triggers. Show the delay if it's set (to a value later than now), as a relative delta for clarity (as normally the delay is in minutes so a full blown date is difficult to read / aprehend), and allow viewing and setting it in the form view. Fixes #982
2024-11-18 20:18:25 +07:00
retry_after_relative = fields.Char(compute="_compute_retry_after_relative")
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
pr_id = fields.Many2one('runbot_merge.pull_requests')
[IMP] forwardport: handling of missing magic refs Github can fail to create the magic refs on PRs (`pull/refs/?/head`). Since forwardport relies on these refs to fetch PR content this is an issue when it occurs, as the forward ports fail in a loop. After discussion with Github support, it turns out Github enabled `allowReachableSHA1InWant` a while back, meaning it's possible to fetch content by commit (rather than ref) as long as the content is "in network". Use this property as fallback when checking if we can see the PR head before forward porting. Also: - remove explicit configuration of GC during fetch, it doesn't disable the autogc (yet?) but that's likely going to happen anyway - update logging and logger hierarchy during forward port to make things clearer and easier to extract, although based on PR id rather than number - rate limit failing forward ports to avoid running them on every cron (~ every minute), run them every ~30mn instead, this provides higher odds of recovery with less log garbage in case of transient github failure, and if the PR is stuck it limits the log pollution Fixes #658
2022-10-28 13:03:12 +07:00
[FIX] forwardport: cron create prototype Apparently when I added these to trigger the corresponding cron the lack of decorator caused them to not work at all, at least when invoked from the interface, consequently I notably wasn't able to force a forward port via creating one such task when trying to work around #953
2024-09-16 17:46:44 +07:00
@api.model_create_multi
[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
def create(self, vals_list):
self.env.ref('forwardport.port_forward')._trigger()
return super().create(vals_list)
def write(self, vals):
if retry := vals.get('retry_after'):
self.env.ref('forwardport.port_forward')\
._trigger(fields.Datetime.to_datetime(retry))
return super().write(vals)
[IMP] forwardport: handling of missing magic refs Github can fail to create the magic refs on PRs (`pull/refs/?/head`). Since forwardport relies on these refs to fetch PR content this is an issue when it occurs, as the forward ports fail in a loop. After discussion with Github support, it turns out Github enabled `allowReachableSHA1InWant` a while back, meaning it's possible to fetch content by commit (rather than ref) as long as the content is "in network". Use this property as fallback when checking if we can see the PR head before forward porting. Also: - remove explicit configuration of GC during fetch, it doesn't disable the autogc (yet?) but that's likely going to happen anyway - update logging and logger hierarchy during forward port to make things clearer and easier to extract, although based on PR id rather than number - rate limit failing forward ports to avoid running them on every cron (~ every minute), run them every ~30mn instead, this provides higher odds of recovery with less log garbage in case of transient github failure, and if the PR is stuck it limits the log pollution Fixes #658
2022-10-28 13:03:12 +07:00
def _search_domain(self):
return super()._search_domain() + [
('retry_after', '<=', fields.Datetime.to_string(fields.Datetime.now())),
]
[IMP] forwardport: batch list Since b45ecf08f955478cc0e0e76177c1223c8dfc0898 forwardport batches which fail have a delay set in order to avoid spamming. However that delay was not displayed anywhere, which made things confusing as the batch would not get run even after creating new triggers. Show the delay if it's set (to a value later than now), as a relative delta for clarity (as normally the delay is in minutes so a full blown date is difficult to read / aprehend), and allow viewing and setting it in the form view. Fixes #982
2024-11-18 20:18:25 +07:00
@api.depends('retry_after')
def _compute_retry_after_relative(self):
now = fields.Datetime.now()
for t in self:
if t.retry_after <= now:
t.retry_after_relative = ""
else:
t.retry_after_relative = format_timedelta(t.retry_after - now, locale=t.env.lang)
[IMP] forwardport: handling of missing magic refs Github can fail to create the magic refs on PRs (`pull/refs/?/head`). Since forwardport relies on these refs to fetch PR content this is an issue when it occurs, as the forward ports fail in a loop. After discussion with Github support, it turns out Github enabled `allowReachableSHA1InWant` a while back, meaning it's possible to fetch content by commit (rather than ref) as long as the content is "in network". Use this property as fallback when checking if we can see the PR head before forward porting. Also: - remove explicit configuration of GC during fetch, it doesn't disable the autogc (yet?) but that's likely going to happen anyway - update logging and logger hierarchy during forward port to make things clearer and easier to extract, although based on PR id rather than number - rate limit failing forward ports to avoid running them on every cron (~ every minute), run them every ~30mn instead, this provides higher odds of recovery with less log garbage in case of transient github failure, and if the PR is stuck it limits the log pollution Fixes #658
2022-10-28 13:03:12 +07:00
def _on_failure(self):
super()._on_failure()
self.retry_after = fields.Datetime.to_string(fields.Datetime.now() + timedelta(minutes=30))
[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 _process_item(self):
batch = self.batch_id
[ADD] runbot_merge: sentry instrumentation Currently sentry is only hooked from the outside, which doesn't necessarily provide sufficiently actionable information. Add some a few hooks to (try and) report odoo / mergebot metadata: - add the user to WSGI transactions - add a transaction (with users) around crons - add the webhook event info to webhook requests - add a few spans to the long-running crons, when they cover multiple units per iteration (e.g. a span per branch being staged) Closes #544
2023-06-15 13:22:38 +07:00
sentry_sdk.set_tag('forward-porting', batch.prs.mapped('display_name'))
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
if self.source == 'complete':
self._complete_batches()
return
[ADD] forwardport: special handling of adding branches to projects If a new branch is added to a project, there's an issue with *ongoing* forward ports (forward ports which were not merged before the branch was forked from an existing one): the new branch gets "skipped" and might be missing some fixes until those are noticed and backported. This commit hooks into updating projects to try and see if the update consists of adding a branch inside the sequence, in which case it tries to find the FP sequences to update and queues up new "intermediate" forward ports to insert into the existing sequences. Note: had to increase the cron socket limit to 2mn as 1mn blew up the big staging cron in the test (after all the forward-port PRs are approved). Fixes #262 [FIX]
2020-01-27 21:39:25 +07:00
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
newbatch = batch._port_forward()
[CHG] *: persistent batches This probably has latent bugs, and is only the start of the road to v2 (#789): PR batches are now created up-front (alongside the PR), with PRs attached and detached as needed, hopefully such that things are not broken (tests pass but...), this required a fair number of ajustments to code not taking batches into account, or creating batches on the fly. `PullRequests.blocked` has also been updated to rely on the batch to get its batch-mates, such that it can now be a stored field with the right dependencies. The next step is to better leverage this change: - move cross-PR state up to the batch (e.g. skipchecks, priority, ...) - add fw info to the batch, perform forward-ports batchwise in order to avoid redundant batch-selection work, and allow altering batches during fw (e.g. adding or removing PRs) - use batches to select stagings - maybe expose staging history of a batch?
2023-12-19 17:10:11 +07:00
if not newbatch: # reached end of seq (or batch is empty)
[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
# FIXME: or configuration is fucky so doesn't want to FP (maybe should error and retry?)
_logger.info(
[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
"Processed %s from %s (%s) -> end of the sequence",
batch, self.source, batch.prs.mapped('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
)
[CHG] *: persistent batches This probably has latent bugs, and is only the start of the road to v2 (#789): PR batches are now created up-front (alongside the PR), with PRs attached and detached as needed, hopefully such that things are not broken (tests pass but...), this required a fair number of ajustments to code not taking batches into account, or creating batches on the fly. `PullRequests.blocked` has also been updated to rely on the batch to get its batch-mates, such that it can now be a stored field with the right dependencies. The next step is to better leverage this change: - move cross-PR state up to the batch (e.g. skipchecks, priority, ...) - add fw info to the batch, perform forward-ports batchwise in order to avoid redundant batch-selection work, and allow altering batches during fw (e.g. adding or removing PRs) - use batches to select stagings - maybe expose staging history of a batch?
2023-12-19 17:10:11 +07:00
return
_logger.info(
[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
"Processed %s from %s (%s) -> %s (%s)",
batch, self.source, ', '.join(batch.prs.mapped('display_name')),
newbatch, ', '.join(newbatch.prs.mapped('display_name')),
[CHG] *: persistent batches This probably has latent bugs, and is only the start of the road to v2 (#789): PR batches are now created up-front (alongside the PR), with PRs attached and detached as needed, hopefully such that things are not broken (tests pass but...), this required a fair number of ajustments to code not taking batches into account, or creating batches on the fly. `PullRequests.blocked` has also been updated to rely on the batch to get its batch-mates, such that it can now be a stored field with the right dependencies. The next step is to better leverage this change: - move cross-PR state up to the batch (e.g. skipchecks, priority, ...) - add fw info to the batch, perform forward-ports batchwise in order to avoid redundant batch-selection work, and allow altering batches during fw (e.g. adding or removing PRs) - use batches to select stagings - maybe expose staging history of a batch?
2023-12-19 17:10:11 +07:00
)
[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
# insert new batch in ancestry sequence
[CHG] *: persistent batches This probably has latent bugs, and is only the start of the road to v2 (#789): PR batches are now created up-front (alongside the PR), with PRs attached and detached as needed, hopefully such that things are not broken (tests pass but...), this required a fair number of ajustments to code not taking batches into account, or creating batches on the fly. `PullRequests.blocked` has also been updated to rely on the batch to get its batch-mates, such that it can now be a stored field with the right dependencies. The next step is to better leverage this change: - move cross-PR state up to the batch (e.g. skipchecks, priority, ...) - add fw info to the batch, perform forward-ports batchwise in order to avoid redundant batch-selection work, and allow altering batches during fw (e.g. adding or removing PRs) - use batches to select stagings - maybe expose staging history of a batch?
2023-12-19 17:10:11 +07:00
if self.source == 'insert':
[IMP] forwardport: automatically approve freeze backfills if the corresponding master PR was approved. That the backfill needs approval can be considered a race condition: its dual is approved and *might* have been merged before the freeze, but was not, so a backfill was needed. Copying over the approval is a convenience feature with pretty much no actual risk I can think of. Fixes #884
2024-06-07 20:02:28 +07:00
self._process_insert(batch, newbatch)
def _process_insert(self, batch, newbatch):
self.env['runbot_merge.batch'].search([
('parent_id', '=', batch.id),
('id', '!=', newbatch.id),
]).parent_id = newbatch.id
# insert new PRs in ancestry sequence unless conflict (= no parent)
for pr in newbatch.prs:
next_target = pr._find_next_target()
if not next_target:
continue
# should have one since it was inserted before an other PR?
descendant = pr.search([
('target', '=', next_target.id),
('source_id', '=', pr.source_id.id),
])
# copy the reviewing of the "descendant" (even if detached) to this pr
if reviewer := descendant.reviewed_by:
pr.reviewed_by = reviewer
# replace parent_id *if not detached*
if descendant.parent_id:
descendant.parent_id = pr.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
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
def _complete_batches(self):
source = pr = self.pr_id
if not pr:
_logger.warning(
"Unable to complete descendants of %s (%s): no new PR",
self.batch_id,
self.batch_id.prs.mapped('display_name'),
)
return
_logger.info(
"Completing batches for descendants of %s (added %s)",
self.batch_id.prs.mapped('display_name'),
self.pr_id.display_name,
)
gh = requests.Session()
repository = pr.repository
gh.headers['Authorization'] = f'token {repository.project_id.fp_github_token}'
PullRequests = self.env['runbot_merge.pull_requests']
self.env.cr.execute('LOCK runbot_merge_pull_requests IN SHARE MODE')
# TODO: extract complete list of targets from `_find_next_target`
# so we can create all the forwardport branches, push them, and
# only then create the PR objects
# TODO: maybe do that after making forward-port WC-less, so all the
# branches can be pushed atomically at once
[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
for descendant in self.batch_id.descendants():
target = pr._find_next_target()
if target is None:
_logger.info("Will not forward-port %s: no next target", pr.display_name)
return
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
[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
if PullRequests.search_count([
('source_id', '=', source.id),
('target', '=', target.id),
('state', 'not in', ('closed', 'merged')),
]):
_logger.warning("Will not forward-port %s: already ported", pr.display_name)
return
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
[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
if target != descendant.target:
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': repository.id,
'pull_request': source.id,
'token_field': 'fp_github_token',
'message': """\
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
{pr.ping}unable to port this PR forwards due to inconsistency: goes from \
{pr.target.name} to {next_target.name} but {batch} ({batch_prs}) targets \
{batch.target.name}.
""".format(pr=pr, next_target=target, batch=descendant, batch_prs=', '.join(descendant.mapped('prs.display_name')))
})
[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
return
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
[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
ref = descendant.prs[:1].refname
# NOTE: ports the new source everywhere instead of porting each
# PR to the next step as it does not *stop* on conflict
repo = git.get_local(source.repository)
[ADD] *: PR backport wizard This is not a full user-driven backport thingie for now, just one admins can use to facilitate thing and debug issues with the system. May eventually graduate to a frontend feature. Fixes #925
2024-10-22 15:53:45 +07:00
conflict, head = source._create_port_branch(repo, target, forward=True)
[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
repo.push(git.fw_url(pr.repository), f'{head}:refs/heads/{ref}')
remote_target = repository.fp_remote_target
owner, _ = remote_target.split('/', 1)
message = source.message + f"\n\nForward-Port-Of: {pr.display_name}"
title, body = re.match(r'(?P<title>[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups()
r = gh.post(f'https://api.github.com/repos/{pr.repository.name}/pulls', json={
'base': target.name,
'head': f'{owner}:{ref}',
'title': '[FW]' + (' ' if title[0] != '[' else '') + title,
'body': body
})
if not r.ok:
_logger.warning("Failed to create forward-port PR for %s, deleting branches", pr.display_name)
# delete all the branches this should automatically close the
# PRs if we've created any. Using the API here is probably
# simpler than going through the working copies
d = gh.delete(f'https://api.github.com/repos/{remote_target}/git/refs/heads/{ref}')
if d.ok:
_logger.info("Deleting %s:%s=success", remote_target, ref)
else:
_logger.warning("Deleting %s:%s=%s", remote_target, ref, d.text)
raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})")
new_pr = PullRequests._from_gh(r.json())
_logger.info("Created forward-port PR %s", new_pr)
new_pr.write({
'batch_id': descendant.id, # should already be set correctly but...
'merge_method': pr.merge_method,
'source_id': source.id,
# only link to previous PR of sequence if cherrypick passed
# FIXME: apply parenting of siblings? Apply parenting *to* siblings?
'parent_id': pr.id if not conflict else False,
'detach_reason': "{1}\n{2}".format(*conflict).strip() if conflict else None,
})
if conflict:
self.env.ref('runbot_merge.forwardport.failure.conflict')._send(
repository=pr.repository,
pull_request=pr.number,
token_field='fp_github_token',
format_args={'source': source, 'pr': pr, 'new': new_pr, 'footer': FOOTER},
)
new_pr._fp_conflict_feedback(pr, {pr: conflict})
labels = ['forwardport']
if conflict:
labels.append('conflict')
self.env['runbot_merge.pull_requests.tagging'].create({
'repository': new_pr.repository.id,
'pull_request': new_pr.number,
'tags_add': labels,
})
[IMP] *: handle the addition of a new PR to a fw-ported batch Given a batch which has been merged, and been forward-ported, to multiple branches (because skipci was set or ci passed on the repos the batch covers). There might come the need to add a PR for one of the uncovered repos. This raises the question of what to do with it, since the forward-ports for the batch already exist it's not going to get forwardported normally, nor may we want to, possibly? Options are: - don't do anything, such additions don't get ported, this is incongruous and unexpected as by default PRs are forward-ported, and if the batch wasn't an intermediate (but e.g. a conflict) it probably would be ported forward - port on merge, this allows configuring the PR properly (as it might need its own limit) but it means further batches may get unexpectedly merged (or at least retied) without the additional PR even though we likely want it in - immediately port the additional PR on creation, this makes the limit harder or impossible to configure but it makes the *batch sequence* more consistent We ended up selecting the latter, it feels closer to the updates system, and it creates more consistent batches through the sequence. It's also technically easier to ad-hoc port a PR through a bunch of branches than it is to update the "normal" forward-port process to handle partial fixups.
2024-03-29 15:16:51 +07:00
[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
pr = new_pr
[IMP] forwardport: handling of updates causing conflicts on followups If a PR is updated and has extent forward-ports, those forwardports get updated automatically ("followup"). However there is an issue if the udpate causes a conflict in the followup: the conflict gets silently pushed, and may fairly easily get merged if it occurs in an area which the CI doesn't cover. It's unclear what the policy really should be for this issue, and there is no real way to *block* a pull request at the moment (save by putting it in error at the mergebot level I guess?), so for now clearly notify the user on both the modified PR and the followup, with a comment on both. We may want to revisit this eventually. Fixes #467
2021-07-27 14:17:18 +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
class UpdateQueue(models.Model, Queue):
_name = 'forwardport.updates'
_description = 'if a forward-port PR gets updated & has followups (cherrypick succeeded) the followups need to be updated as well'
[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
limit = 10
[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
original_root = fields.Many2one('runbot_merge.pull_requests')
new_root = fields.Many2one('runbot_merge.pull_requests')
[FIX] forwardport: cron create prototype Apparently when I added these to trigger the corresponding cron the lack of decorator caused them to not work at all, at least when invoked from the interface, consequently I notably wasn't able to force a forward port via creating one such task when trying to work around #953
2024-09-16 17:46:44 +07:00
@api.model_create_multi
[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
def create(self, vals_list):
self.env.ref('forwardport.updates')._trigger()
return super().create(vals_list)
[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 _process_item(self):
previous = self.new_root
[ADD] runbot_merge: sentry instrumentation Currently sentry is only hooked from the outside, which doesn't necessarily provide sufficiently actionable information. Add some a few hooks to (try and) report odoo / mergebot metadata: - add the user to WSGI transactions - add a transaction (with users) around crons - add the webhook event info to webhook requests - add a few spans to the long-running crons, when they cover multiple units per iteration (e.g. a span per branch being staged) Closes #544
2023-06-15 13:22:38 +07:00
sentry_sdk.set_tag("update-root", self.new_root.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 ExitStack() as s:
for child in self.new_root._iter_descendants():
[FIX] forwardport: possible race in forwardport followup update Normally when a forwardport is updated the forward-bot cascades the update onto its followups (if it has any), but takes care to keep the followups attached as they were not updated "by hand". In the case of odoo/odoo#77677 however that did not work and the followup PRs got detached. Looking at the logs, it becomes flagrant that there was a race condition: either Git took a long time to respond to the push, or there was an IO slowdown which led to the "local update" taking a very long time. Either way this allowed the "synchronize" webhook from github to arrive before the current transaction was committed, rolling back said transaction and making the forwardbot assume this was a "real" sync and detach the followup from its parent. Locking the PR row up-front ought fix the issue, and also move the local update to before having pushed: the "extra" commits in the local cache don't matter too much even if pushing to github fails, they'll be cleaned up by a GC eventually. Also migrate the `-f` on push to `--force-if-includes` in order to avoid possible race conditions on the push (since we're not fetching the current branch, use the full-syntax explicit CAS form, that's exactly what we're looking for anyway). Fixes #541 (hopefully)
2021-10-19 15:30:55 +07:00
self.env.cr.execute("""
SELECT id
FROM runbot_merge_pull_requests
WHERE id = %s
FOR UPDATE NOWAIT
""", [child.id])
[FIX] forwardport: PR update through a closed PR Fixes #328
2020-02-19 18:12:02 +07:00
_logger.info(
"Re-port %s from %s (changed root %s -> %s)",
child.display_name,
previous.display_name,
self.original_root.display_name,
self.new_root.display_name
)
if child.state in ('closed', 'merged'):
[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
self.env.ref('runbot_merge.forwardport.updates.closed')._send(
repository=child.repository,
pull_request=child.number,
token_field='fp_github_token',
format_args={'pr': child, 'parent': self.new_root},
)
[FIX] forwardport: PR update through a closed PR Fixes #328
2020-02-19 18:12:02 +07:00
return
[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
[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
repo = git.get_local(previous.repository)
[ADD] *: PR backport wizard This is not a full user-driven backport thingie for now, just one admins can use to facilitate thing and debug issues with the system. May eventually graduate to a frontend feature. Fixes #925
2024-10-22 15:53:45 +07:00
conflicts, new_head = previous._create_port_branch(repo, child.target, forward=True)
[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
[IMP] forwardport: handling of updates causing conflicts on followups If a PR is updated and has extent forward-ports, those forwardports get updated automatically ("followup"). However there is an issue if the udpate causes a conflict in the followup: the conflict gets silently pushed, and may fairly easily get merged if it occurs in an area which the CI doesn't cover. It's unclear what the policy really should be for this issue, and there is no real way to *block* a pull request at the moment (save by putting it in error at the mergebot level I guess?), so for now clearly notify the user on both the modified PR and the followup, with a comment on both. We may want to revisit this eventually. Fixes #467
2021-07-27 14:17:18 +07:00
if conflicts:
[IMP] forwardport: provide clearer picture of conflicts On conflicts in multi-commit PRs developers sometimes get confused as to what happened why. If a conflict occurs and the source pull request had multiple commits, list all the source commits and show which one broke. Related to #505
2021-08-11 19:08:18 +07:00
_, out, err, _ = conflicts
[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
self.env.ref('runbot_merge.forwardport.updates.conflict.parent')._send(
repository=previous.repository,
pull_request=previous.number,
token_field='fp_github_token',
format_args={'pr': previous, 'next': child},
)
self.env.ref('runbot_merge.forwardport.updates.conflict.child')._send(
repository=child.repository,
pull_request=child.number,
token_field='fp_github_token',
format_args={
'previous': previous,
'pr': child,
'stdout': (f'\n\nstdout:\n```\n{out.strip()}\n```' if out.strip() else ''),
'stderr': (f'\n\nstderr:\n```\n{err.strip()}\n```' if err.strip() else ''),
},
)
[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] 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
commits_count = int(repo.stdout().rev_list(
f'{child.target.name}..{new_head}',
[FIX] forwardport: completely update PR data when forwarding updates The process did properly update the state, but not the squash state. It's somewhat unclear whether the state should be fully reset and require reapproval though. Maybe only the validation should be reset? The CI will eventually run and either succeed (re-validating) or fail (devalidating, hopefully) but I'm not entirely sure this is correct.
2021-02-26 15:45:44 +07:00
count=True
).stdout.decode().strip())
[FIX] forwardport: possible race in forwardport followup update Normally when a forwardport is updated the forward-bot cascades the update onto its followups (if it has any), but takes care to keep the followups attached as they were not updated "by hand". In the case of odoo/odoo#77677 however that did not work and the followup PRs got detached. Looking at the logs, it becomes flagrant that there was a race condition: either Git took a long time to respond to the push, or there was an IO slowdown which led to the "local update" taking a very long time. Either way this allowed the "synchronize" webhook from github to arrive before the current transaction was committed, rolling back said transaction and making the forwardbot assume this was a "real" sync and detach the followup from its parent. Locking the PR row up-front ought fix the issue, and also move the local update to before having pushed: the "extra" commits in the local cache don't matter too much even if pushing to github fails, they'll be cleaned up by a GC eventually. Also migrate the `-f` on push to `--force-if-includes` in order to avoid possible race conditions on the push (since we're not fetching the current branch, use the full-syntax explicit CAS form, that's exactly what we're looking for anyway). Fixes #541 (hopefully)
2021-10-19 15:30:55 +07:00
old_head = child.head
[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
# update child's head to the head we're going to push
[FIX] forwardport: completely update PR data when forwarding updates The process did properly update the state, but not the squash state. It's somewhat unclear whether the state should be fully reset and require reapproval though. Maybe only the validation should be reset? The CI will eventually run and either succeed (re-validating) or fail (devalidating, hopefully) but I'm not entirely sure this is correct.
2021-02-26 15:45:44 +07:00
child.with_context(ignore_head_update=True).write({
'head': new_head,
# 'state': 'opened',
'squash': commits_count == 1,
})
[FIX] forwardport: possible race in forwardport followup update Normally when a forwardport is updated the forward-bot cascades the update onto its followups (if it has any), but takes care to keep the followups attached as they were not updated "by hand". In the case of odoo/odoo#77677 however that did not work and the followup PRs got detached. Looking at the logs, it becomes flagrant that there was a race condition: either Git took a long time to respond to the push, or there was an IO slowdown which led to the "local update" taking a very long time. Either way this allowed the "synchronize" webhook from github to arrive before the current transaction was committed, rolling back said transaction and making the forwardbot assume this was a "real" sync and detach the followup from its parent. Locking the PR row up-front ought fix the issue, and also move the local update to before having pushed: the "extra" commits in the local cache don't matter too much even if pushing to github fails, they'll be cleaned up by a GC eventually. Also migrate the `-f` on push to `--force-if-includes` in order to avoid possible race conditions on the push (since we're not fetching the current branch, use the full-syntax explicit CAS form, that's exactly what we're looking for anyway). Fixes #541 (hopefully)
2021-10-19 15:30:55 +07:00
# then update the child's branch to the new head
[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
repo.push(
f'--force-with-lease={child.refname}:{old_head}',
git.fw_url(child.repository),
f"{new_head}:refs/heads/{child.refname}")
[IMP] forwardport: mitigate cat-file not finding commit on updates Fix #457 hopefully: I didn't manage to repro / create a test for. It looks like in some cases during the update process the PR ref lags behind the branch itself. This means `forwardport.updates` creates a new commit, pushes it, then on the next iteration updates the local cache, tries to find the commit we just pushed... and that fails. I can only assume this is because when there's enough load on the github side the update to the `info/refs` pseudo-file can fall behind (it's now 4MB and holding nearly 65k refs). So cheat: take the commit we just pushed to the dev remote and... immediately push it to the local cache under a dummy branch, which we delete. Since we only gc "1 day ago" this should not vacuum.
2021-03-01 18:38:23 +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
# committing here means github could technically trigger its
# webhook before sending a response, but committing before
# would mean we can update the PR in database but fail to
# update on github, which is probably worse?
# alternatively we can commit, push, and rollback if the push
# fails
# FIXME: handle failures (especially on non-first update)
self.env.cr.commit()
previous = child
[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
_deleter = _logger.getChild('deleter')
class DeleteBranches(models.Model, Queue):
_name = 'forwardport.branch_remover'
_description = "Removes branches of merged PRs"
pr_id = fields.Many2one('runbot_merge.pull_requests')
[FIX] forwardport: cron create prototype Apparently when I added these to trigger the corresponding cron the lack of decorator caused them to not work at all, at least when invoked from the interface, consequently I notably wasn't able to force a forward port via creating one such task when trying to work around #953
2024-09-16 17:46:44 +07:00
@api.model_create_multi
[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
def create(self, vals_list):
self.env.ref('forwardport.remover')._trigger(datetime.now() - MERGE_AGE)
return super().create(vals_list)
[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
def _search_domain(self):
[IMP] *: crons tests running for better triggered compatibility Mergebot / forwardport crons need to run in a specific ordering in order to flow into one another correctly. The default ordering being unspecified, it was not possible to use the normal cron runner (instead of the external driver running crons in sequence one at a time). This can be fixed by setting *sequences* on crons, as the cron runner (`_process_jobs`) will use that order to acquire and run crons. Also override `_process_jobs` however: the built-in cron runner fetches a static list of ready crons, then runs that. This is fine for normal situation where the cron runner runs in a loop anyway but it's any issue for the tests, as we expect that cron A can trigger cron B, and we want cron B to run *right now* even if it hadn't been triggered before cron A ran. We can replace `_process_job` with a cut down version which does that (cut down because we don't need most of the error handling / resilience, there's no concurrent workers, there's no module being installed, versions must match, ...). This allows e.g. the cron propagating commit statuses to trigger the staging cron, and both will run within the same `run_crons` session. Something I didn't touch is that `_process_jobs` internally creates completely new environments so there is no way to pass context into the cron jobs anymore (whereas it works for `method_direct_trigger`), this means the context values have to be shunted elsewhere for that purpose which is gross. But even though I'm replacing `_process_jobs`, this seems a bit too much of a change in cron execution semantics. So left it out. While at it tho, silence the spammy `py.warnings` stuff I can't do much about.
2024-07-30 14:21:05 +07:00
cutoff = getattr(builtins, 'forwardport_merged_before', None) \
[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
or fields.Datetime.to_string(datetime.now() - MERGE_AGE)
[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
return [('pr_id.merge_date', '<', cutoff)]
[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
def _process_item(self):
_deleter.info(
"PR %s: checking deletion of linked branch %s",
self.pr_id.display_name,
self.pr_id.label
)
if self.pr_id.state != 'merged':
_deleter.info('✘ PR is not "merged" (got %s)', self.pr_id.state)
return
repository = self.pr_id.repository
fp_remote = repository.fp_remote_target
if not fp_remote:
_deleter.info('✘ no forward-port target')
return
repo_owner, repo_name = fp_remote.split('/')
owner, branch = self.pr_id.label.split(':')
if repo_owner != owner:
_deleter.info('✘ PR owner != FP target owner (%s)', repo_owner)
return # probably don't have access to arbitrary repos
[FIX] forwardport: the fix it's a per-call parameter not a per-instance one
2019-10-18 13:11:27 +07:00
github = GH(token=repository.project_id.fp_github_token, repo=fp_remote)
[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
refurl = 'git/refs/heads/' + branch
[FIX] forwardport: the fix it's a per-call parameter not a per-instance one
2019-10-18 13:11:27 +07:00
ref = github('get', refurl, check=False)
[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
if ref.status_code != 200:
_deleter.info("✘ branch already deleted (%s)", ref.json())
return
ref = ref.json()
[FIX] forwardport: apparently git/refs/heads can fuzzy-match If the ref we asked for does not exist, github apparently decides to fall-back to prefix-matching. So if we're trying to delete already-deleted branch A and someone called their branch A-x we're going to get it as a result. Thankfully they were apparently smart enough to return a list even if there's only a single fuzzy match. So if we get a list (instead of a dict) as response to git/refs/heads assume the branch was already deleted as if we got a 404.
2019-10-18 16:21:52 +07:00
if isinstance(ref, list):
_deleter.info(
"✘ got a fuzzy match (%s), branch probably deleted",
', '.join(r['ref'] for r in ref)
)
[FIX] forwardport: the fix Of course I forgot the most relevant bit
2019-10-18 17:01:47 +07:00
return
[FIX] forwardport: apparently git/refs/heads can fuzzy-match If the ref we asked for does not exist, github apparently decides to fall-back to prefix-matching. So if we're trying to delete already-deleted branch A and someone called their branch A-x we're going to get it as a result. Thankfully they were apparently smart enough to return a list even if there's only a single fuzzy match. So if we get a list (instead of a dict) as response to git/refs/heads assume the branch was already deleted as if we got a 404.
2019-10-18 16:21:52 +07:00
[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
if ref['object']['sha'] != self.pr_id.head:
_deleter.info(
"✘ branch %s head mismatch, expected %s, got %s",
self.pr_id.label,
self.pr_id.head,
ref['object']['sha']
)
return
[FIX] forwardport: the fix it's a per-call parameter not a per-instance one
2019-10-18 13:11:27 +07:00
r = github('delete', refurl, check=False)
[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 r.status_code == 204, \
"Tried to delete branch %s of %s, got %s" % (
branch, self.pr_id.display_name,
r.json()
)
_deleter.info('✔ deleted branch %s of PR %s', self.pr_id.label, self.pr_id.display_name)
Reference in New Issue Copy Permalink