[IMP] runbot_merge: add callback on patch application#1430
Conversation
dylankiss
force-pushed
the
17.0-imp-merge-commit-callback
branch
from
51458a8 to
329e5bb
Compare
| data = { | ||
| 'jsonrpc': '2.0', | ||
| 'id': None, | ||
| 'method': 'call', | ||
| 'params': { | ||
| 'repository': self.repository.name, | ||
| 'branch': self.target.name, | ||
| 'patch': self.id, | ||
| 'success': success, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Unnecessarily complicated, this has no reason to be jsonrpc it should just be an HTTP POST to the specified URL (which is what the help text hints at), and since the callback is per-patch anyway it should just add the success state to the query parameters: the caller can set whatever state they need in the URL they set.
There was a problem hiding this comment.
Regarding jsonrpc, I just took the same approach as the /i18n/merge_commit route in Mergebot. I suppose that doesn't require jsonrpc either then?
Anyway, you're right indeed that it can be made simpler by already passing the necessary state in the URL. Adapting it.
There was a problem hiding this comment.
Regarding jsonrpc, I just took the same approach as the
/i18n/merge_commitroute in Mergebot. I suppose that doesn't require jsonrpc either then?
Strictly speaking it doesn't but that's an internal communication between odoo services (and very specifically between the translation server and the mergebot) and that's how mat wrote it, whereas this is more of a general purpose extension.
Incidentally something else I thought of yesterday is that SaasAuth is not necessary either, for authentication of the hook / callback the requesting service can set some sort of random one-time code (which can be the entirety of the state on its side). The callback system is not tied to tx, or to contacting internal odoo services.
| # TODO: do better | ||
| body=e.args[0] if isinstance(e.args[0], Markup) else Markup("<pre>{}</pre>").format(e), | ||
| ) | ||
| patch._fire_callback(success=False) |
There was a problem hiding this comment.
Needs to move out of line to a cron job.
There was a problem hiding this comment.
Just to be sure: you want me to trigger a cronjob that does this instead (so it doesn't block the flow)?
There was a problem hiding this comment.
Indeed. A cron on a queue which should probably just take the callback URL and something like a JSON payload of additional parameters (that way if the callback queue is reused for other services that works out of the box).
| }, | ||
| } | ||
| try: | ||
| res = requests.post(self.callback_url, json=data, auth=SaasAuth(), timeout=30) |
There was a problem hiding this comment.
30 seconds timeout is way too long.
There was a problem hiding this comment.
Yeah, probably. It's because the other side is also doing a request to Weblate to unlock components. I can probably move that to a cronjob as well. What timeout would you suggest?
There was a problem hiding this comment.
Github webhooks require a response within 10s, that seems pretty reasonable.
dylankiss
force-pushed
the
17.0-imp-merge-commit-callback
branch
from
329e5bb to
56942ee
Compare
dylankiss
force-pushed
the
17.0-imp-merge-commit-callback
branch
from
56942ee to
fb8a20f
Compare
We introduce a mechanism of calling a callback URL with the status of a patch when it either succeeded or failed to apply. The translations sync server uses this to know when to unlock the relevant components in Weblate.
dylankiss
force-pushed
the
17.0-imp-merge-commit-callback
branch
from
fb8a20f to
eb9c0f0
Compare
|
@xmo-odoo I tried to rework the flow as to your suggestions. |
|
@xmo-odoo Small reminder. It'd great if you could have another look 🙏 |
|
FWIW I reworked it locally to use the mergebot's retryable queue crons instead of being bespoke, I also changed the value from a direct-serialized python boolean to an integer (1 for success, currently 0 for failure but technically codes -- probably negative -- could be sent for more specific failures if that's of use). I'll close the PR when it's deployed (unless I forget). |
2 participants
The Weblate sync server uses this to know when to unlock its components.
Related: https://github.com/odoo/internal/pull/4231