Cleanups and fixes #1
Conversation
- Allow the webhook to run on 0.0.0.0 so that it can be called by others. - Add log verbosity option (so it is not always debug). - Allow clients from config to pass there own proxy timeout (used when sending calls to them); defaults to a new global (PROXY_TIMEOUT) - Use the github meta cidrs and validate against them and cache them (in memory) for up to 1 hour. - Avoid blocking the response back to github and have all the proxying happen on a different part of the event loop (this way github will get a 200 back very quickly). - Pass through 'X-GitHub-Delivery' header - Use the right imports (flake8 errors fixed). - Add in allowed ips from configuration (used along with the github cidrs to validate incoming ips); useful to allow specific other ips to call into post hooks. - Logging cleanups and improvements. - Import the right things (vs not).
There was a problem hiding this comment.
Hey @harlowja 👋
So I'd need to remember how all this works and spin up an environment to actually test this out. The review notes are just first glance things.
The BonnyCI org hasn't been active in about a year now, are you looking to use this for something? I've been out of the Zuul loop, but i'm sure we'd be happy to donate it if it's useful to you.
What's the goal updating the old repo?
I'm happy someone's looking at this stuff again, there's some good bits in here :)
| now = time.monotonic() | ||
| if (self.hook_blocks.last_updated is None or | ||
| (now - self.hook_blocks.last_updated) > GITHUB_META_CACHE_TTL): | ||
| resp = requests.get(GITHUB_META_URL, |
There was a problem hiding this comment.
this would be a sync request in an async block.
There was a problem hiding this comment.
I suppose that could be changed, though I'm hoping its once per hour so does it matter that much?
|
|
||
| def sig_handler(): | ||
| LOG.info("Reloading configuration from %s", app.config_file) | ||
| app.load_config() | ||
|
|
||
| if app: | ||
| app.loop.add_signal_handler(signal.SIGHUP, sig_handler) | ||
| aiohttp.web.run_app(app.app, host='127.0.0.1', port=8080) | ||
| if opts.expose: |
There was a problem hiding this comment.
would prefer to specify a bind IP address, more generally useful. (although maybe less so in a docker world)
|
And also 👋 lol |
Two commits (ya)! One is bigger :-/