feat: Improved server, security and commands #529
Conversation
Signed-off-by: Luca Simone <[email protected]>
|
@okotek |
| log_context["api_url"] = url | ||
| with get_logger().contextualize(**log_context): | ||
| background_tasks.add_task(PRAgent().handle_request, url, body) | ||
|
|
||
|
|
||
| @router.post("/webhook") | ||
| @router.post("/") |
There was a problem hiding this comment.
Why did you change the webhook endpoint to /?
There was a problem hiding this comment.
Because on the configuration page in bitbucket, there is a Test connection button, if the webhook has the same get and post URL you can see if the call succeeds or not otherwise this test will always fail.
An example, it succeeds because the URL supports get and post
|
|
||
| router = APIRouter() | ||
|
|
||
|
|
||
| def handle_request(background_tasks: BackgroundTasks, url: str, body: str, log_context: dict): | ||
| def handle_request( |
There was a problem hiding this comment.
Why did you change the parenthesis styling?
There was a problem hiding this comment.
Black... I linted and formatted with black
| if webhook_secret: | ||
| body_bytes = await request.body() | ||
| signature_header = request.headers.get("x-hub-signature", None) | ||
| verify_signature(body_bytes, webhook_secret, signature_header) |
There was a problem hiding this comment.
Did you verify that it works as expected?
There was a problem hiding this comment.
Yes it works... the HMAC method is the same as GitHub just the header has a slightly different name (without -256)
| elif data["eventKey"] == "pr:comment:added": | ||
| body = data["comment"]["text"] | ||
| else: | ||
| return JSONResponse( |
There was a problem hiding this comment.
I think we should return 200 in this case, this is not an error, unless you know differently?
There was a problem hiding this comment.
Well we can, but you have to configure which events you want to be pushed as webhook. If you push some events that are not supported it could be a bad request, or could be a 200 and who cares or, could also be a 501 not implemented which makes also sense.
There was a problem hiding this comment.
I'm asking before in some cases, after a certain threshold of errors the webhook is disabled.
| app.include_router(router) | ||
| uvicorn.run(app, host="0.0.0.0", port=3000) | ||
| uvicorn.run(app, host="0.0.0.0", port=int(os.environ.get("PORT", "3000"))) |
There was a problem hiding this comment.
Please change this to BITBUCKET_SERVER.PORT, use get_settings().get(), add to configuration file
There was a problem hiding this comment.
It's like this because some cloud providers uses an environment variable PORT to bind the ingress... also in bitbucket_app.py is done like this:
https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/servers/bitbucket_app.py#L162
There was a problem hiding this comment.
can't complain on my own code :(
…ements feat: Improved server, security and commands
I improved the Bitbucket server to enable all commands like review, ask and so on... plus I added the missing webhook HMAC security check.