2022-11-17 16:30:04 +07:00
|
|
|
import collections.abc
|
2018-06-21 14:55:14 +07:00
|
|
|
import itertools
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
import json
|
2018-03-14 16:37:46 +07:00
|
|
|
import logging
|
2019-11-18 19:57:32 +07:00
|
|
|
import logging.handlers
|
|
|
|
import os
|
|
|
|
import pathlib
|
|
|
|
import pprint
|
[IMP] runbot_merge: ensure at least 1s between mutating GH calls
Mostly a temporary safety feature after the events of 07-31: it's
still not clear whether that was a one-off issue or a change in
policy (I was not able to reproduce locally even doing several
set_refs a second) and the gh support is not super talkative, but it
probably doesn't hurt to commit the workaround until #247 gets
implemented.
On 2023-07-31, around 08:30 UTC, `set_ref` started failing, a lot
(although oddly enough not continuously), with the unhelpful message
that
> 422: Reference cannot be updated
This basically broke all stagings, until a workaround was implemented
by adding a 1s sleep before `set_ref` to ensure no more than 1
`set_ref` per second, which kinda sorta has been the github
recommendation forever but had never been an issue
before. Contributing to this suspicion is that in late 2022, the
documentation of error 422 on `PATCH git/refs/{ref}` was updated to:
> Validation failed, or the endpoint has been spammed.
Still would be nice if GH was clear about it and sent a 429 instead.
Technically the recommendation is:
> If you're making a large number of POST, PATCH, PUT, or DELETE
> requests for a single user or client ID, wait at least one second
> between each request.
So... actually implement that. On a per-worker basis as for the most
part these are serial processes (e.g. crons), we can still get above
the rate limit due to concurrent crons but it should be less likely.
Also take `Retry-After` in account, can't hurt, though we're supposed
to retry just the request rather than abort the entire thing. Maybe a
future update can improve this handling.
Would also be nice to take `X-RateLimit` in account, although that's
supposed to apply to *all* requests so we'd need a second separate
timestamp to track it. Technically that's probably also the case for
`Retry-After`. And fixing #247 should cut down drastically on the API
calls traffic as staging is a very API-intensive process, especially
with the sanity checks we had to add, these days we might be at 4
calls per commit per PR, and up to 80 PRs/staging (5 repositories and
16 batches per staging), with 13 live branches (though realistically
only 6-7 have significant traffic, and only 1~2 get close to filling
their staging slots).
2023-08-11 16:50:38 +07:00
|
|
|
import time
|
2019-11-18 19:57:32 +07:00
|
|
|
import unicodedata
|
2018-03-14 16:37:46 +07:00
|
|
|
|
|
|
|
import requests
|
2019-11-18 19:57:32 +07:00
|
|
|
import werkzeug.urls
|
2018-03-14 16:37:46 +07:00
|
|
|
|
2019-11-18 19:57:32 +07:00
|
|
|
import odoo.netsvc
|
|
|
|
from odoo.tools import topological_sort, config
|
2019-02-28 20:45:31 +07:00
|
|
|
from . import exceptions, utils
|
2018-03-14 16:37:46 +07:00
|
|
|
|
2021-08-03 18:45:21 +07:00
|
|
|
class MergeError(Exception): ...
|
2019-11-18 19:57:32 +07:00
|
|
|
|
[FIX] runbot_merge: handle the bot user not being able to comment
If the author of a PR has blocked the bot user, commenting on the PR
will fail. While comment failure is technically handled in the feedback
cron, the cron will simply retry commenting on every run filling the
log with useless unactionable garbage.
Retrying is the right thing to do in the normal case (e.g. changing tags
often has transient failures), but if we figure out we're blocked we
might as well just log a warning and drop the comment on the floor, it's
unlikely the situation will resolve itself.
Couldn't test it, because the block API is a developer preview and I
just can't get it to work anyway (404 on /user/blocks even providing the
suggested media type).
And the way the block is inferred is iffy (based on an error message),
the error body doesn't seem to provide any clean / clear cut error code:
{
"message": "Validation Failed",
"errors": [
{
"resource": "IssueComment",
"code": "unprocessable",
"field": "data",
"message": "User is blocked"
}
],
"documentation_url": "https://developer.github.com/v3/issues/comments/#create-a-comment"
}
No useful headers either.
Fixes #127
2019-05-07 15:36:53 +07:00
|
|
|
def _is_json(r):
|
|
|
|
return r and r.headers.get('content-type', '').startswith(('application/json', 'application/javascript'))
|
|
|
|
|
2018-03-14 16:37:46 +07:00
|
|
|
_logger = logging.getLogger(__name__)
|
2019-11-18 19:57:32 +07:00
|
|
|
_gh = logging.getLogger('github_requests')
|
|
|
|
def _init_gh_logger():
|
|
|
|
""" Log all GH requests / responses so we have full tracking, but put them
|
|
|
|
in a separate file if we're logging to a file
|
|
|
|
"""
|
|
|
|
if not config['logfile']:
|
|
|
|
return
|
|
|
|
original = pathlib.Path(config['logfile'])
|
|
|
|
new = original.with_name('github_requests')\
|
|
|
|
.with_suffix(original.suffix)
|
|
|
|
|
|
|
|
if os.name == 'posix':
|
|
|
|
handler = logging.handlers.WatchedFileHandler(str(new))
|
|
|
|
else:
|
|
|
|
handler = logging.FileHandler(str(new))
|
|
|
|
|
|
|
|
handler.setFormatter(odoo.netsvc.DBFormatter(
|
|
|
|
'%(asctime)s %(pid)s %(levelname)s %(dbname)s %(name)s: %(message)s'
|
|
|
|
))
|
|
|
|
_gh.addHandler(handler)
|
|
|
|
_gh.propagate = False
|
|
|
|
|
|
|
|
if odoo.netsvc._logger_init:
|
|
|
|
_init_gh_logger()
|
|
|
|
|
2023-08-10 21:14:33 +07:00
|
|
|
GH_LOG_PATTERN = """=> {method} {path}{qs}{body}
|
2019-11-18 19:57:32 +07:00
|
|
|
|
|
|
|
<= {r.status_code} {r.reason}
|
|
|
|
{headers}
|
|
|
|
{body2}
|
|
|
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
|
|
"""
|
2018-03-14 16:37:46 +07:00
|
|
|
class GH(object):
|
|
|
|
def __init__(self, token, repo):
|
|
|
|
self._url = 'https://api.github.com'
|
|
|
|
self._repo = repo
|
[IMP] runbot_merge: ensure at least 1s between mutating GH calls
Mostly a temporary safety feature after the events of 07-31: it's
still not clear whether that was a one-off issue or a change in
policy (I was not able to reproduce locally even doing several
set_refs a second) and the gh support is not super talkative, but it
probably doesn't hurt to commit the workaround until #247 gets
implemented.
On 2023-07-31, around 08:30 UTC, `set_ref` started failing, a lot
(although oddly enough not continuously), with the unhelpful message
that
> 422: Reference cannot be updated
This basically broke all stagings, until a workaround was implemented
by adding a 1s sleep before `set_ref` to ensure no more than 1
`set_ref` per second, which kinda sorta has been the github
recommendation forever but had never been an issue
before. Contributing to this suspicion is that in late 2022, the
documentation of error 422 on `PATCH git/refs/{ref}` was updated to:
> Validation failed, or the endpoint has been spammed.
Still would be nice if GH was clear about it and sent a 429 instead.
Technically the recommendation is:
> If you're making a large number of POST, PATCH, PUT, or DELETE
> requests for a single user or client ID, wait at least one second
> between each request.
So... actually implement that. On a per-worker basis as for the most
part these are serial processes (e.g. crons), we can still get above
the rate limit due to concurrent crons but it should be less likely.
Also take `Retry-After` in account, can't hurt, though we're supposed
to retry just the request rather than abort the entire thing. Maybe a
future update can improve this handling.
Would also be nice to take `X-RateLimit` in account, although that's
supposed to apply to *all* requests so we'd need a second separate
timestamp to track it. Technically that's probably also the case for
`Retry-After`. And fixing #247 should cut down drastically on the API
calls traffic as staging is a very API-intensive process, especially
with the sanity checks we had to add, these days we might be at 4
calls per commit per PR, and up to 80 PRs/staging (5 repositories and
16 batches per staging), with 13 live branches (though realistically
only 6-7 have significant traffic, and only 1~2 get close to filling
their staging slots).
2023-08-11 16:50:38 +07:00
|
|
|
self._last_update = 0
|
2018-03-14 16:37:46 +07:00
|
|
|
session = self._session = requests.Session()
|
2018-03-26 18:08:49 +07:00
|
|
|
session.headers['Authorization'] = 'token {}'.format(token)
|
2019-08-21 16:17:04 +07:00
|
|
|
session.headers['Accept'] = 'application/vnd.github.symmetra-preview+json'
|
2018-03-14 16:37:46 +07:00
|
|
|
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
def _log_gh(self, logger: logging.Logger, response: requests.Response, level: int = logging.INFO, extra=None):
|
2019-11-18 19:57:32 +07:00
|
|
|
""" Logs a pair of request / response to github, to the specified
|
|
|
|
logger, at the specified level.
|
|
|
|
|
|
|
|
Tries to format all the information (including request / response
|
|
|
|
bodies, at least in part) so we have as much information as possible
|
|
|
|
for post-mortems.
|
|
|
|
"""
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
req = response.request
|
|
|
|
url = werkzeug.urls.url_parse(req.url)
|
2023-08-10 18:21:21 +07:00
|
|
|
if url.netloc != 'api.github.com':
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
return
|
2019-11-18 19:57:32 +07:00
|
|
|
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
body = '' if not req.body else ('\n' + pprint.pformat(json.loads(req.body.decode()), indent=4))
|
2019-11-18 19:57:32 +07:00
|
|
|
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
body2 = ''
|
2019-11-18 19:57:32 +07:00
|
|
|
if response.content:
|
|
|
|
if _is_json(response):
|
|
|
|
body2 = pprint.pformat(response.json(), depth=4)
|
|
|
|
elif response.encoding is not None:
|
|
|
|
body2 = response.text
|
|
|
|
else: # fallback: universal decoding & replace nonprintables
|
|
|
|
body2 = ''.join(
|
|
|
|
'\N{REPLACEMENT CHARACTER}' if unicodedata.category(c) == 'Cc' else c
|
|
|
|
for c in response.content.decode('iso-8859-1')
|
|
|
|
)
|
|
|
|
|
|
|
|
logger.log(level, GH_LOG_PATTERN.format(
|
|
|
|
# requests data
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
method=req.method, path=url.path, qs=url.query, body=body,
|
2019-11-18 19:57:32 +07:00
|
|
|
# response data
|
|
|
|
r=response,
|
|
|
|
headers='\n'.join(
|
|
|
|
'\t%s: %s' % (h, v) for h, v in response.headers.items()
|
|
|
|
),
|
|
|
|
body2=utils.shorten(body2.strip(), 400)
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
), extra=extra)
|
2019-11-18 19:57:32 +07:00
|
|
|
|
2018-08-28 20:42:28 +07:00
|
|
|
def __call__(self, method, path, params=None, json=None, check=True):
|
2018-03-14 16:37:46 +07:00
|
|
|
"""
|
|
|
|
:type check: bool | dict[int:Exception]
|
|
|
|
"""
|
[IMP] runbot_merge: ensure at least 1s between mutating GH calls
Mostly a temporary safety feature after the events of 07-31: it's
still not clear whether that was a one-off issue or a change in
policy (I was not able to reproduce locally even doing several
set_refs a second) and the gh support is not super talkative, but it
probably doesn't hurt to commit the workaround until #247 gets
implemented.
On 2023-07-31, around 08:30 UTC, `set_ref` started failing, a lot
(although oddly enough not continuously), with the unhelpful message
that
> 422: Reference cannot be updated
This basically broke all stagings, until a workaround was implemented
by adding a 1s sleep before `set_ref` to ensure no more than 1
`set_ref` per second, which kinda sorta has been the github
recommendation forever but had never been an issue
before. Contributing to this suspicion is that in late 2022, the
documentation of error 422 on `PATCH git/refs/{ref}` was updated to:
> Validation failed, or the endpoint has been spammed.
Still would be nice if GH was clear about it and sent a 429 instead.
Technically the recommendation is:
> If you're making a large number of POST, PATCH, PUT, or DELETE
> requests for a single user or client ID, wait at least one second
> between each request.
So... actually implement that. On a per-worker basis as for the most
part these are serial processes (e.g. crons), we can still get above
the rate limit due to concurrent crons but it should be less likely.
Also take `Retry-After` in account, can't hurt, though we're supposed
to retry just the request rather than abort the entire thing. Maybe a
future update can improve this handling.
Would also be nice to take `X-RateLimit` in account, although that's
supposed to apply to *all* requests so we'd need a second separate
timestamp to track it. Technically that's probably also the case for
`Retry-After`. And fixing #247 should cut down drastically on the API
calls traffic as staging is a very API-intensive process, especially
with the sanity checks we had to add, these days we might be at 4
calls per commit per PR, and up to 80 PRs/staging (5 repositories and
16 batches per staging), with 13 live branches (though realistically
only 6-7 have significant traffic, and only 1~2 get close to filling
their staging slots).
2023-08-11 16:50:38 +07:00
|
|
|
if method.casefold() != 'get':
|
|
|
|
to_sleep = 1. - (time.time() - self._last_update)
|
|
|
|
if to_sleep > 0:
|
|
|
|
time.sleep(to_sleep)
|
|
|
|
|
2023-02-21 21:11:59 +07:00
|
|
|
path = f'/repos/{self._repo}/{path}'
|
|
|
|
r = self._session.request(method, self._url + path, params=params, json=json)
|
[IMP] runbot_merge: ensure at least 1s between mutating GH calls
Mostly a temporary safety feature after the events of 07-31: it's
still not clear whether that was a one-off issue or a change in
policy (I was not able to reproduce locally even doing several
set_refs a second) and the gh support is not super talkative, but it
probably doesn't hurt to commit the workaround until #247 gets
implemented.
On 2023-07-31, around 08:30 UTC, `set_ref` started failing, a lot
(although oddly enough not continuously), with the unhelpful message
that
> 422: Reference cannot be updated
This basically broke all stagings, until a workaround was implemented
by adding a 1s sleep before `set_ref` to ensure no more than 1
`set_ref` per second, which kinda sorta has been the github
recommendation forever but had never been an issue
before. Contributing to this suspicion is that in late 2022, the
documentation of error 422 on `PATCH git/refs/{ref}` was updated to:
> Validation failed, or the endpoint has been spammed.
Still would be nice if GH was clear about it and sent a 429 instead.
Technically the recommendation is:
> If you're making a large number of POST, PATCH, PUT, or DELETE
> requests for a single user or client ID, wait at least one second
> between each request.
So... actually implement that. On a per-worker basis as for the most
part these are serial processes (e.g. crons), we can still get above
the rate limit due to concurrent crons but it should be less likely.
Also take `Retry-After` in account, can't hurt, though we're supposed
to retry just the request rather than abort the entire thing. Maybe a
future update can improve this handling.
Would also be nice to take `X-RateLimit` in account, although that's
supposed to apply to *all* requests so we'd need a second separate
timestamp to track it. Technically that's probably also the case for
`Retry-After`. And fixing #247 should cut down drastically on the API
calls traffic as staging is a very API-intensive process, especially
with the sanity checks we had to add, these days we might be at 4
calls per commit per PR, and up to 80 PRs/staging (5 repositories and
16 batches per staging), with 13 live branches (though realistically
only 6-7 have significant traffic, and only 1~2 get close to filling
their staging slots).
2023-08-11 16:50:38 +07:00
|
|
|
if method.casefold() != 'get':
|
|
|
|
self._last_update = time.time() + int(r.headers.get('Retry-After', 0))
|
|
|
|
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
self._log_gh(_gh, r)
|
2018-03-14 16:37:46 +07:00
|
|
|
if check:
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
try:
|
|
|
|
if isinstance(check, collections.abc.Mapping):
|
|
|
|
exc = check.get(r.status_code)
|
|
|
|
if exc:
|
|
|
|
raise exc(r.text)
|
|
|
|
if r.status_code >= 400:
|
|
|
|
raise requests.HTTPError(r.text, response=r)
|
|
|
|
except Exception:
|
|
|
|
self._log_gh(_logger, r, level=logging.ERROR, extra={
|
|
|
|
'github-request-id': r.headers.get('x-github-request-id'),
|
|
|
|
})
|
|
|
|
raise
|
|
|
|
|
2018-03-14 16:37:46 +07:00
|
|
|
return r
|
|
|
|
|
2018-11-22 00:43:05 +07:00
|
|
|
def user(self, username):
|
|
|
|
r = self._session.get("{}/users/{}".format(self._url, username))
|
|
|
|
r.raise_for_status()
|
|
|
|
return r.json()
|
|
|
|
|
2018-03-14 16:37:46 +07:00
|
|
|
def head(self, branch):
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
d = utils.backoff(
|
|
|
|
lambda: self('get', 'git/refs/heads/{}'.format(branch)).json(),
|
|
|
|
exc=requests.HTTPError
|
|
|
|
)
|
2018-03-14 16:37:46 +07:00
|
|
|
|
2018-03-26 18:08:49 +07:00
|
|
|
assert d['ref'] == 'refs/heads/{}'.format(branch)
|
2018-03-14 16:37:46 +07:00
|
|
|
assert d['object']['type'] == 'commit'
|
2018-09-20 14:25:13 +07:00
|
|
|
_logger.debug("head(%s, %s) -> %s", self._repo, branch, d['object']['sha'])
|
2018-03-14 16:37:46 +07:00
|
|
|
return d['object']['sha']
|
|
|
|
|
|
|
|
def commit(self, sha):
|
2018-09-20 14:25:13 +07:00
|
|
|
c = self('GET', 'git/commits/{}'.format(sha)).json()
|
|
|
|
_logger.debug('commit(%s, %s) -> %s', self._repo, sha, shorten(c['message']))
|
|
|
|
return c
|
2018-03-14 16:37:46 +07:00
|
|
|
|
|
|
|
def comment(self, pr, message):
|
[FIX] runbot_merge: handle the bot user not being able to comment
If the author of a PR has blocked the bot user, commenting on the PR
will fail. While comment failure is technically handled in the feedback
cron, the cron will simply retry commenting on every run filling the
log with useless unactionable garbage.
Retrying is the right thing to do in the normal case (e.g. changing tags
often has transient failures), but if we figure out we're blocked we
might as well just log a warning and drop the comment on the floor, it's
unlikely the situation will resolve itself.
Couldn't test it, because the block API is a developer preview and I
just can't get it to work anyway (404 on /user/blocks even providing the
suggested media type).
And the way the block is inferred is iffy (based on an error message),
the error body doesn't seem to provide any clean / clear cut error code:
{
"message": "Validation Failed",
"errors": [
{
"resource": "IssueComment",
"code": "unprocessable",
"field": "data",
"message": "User is blocked"
}
],
"documentation_url": "https://developer.github.com/v3/issues/comments/#create-a-comment"
}
No useful headers either.
Fixes #127
2019-05-07 15:36:53 +07:00
|
|
|
# if the mergebot user has been blocked by the PR author, this will
|
|
|
|
# fail, but we don't want the closing of the PR to fail, or for the
|
|
|
|
# feedback cron to get stuck
|
|
|
|
try:
|
|
|
|
self('POST', 'issues/{}/comments'.format(pr), json={'body': message})
|
|
|
|
except requests.HTTPError as r:
|
|
|
|
if _is_json(r.response):
|
|
|
|
body = r.response.json()
|
|
|
|
if any(e.message == 'User is blocked' for e in (body.get('errors') or [])):
|
2020-01-28 21:34:29 +07:00
|
|
|
_logger.warning("comment(%s#%s) failed: user likely blocked", self._repo, pr)
|
[FIX] runbot_merge: handle the bot user not being able to comment
If the author of a PR has blocked the bot user, commenting on the PR
will fail. While comment failure is technically handled in the feedback
cron, the cron will simply retry commenting on every run filling the
log with useless unactionable garbage.
Retrying is the right thing to do in the normal case (e.g. changing tags
often has transient failures), but if we figure out we're blocked we
might as well just log a warning and drop the comment on the floor, it's
unlikely the situation will resolve itself.
Couldn't test it, because the block API is a developer preview and I
just can't get it to work anyway (404 on /user/blocks even providing the
suggested media type).
And the way the block is inferred is iffy (based on an error message),
the error body doesn't seem to provide any clean / clear cut error code:
{
"message": "Validation Failed",
"errors": [
{
"resource": "IssueComment",
"code": "unprocessable",
"field": "data",
"message": "User is blocked"
}
],
"documentation_url": "https://developer.github.com/v3/issues/comments/#create-a-comment"
}
No useful headers either.
Fixes #127
2019-05-07 15:36:53 +07:00
|
|
|
return
|
|
|
|
raise
|
2018-09-20 14:25:13 +07:00
|
|
|
_logger.debug('comment(%s, %s, %s)', self._repo, pr, shorten(message))
|
2018-03-14 16:37:46 +07:00
|
|
|
|
[IMP] runbot_merge: limit spamming on PR close
When closing a PR, github completely separates the events "close the
PR" and "comment on the PR" (even when using "comment and close" in
the UI, a feature which isn't even available in the API). It doesn't
aggregate the notifications either, so users following the PR for
one reason or another get 2 notifications / mails every time a PR
gets merged, which is a lot of traffic, even more so with
forward-ported PRs multiplying the amount of PRs users are involved
in.
The comment on top of the closure itself is useful though: it allows
tracking exactly where and how the PR was merged from the PR, this
information should not be lost.
While more involved than a simple comment, *deployments* seem like
a suitable solution: they allow providing links as permanent
information / metadata on the PRs, and apparently don't trigger
notifications to users.
Therefore, modify the "close" method so it doesn't do
"comment-and-close", and provide a way to close PRs with non-comment
feedback: when the feedback's message is structured (parsable as
json) assume it's intended as deployment-bound notifications.
TODO: maybe add more keys to the feedback event payload, though in my
tests (odoo/runbot#222) none of the deployment metadata
outside of "environment" and "target_url" is listed on the PR
UI
Fixes #224
2019-11-19 16:04:44 +07:00
|
|
|
def close(self, pr):
|
2018-03-26 18:08:49 +07:00
|
|
|
self('PATCH', 'pulls/{}'.format(pr), json={'state': 'closed'})
|
2018-03-14 16:37:46 +07:00
|
|
|
|
2020-03-10 19:36:46 +07:00
|
|
|
def change_tags(self, pr, remove, add):
|
2019-08-21 16:17:04 +07:00
|
|
|
labels_endpoint = 'issues/{}/labels'.format(pr)
|
|
|
|
tags_before = {label['name'] for label in self('GET', labels_endpoint).json()}
|
2020-03-10 19:36:46 +07:00
|
|
|
tags_after = (tags_before - remove) | add
|
2019-08-21 16:17:04 +07:00
|
|
|
# replace labels entirely
|
|
|
|
self('PUT', labels_endpoint, json={'labels': list(tags_after)})
|
|
|
|
|
|
|
|
_logger.debug('change_tags(%s, %s, from=%s, to=%s)', self._repo, pr, tags_before, tags_after)
|
2018-09-20 14:25:13 +07:00
|
|
|
|
2019-11-07 14:14:45 +07:00
|
|
|
def _check_updated(self, branch, to):
|
|
|
|
"""
|
|
|
|
:return: nothing if successful, the incorrect HEAD otherwise
|
|
|
|
"""
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
r = self('get', 'git/refs/heads/{}'.format(branch), check=False)
|
|
|
|
if r.status_code == 200:
|
|
|
|
head = r.json()['object']['sha']
|
|
|
|
else:
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
head = '<Response [%s]: %s)>' % (r.status_code, r.text)
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
|
2019-11-07 14:14:45 +07:00
|
|
|
if head == to:
|
2022-07-11 13:17:04 +07:00
|
|
|
_logger.debug("Sanity check ref update of %s to %s: ok", branch, to)
|
2019-11-07 14:14:45 +07:00
|
|
|
return
|
|
|
|
|
[IMP] runbot_merge: error reporting
Largely informed by sentry,
- Fix an inconsistency in staging ref verification, `set_ref`
internally waits for the observed head to match the requested head,
but then `try_staging` would re-check that and immediately fail if
it didn't.
Because github is *eventually* consistent (hopefully) this second
check can fail (and is also an extra API call), breaking staging
unnecessarily, especially as we're still going to wait for the
update to be visible to git.
Remove this redundant check entirely, as github provides no way to
ensure we have a consistent view of anything, it doesn't have much
value and can do much harm.
- Add github request id to one of the sanity check warnings as that
could be a useful thing to send upstream, missing github request ids
in the future should be noted and added.
- Reworked the GH object's calls to be clearer and more coherent:
consistently log the same thing on all GH errors (if `check`),
rather than just on the one without a `check` entry.
Also remove `raise_for_status` and raise `HTTPError` by hand every
time we hit a status >= 400, so we always forward the response body
no matter what its type is.
- Try again to log the request body (in full as it should be pretty
small), also remove stripping since we specifically wanted to add a
newline at the start, I've no idea what I was thinking.
Fixes #735, #764, #544
2023-06-13 19:22:40 +07:00
|
|
|
_logger.warning(
|
|
|
|
"Sanity check ref update of %s, expected %s got %s (response-id %s)",
|
|
|
|
branch, to, head,
|
|
|
|
r.headers.get('x-github-request-id')
|
|
|
|
)
|
2019-11-07 14:14:45 +07:00
|
|
|
return head
|
|
|
|
|
2018-03-14 16:37:46 +07:00
|
|
|
def fast_forward(self, branch, sha):
|
|
|
|
try:
|
2018-03-26 18:08:49 +07:00
|
|
|
self('patch', 'git/refs/heads/{}'.format(branch), json={'sha': sha})
|
2018-09-20 14:25:13 +07:00
|
|
|
_logger.debug('fast_forward(%s, %s, %s) -> OK', self._repo, branch, sha)
|
2019-11-07 14:14:45 +07:00
|
|
|
@utils.backoff(exc=exceptions.FastForwardError)
|
|
|
|
def _wait_for_update():
|
|
|
|
if not self._check_updated(branch, sha):
|
|
|
|
return
|
2022-06-08 19:45:32 +07:00
|
|
|
raise exceptions.FastForwardError(self._repo) \
|
|
|
|
from Exception("timeout: never saw %s" % sha)
|
|
|
|
except requests.HTTPError as e:
|
2018-09-20 14:25:13 +07:00
|
|
|
_logger.debug('fast_forward(%s, %s, %s) -> ERROR', self._repo, branch, sha, exc_info=True)
|
2022-06-08 19:45:32 +07:00
|
|
|
if e.response.status_code == 422:
|
|
|
|
try:
|
|
|
|
r = e.response.json()
|
|
|
|
except Exception:
|
|
|
|
pass
|
|
|
|
else:
|
|
|
|
if isinstance(r, dict) and 'message' in r:
|
|
|
|
e = Exception(r['message'].lower())
|
|
|
|
raise exceptions.FastForwardError(self._repo) from e
|
2018-03-14 16:37:46 +07:00
|
|
|
|
|
|
|
def set_ref(self, branch, sha):
|
|
|
|
# force-update ref
|
2018-03-26 18:08:49 +07:00
|
|
|
r = self('patch', 'git/refs/heads/{}'.format(branch), json={
|
2018-03-14 16:37:46 +07:00
|
|
|
'sha': sha,
|
|
|
|
'force': True,
|
|
|
|
}, check=False)
|
2018-10-17 16:31:52 +07:00
|
|
|
|
|
|
|
status0 = r.status_code
|
|
|
|
_logger.debug(
|
2023-08-10 21:14:33 +07:00
|
|
|
'set_ref(%s, %s, %s -> %s (%s)',
|
2018-10-17 16:31:52 +07:00
|
|
|
self._repo, branch, sha, status0,
|
|
|
|
'OK' if status0 == 200 else r.text or r.reason
|
|
|
|
)
|
|
|
|
if status0 == 200:
|
2019-11-07 14:14:45 +07:00
|
|
|
@utils.backoff(exc=AssertionError)
|
|
|
|
def _wait_for_update():
|
|
|
|
head = self._check_updated(branch, sha)
|
|
|
|
assert not head, "Sanity check ref update of %s, expected %s got %s" % (
|
|
|
|
branch, sha, head
|
|
|
|
)
|
2018-03-14 16:37:46 +07:00
|
|
|
return
|
|
|
|
|
2018-06-07 19:44:44 +07:00
|
|
|
# 422 makes no sense but that's what github returns, leaving 404 just
|
|
|
|
# in case
|
2018-10-17 16:31:52 +07:00
|
|
|
if status0 in (404, 422):
|
2018-03-14 16:37:46 +07:00
|
|
|
# fallback: create ref
|
2022-07-11 13:17:04 +07:00
|
|
|
status1 = self.create_ref(branch, sha)
|
2018-10-17 16:31:52 +07:00
|
|
|
if status1 == 201:
|
2018-03-14 16:37:46 +07:00
|
|
|
return
|
2022-07-11 13:17:04 +07:00
|
|
|
else:
|
|
|
|
status1 = None
|
2018-10-17 16:31:52 +07:00
|
|
|
|
|
|
|
raise AssertionError("set_ref failed(%s, %s)" % (status0, status1))
|
2018-03-14 16:37:46 +07:00
|
|
|
|
2022-07-11 13:17:04 +07:00
|
|
|
def create_ref(self, branch, sha):
|
|
|
|
r = self('post', 'git/refs', json={
|
|
|
|
'ref': 'refs/heads/{}'.format(branch),
|
|
|
|
'sha': sha,
|
|
|
|
}, check=False)
|
|
|
|
status = r.status_code
|
|
|
|
_logger.debug(
|
|
|
|
'ref_create(%s, %s, %s) -> %s (%s)',
|
|
|
|
self._repo, branch, sha, status,
|
|
|
|
'OK' if status == 201 else r.text or r.reason
|
|
|
|
)
|
|
|
|
if status == 201:
|
|
|
|
@utils.backoff(exc=AssertionError)
|
|
|
|
def _wait_for_update():
|
|
|
|
head = self._check_updated(branch, sha)
|
|
|
|
assert not head, \
|
|
|
|
f"Sanity check ref update of {branch}, expected {sha} got {head}"
|
|
|
|
return status
|
|
|
|
|
2018-08-28 20:42:28 +07:00
|
|
|
def merge(self, sha, dest, message):
|
|
|
|
r = self('post', 'merges', json={
|
|
|
|
'base': dest,
|
|
|
|
'head': sha,
|
|
|
|
'commit_message': message,
|
2021-08-03 18:45:21 +07:00
|
|
|
}, check={409: MergeError})
|
2018-10-08 21:30:51 +07:00
|
|
|
try:
|
|
|
|
r = r.json()
|
|
|
|
except Exception:
|
2023-06-14 18:34:22 +07:00
|
|
|
raise MergeError("got non-JSON reponse from github: %s %s (%s)" % (r.status_code, r.reason, r.text))
|
2019-11-07 14:14:45 +07:00
|
|
|
_logger.debug(
|
|
|
|
"merge(%s, %s (%s), %s) -> %s",
|
|
|
|
self._repo, dest, r['parents'][0]['sha'],
|
|
|
|
shorten(message), r['sha']
|
|
|
|
)
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
return dict(r['commit'], sha=r['sha'], parents=r['parents'])
|
2018-08-28 20:42:28 +07:00
|
|
|
|
|
|
|
def rebase(self, pr, dest, reset=False, commits=None):
|
|
|
|
""" Rebase pr's commits on top of dest, updates dest unless ``reset``
|
|
|
|
is set.
|
|
|
|
|
2019-07-31 14:19:39 +07:00
|
|
|
Returns the hash of the rebased head and a map of all PR commits (to the PR they were rebased to)
|
2018-08-28 20:42:28 +07:00
|
|
|
"""
|
2019-01-25 21:45:38 +07:00
|
|
|
logger = _logger.getChild('rebase')
|
2018-08-28 20:42:28 +07:00
|
|
|
original_head = self.head(dest)
|
|
|
|
if commits is None:
|
|
|
|
commits = self.commits(pr)
|
|
|
|
|
2019-01-25 21:45:38 +07:00
|
|
|
logger.debug("rebasing %s, %s on %s (reset=%s, commits=%s)",
|
|
|
|
self._repo, pr, dest, reset, len(commits))
|
|
|
|
|
2023-06-14 18:34:22 +07:00
|
|
|
if not commits:
|
|
|
|
raise MergeError("PR has no commits")
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
prev = original_head
|
|
|
|
for original in commits:
|
2023-06-14 18:34:22 +07:00
|
|
|
if len(original['parents']) != 1:
|
|
|
|
raise MergeError(
|
|
|
|
"commits with multiple parents ({sha}) can not be rebased, "
|
|
|
|
"either fix the branch to remove merges or merge without "
|
|
|
|
"rebasing".format_map(
|
|
|
|
original
|
|
|
|
))
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
tmp_msg = 'temp rebasing PR %s (%s)' % (pr, original['sha'])
|
|
|
|
merged = self.merge(original['sha'], dest, tmp_msg)
|
|
|
|
|
|
|
|
# whichever parent is not original['sha'] should be what dest
|
|
|
|
# deref'd to, and we want to check that matches the "left parent" we
|
|
|
|
# expect (either original_head or the previously merged commit)
|
|
|
|
[base_commit] = (parent['sha'] for parent in merged['parents']
|
|
|
|
if parent['sha'] != original['sha'])
|
2023-06-14 18:34:22 +07:00
|
|
|
if prev != base_commit:
|
|
|
|
raise MergeError(
|
|
|
|
f"Inconsistent view of branch {dest} while rebasing "
|
|
|
|
f"PR {pr} expected commit {prev} but the other parent of "
|
|
|
|
f"merge commit {merged['sha']} is {base_commit}.\n\n"
|
|
|
|
f"The branch may be getting concurrently modified."
|
[IMP] runbot_merge: sanity check when rebasing
Ensure that the commits we're creating are based on the commit we're
expecting.
This is the second cause (and really the biggest issue) of the "Great
Reset" of master on November 6: a previous commit explains the issue
with non-linear github operations (update a branch, get the branch
head, they don't match).
The second issue is why @awa-odoo's PR was merged with a reversion of
@tivisse's as part of its first commit.
The stage for this issues is based on the incoherence noted above:
having updated a branch, getting that branch's head afterward may
still return the old head. However either delays allow that update to
be visible *or* different operations can have different views of the
system. Regardless it's possible that `repos/merges` "sees" a
different branch head than a `git/refs/heads` which preceded it by a
few milliseconds. This is an issue because github's API does not
provide a generic "rebase" operation, and the operation is thus
implemented by hand:
1. get the head of the branch we're trying to rebase a PR on
2. for each commit of the PR (oldest to newest), *merge* commit on the
base and associate the merge commit with the original
3. reset the branch to the head we stored previously
4. for each commit of the PR, create a new commit with:
- the metadata of the original
- the tree of the merge commit
- the "current head" as parent
then update the "current head" to that commit's ref'
If the head fetched at (1) and the one the first merge of (2) sees are
different, the first commit created during (4) will look like it has
not only its own changes but also all the changes between the two
heads, as github records not changes but snapshots of working
copies (that's what a git tree is, a complete snapshot of the entire
state of a working copy).
As a result, we end up not only with commits from a previous staging
but the first commit of the next PR rollbacks the changes of those
commits, making a mess of the entire thing twice over. And because the
commits of the previous staging get reverted, even if there was a good
reason for them to fail (not the case here it was a false positive)
the new staging might just go through.
As noted at the start, mitigate that by asserting that the merge
commits created at (2) have the "base parent" (left parent / parent
from the base branch) we were expecting, and cancel the staging if
that's not the case.
This can probably be removed if / when odoo/runbot#247 happens.
2019-11-07 18:00:42 +07:00
|
|
|
)
|
|
|
|
prev = merged['sha']
|
|
|
|
original['new_tree'] = merged['tree']['sha']
|
2018-08-28 20:42:28 +07:00
|
|
|
|
|
|
|
prev = original_head
|
2019-07-31 14:19:39 +07:00
|
|
|
mapping = {}
|
2018-08-28 20:42:28 +07:00
|
|
|
for c in commits:
|
2022-02-07 18:00:31 +07:00
|
|
|
committer = c['commit']['committer']
|
|
|
|
committer.pop('date')
|
2018-08-28 20:42:28 +07:00
|
|
|
copy = self('post', 'git/commits', json={
|
|
|
|
'message': c['commit']['message'],
|
|
|
|
'tree': c['new_tree'],
|
|
|
|
'parents': [prev],
|
|
|
|
'author': c['commit']['author'],
|
2022-02-07 18:00:31 +07:00
|
|
|
'committer': committer,
|
2021-08-03 18:45:21 +07:00
|
|
|
}, check={409: MergeError}).json()
|
2019-01-25 21:45:38 +07:00
|
|
|
logger.debug('copied %s to %s (parent: %s)', c['sha'], copy['sha'], prev)
|
2019-07-31 14:19:39 +07:00
|
|
|
prev = mapping[c['sha']] = copy['sha']
|
2018-08-28 20:42:28 +07:00
|
|
|
|
|
|
|
if reset:
|
|
|
|
self.set_ref(dest, original_head)
|
2018-09-20 15:08:08 +07:00
|
|
|
else:
|
|
|
|
self.set_ref(dest, prev)
|
2018-08-28 20:42:28 +07:00
|
|
|
|
2019-01-25 21:45:38 +07:00
|
|
|
logger.debug('rebased %s, %s on %s (reset=%s, commits=%s) -> %s',
|
|
|
|
self._repo, pr, dest, reset, len(commits),
|
2018-09-20 14:25:13 +07:00
|
|
|
prev)
|
2018-08-28 20:42:28 +07:00
|
|
|
# prev is updated after each copy so it's the rebased PR head
|
2019-07-31 14:19:39 +07:00
|
|
|
return prev, mapping
|
2018-03-14 16:37:46 +07:00
|
|
|
|
2018-06-21 14:55:14 +07:00
|
|
|
# fetch various bits of issues / prs to load them
|
|
|
|
def pr(self, number):
|
|
|
|
return (
|
|
|
|
self('get', 'issues/{}'.format(number)).json(),
|
|
|
|
self('get', 'pulls/{}'.format(number)).json()
|
|
|
|
)
|
|
|
|
|
|
|
|
def comments(self, number):
|
|
|
|
for page in itertools.count(1):
|
2018-08-28 20:42:28 +07:00
|
|
|
r = self('get', 'issues/{}/comments'.format(number), params={'page': page})
|
2018-06-21 14:55:14 +07:00
|
|
|
yield from r.json()
|
|
|
|
if not r.links.get('next'):
|
|
|
|
return
|
|
|
|
|
|
|
|
def reviews(self, number):
|
|
|
|
for page in itertools.count(1):
|
2018-08-28 20:42:28 +07:00
|
|
|
r = self('get', 'pulls/{}/reviews'.format(number), params={'page': page})
|
2018-06-21 14:55:14 +07:00
|
|
|
yield from r.json()
|
|
|
|
if not r.links.get('next'):
|
|
|
|
return
|
|
|
|
|
2018-09-20 20:02:18 +07:00
|
|
|
def commits_lazy(self, pr):
|
|
|
|
for page in itertools.count(1):
|
|
|
|
r = self('get', 'pulls/{}/commits'.format(pr), params={'page': page})
|
|
|
|
yield from r.json()
|
|
|
|
if not r.links.get('next'):
|
|
|
|
return
|
|
|
|
|
2018-08-28 20:42:28 +07:00
|
|
|
def commits(self, pr):
|
|
|
|
""" Returns a PR's commits oldest first (that's what GH does &
|
|
|
|
is what we want)
|
|
|
|
"""
|
2018-10-09 20:01:45 +07:00
|
|
|
commits = list(self.commits_lazy(pr))
|
|
|
|
# map shas to the position the commit *should* have
|
|
|
|
idx = {
|
|
|
|
c: i
|
|
|
|
for i, c in enumerate(topological_sort({
|
|
|
|
c['sha']: [p['sha'] for p in c['parents']]
|
|
|
|
for c in commits
|
|
|
|
}))
|
|
|
|
}
|
|
|
|
return sorted(commits, key=lambda c: idx[c['sha']])
|
2018-08-28 20:42:28 +07:00
|
|
|
|
2018-06-21 14:55:14 +07:00
|
|
|
def statuses(self, h):
|
|
|
|
r = self('get', 'commits/{}/status'.format(h)).json()
|
|
|
|
return [{
|
|
|
|
'sha': r['sha'],
|
2018-09-17 16:04:31 +07:00
|
|
|
**s,
|
2018-06-21 14:55:14 +07:00
|
|
|
} for s in r['statuses']]
|
2018-08-28 20:42:28 +07:00
|
|
|
|
2018-09-20 14:25:13 +07:00
|
|
|
def shorten(s):
|
|
|
|
if not s:
|
|
|
|
return s
|
|
|
|
|
|
|
|
line1 = s.split('\n', 1)[0]
|
|
|
|
if len(line1) < 50:
|
|
|
|
return line1
|
|
|
|
|
|
|
|
return line1[:47] + '...'
|