Allow for email notifications with GitLab #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
December 5, 2020 14:54
…itHub). - Modify handlePR.js to instantiate a generic GitService instead of GitHub. - Modify GitHub.js so that a blank (Staticman) version number can resolve to GitHub token authorization (as the version number is not available when instantiating GitHub from handlePR). - Modify server.js to provide v3 service-specific "webhook" endpoints. Retain usage of the express-github-webhook module for GitHub. - Update tests and add test cases for GitLab. - Add new webhook controller.
…error handling, and webhook request authentication. - Add support in the Staticman JSON configuration for GitLab and GitHub webhook request authentication via "gitlabWebhookSecret" and "githubWebhookSecret" properties. - In the handlePR controller, pass real values for the "branch" to the GitFactory. - In the handlePR controller, pass a value for "username" to the GitFactory that identifies the repo owner, not the submitter of the merge/pull request. - In the handlePR controller, when appropriate, return helpful error messages instead of simply null. - In the handlePR controller, fix the "staticman_" source branch test. - In the handlePR controller, clean up the analytics events invocations, as the logic is doing more than just deleting the source branch. - Improve server.js to silence UnhandledPromiseRejectionWarning messages. - Update and add unit tests.
- Remove commented-out code. - Add explicit error handler to allows for overriding the system/express error handler.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main thrust of these modifications is to enable the sending of email notifications to comment subscribers when the backing git service is GitLab (as opposed to GitHub).
I see that @OlafHaag mentioned this as being an issue in 2019:
eduardoboucas#22 (comment)
My understanding of the history of this issue is that when @ntsim added GitLab support to the codebase in 2018, they left the webhook endpoint untouched. This was because, in GitLab, merge requests are set to automatically close the source branch:
eduardoboucas#219
However, it looks like @eduardoboucas added in support for sending email notifications (via webhook) as far back as 2016:
eduardoboucas#42 (comment)
Not sure why the miss, but these modifications address it, regardless.
Staticman v3 service-specific
webhookendpoints have been added (i.e.,/v3/webhook/githuband/v3/webhook/gitlab). In the corresponding code, support has been added for webhook request authentication, which may be configured via the JSON config. This amounts to allowing forgitlabWebhookSecretandgithubWebhookSecretproperties to be set, thereby triggering webhook request authentication for GitHub and/or GitLab. If either secret is configured in Staticman, but missing from the webhook calls (or not valued as expected), an error is thrown.This also addresses eduardoboucas#389
Update: The webhook request authentication functionality introduced here is further refined in #8