Skip to content
This repository has been archived by the owner on Aug 27, 2024. It is now read-only.

feat: Support webhook url config key #564

Merged
merged 2 commits into from
Jun 7, 2019
+79 −1
Merged

feat: Support webhook url config key #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb13bab
feat: Support webhook url config key
startnow65 Feb 11, 2019
2770742
add note about HOOK_URL config entry
startnow65 May 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ VALID_PR_TEMPLATE_PATHS:
- "/PULL_REQUEST_TEMPLATE"
- "/PULL_REQUEST_TEMPLATE.txt"
- "/PULL_REQUEST_TEMPLATE.md"

# Explicit webhook url to use for receiving webhooks from Github
# If this is commented out, Zappr uses "<HOST_ADDR config value>/api/hook' as the webhook url
# HOOK_URL: http://mywebhookdomain/zappr/api/hook

ZAPPR_DEFAULT_CONFIG:
autobranch:
pattern: "{number}-{title}"
Expand Down
1 change: 1 addition & 0 deletions docs/run-your-own.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Zappr provides a whole lot of configuration options for flexibility.
* `GITHUB_HOOK_SECRET`: Secret value used to verify only Github calls Zappr
* `GITHUB_UI_URL`: URL to Github web user interface
* `GITHUB_API_URL`: URL to Github API
* `HOOK_URL`: Optional explicit webhook url to use for receiving webhooks from Github. Required if there is a proxy or load balancer that receives traffic from the internet and forwards it to Zappr.

### Server

Expand Down
3 changes: 2 additions & 1 deletion server/service/GithubService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// payload for hook
let payload = {
name: 'web',
Expand Down
71 changes: 71 additions & 0 deletions test/server/github.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,4 +317,75 @@ describe('The Github service', () => {
}
})
})

describe('#updateWebhookFor', () => {
it('should build webhook url from host address', async(done) => {
try {
const USER = 'user'
const REPO = 'repo'
const TOKEN = 'token'

github.fetchPath.returns([])
nconf.set('HOOK_URL', null)

await github.updateWebhookFor(USER, REPO, [], TOKEN)
expect(github.fetchPath.args).to.deep.equal([
['GET', `/repos/${USER}/${REPO}/hooks`,
null,
TOKEN],
['POST', `/repos/${USER}/${REPO}/hooks`,
{
name: 'web',
active: true,
events: [],
config: {
url: nconf.get('HOST_ADDR') + '/api/hook',
content_type: 'json',
secret: nconf.get('GITHUB_HOOK_SECRET')
}
},
TOKEN]
])

done()
} catch (e) {
done(e)
}
})

it('should use provided webhook url', async(done) => {
try {
const USER = 'user'
const REPO = 'repo'
const TOKEN = 'token'
const HOOK_URL = 'hookurl'

github.fetchPath.returns([])
nconf.set('HOOK_URL', HOOK_URL)

await github.updateWebhookFor(USER, REPO, [], TOKEN)
expect(github.fetchPath.args).to.deep.equal([
['GET', `/repos/${USER}/${REPO}/hooks`,
null,
TOKEN],
['POST', `/repos/${USER}/${REPO}/hooks`,
{
name: 'web',
active: true,
events: [],
config: {
url: HOOK_URL,
content_type: 'json',
secret: nconf.get('GITHUB_HOOK_SECRET')
}
},
TOKEN]
])

done()
} catch (e) {
done(e)
}
})
})
})