feat: Support webhook url config key #564
Conversation
|
Thanks @startnow65 for contributing. I just wanted to inform you that we are currently, focused on #560 and #556 once they are merged, we will be able to merge yours. |
|
Hi @fokusferit done with the blocking issues now ? |
| @@ -268,7 +268,8 @@ export class GithubService { | |||
| async updateWebhookFor(user, repo, events, accessToken) { | |||
| debug(`${user}/${repo}: updating webhook with events: ${events.join(", ")}`) | |||
| let path = API_URL_TEMPLATES.HOOK.replace('${owner}', user).replace('${repo}', repo) | |||
| let hook_url = nconf.get('HOST_ADDR') + '/api/hook' | |||
| const HOOK_URL = nconf.get('HOOK_URL') | |||
| let hook_url = HOOK_URL ? HOOK_URL : nconf.get('HOST_ADDR') + '/api/hook' | |||
There was a problem hiding this comment.
Why providing a complete custom configuration for the Hooks API ?
This would allow a Zappr deployment where you could write just any endpoint and than your Zappr instance won't receive any data.
Therefore, not seeing any value here w/o opining up Zappr to potential problems.
There was a problem hiding this comment.
This is for specific use cases where webhooks from the internet to internal systems (Zappr, CI, et.c) go through a single endpoint.
For example, in a case where Zappr is not accessible from the internet but a single endpoint, say https://my-webhook-entry-point/zappr is accessible to the internet and forwards requests (webhooks) internally to Zappr. Zappr should therefore set https://my-webhook-entry-point/zappr as the webhook on Github and not https://zappr-internal-domain/api/hook, since https://zappr-internal-domain/api/hook would not be reachable from the internet.
Please let me know if you need more information
There was a problem hiding this comment.
Thanks a lot for the explanation! This makes totally sense. Hopefully this week we can merge this PR. We are right now finishing our internal testing of the new feature.
There was a problem hiding this comment.
One thing, now that I'm rereading it. In your case, wouldn't the setting be setup on the Github OAuth Apps page rather than here? This Hook_URL is for internal usage in Zappr, which in that case should be the internal Domain which the Loadbalancer will redirect to, right?
There was a problem hiding this comment.
The one on the Github Oauth Apps page is different, mostly used for web oauth flow (redirect URL). This one is set per github repo as part of setting up a webhook.
Since Zappr sets up the webhook as part of "activating" the approval check, it builds the webhook url from its own LB url, this config key allows us specify the webhook url Zappr should set for the repo.
|
@startnow65 sorry, now I see your details in the issue description. Before I move here, I will follow up on the issue list. |
|
@fokusferit @startnow65 is pr still relevant? |
|
@startnow65 generally, this PR we would like merge it, but could you also update the README / Documentation so the functionality is visible? |
Thanks @fokusferit I've added a note to |
|
👍 |
|
Pinging @rashamalek :) |
|
👍 |
Implements #561
Adds a new config key
HOOK_URLto support explicitly setting the webhook url to be used by Zappr