Allow for email notifications with GitLab #388
Conversation
…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.
|
@hispanic thank you for this contribution! I wanted to comment initially just to say that we appreciate the work that you've put into this PR and I'll do my best to find some time to review it. Just wanted to give you a heads up that it may take me a bit to get to reviewing this but it's on my todo list. |
|
@alexwaibel Thanks! No need to expedite. Please LMK if you have any concerns with my submission. I have my eye on submitting a few more PRs. |
|
@alexwaibel A question to consider when reviewing. Should the v3 webhook endpoints contain the :username and :repository path segments? As is coded in this PR, the paths stop at :service (i.e., "/v3/webhook/github" and "/v3/webhook/gitlab") because I didn't need them to be included. However, it seems like always placing the :username and :repository path segments in the endpoints allows for looking up the git-resident YAML config file, which provides good flexibility. With that being said, I have to wonder about the role the git-resident YAML config file will/should play going forward. It seems to me that the motivation behind it was to allow for multiple user-specific git repos to all point at one generic Staticman install, while still allowing for some customization. But, with the hosted install at api.staticman.net failing under the demand and it now being a requirement that users stand-up their own installs of Staticman, I have to wonder if there's much of any point in continuing to use the git-resident YAML config file (in addition to the codebase-resident JSON config file). If for no other reason, it would make things less confusing to move toward one config file. |
|
I haven't worked with this part of the codebase before so I'm less familiar with its design. I agree that it seems that there's not a very good way to continue to use My initial reaction is that we should do the following:
Thoughts? I think those changes would be a big help in bringing this part of the codebase up to speed with the rest of the design and keeping the GitHub and GitLab experiences consistent. |
|
Thanks for reviewing, @alexwaibel
Agreed. The
I don't follow here. As I've currently got it coded, the
The
Does this imply that you believe that the v3 webhook endpoints should contain the :username and :repository path segments? I'm happy to make these updates, but would you be agreeable to me handling them as a separate issue/PR? Including the last item I've got listed here in this PR would be easy enough, though. Please let me know your thoughts. Thanks for the feedback! |
I think we're essentially saying the same thing. I meant only to suggest that once we remove this.server.post(
'/:version/webook?/:service/:username/:repository',
this.bruteforce.prevent,
this.requireApiVersion([1, 3]),
this.controllers.handlePR
)To match all three endpoints:
And subsequently pass them off to one controller method which would in turn reduce a lot of the complexity added to
From my understanding, handlePR is named as such because we currently only use the webhooks for when PRs have been merged. I believe the only function of the webhook is to delete merged branches (if not already deleted) and to send a notification to subscribers if that feature is set up on the instance. I'm fine with either name, the more important thing is that I think they shouldn't be separate files as they are now.
Sorry, I should have clarified. Yes I think these elements should be included in the endpoint. This endpoint is something a user enters into GH or GL once when setting up their app and then likely won't edit again so even though we don't currently use the username or repository in this API call, IMO it doesn't hurt to add them to the route right now in case we eventually need them. It would also be good to make a PR to update the documentation website with the new endpoint format so we can try to merge both in at the same time and keep the docs updated.
I would prefer the follow up was done as revisions to this PR. Most of the follow up is refactoring so it's still within the scope of this PR vs a new one. Hopefully this answered your questions. Let me know if you have any more! |
O-h-h-h, I gotcha'. I didn't realize that the question mark can act as a regex-style matcher. Agreed.
Agreed. I'm partial to "webhook", as it would future-proof us for the possibility that we start accepting webhook calls for events other than PR merges.
Agreed.
Agreed. I have mentioned doc updates in the other PRs I've submitted.
Understood. Fortunately, it doesn't look like I've hit the relevant files very hard as part of my subsequent coding efforts. As such, it shouldn't be a significant burden. If I'm mistaken, I'll let you know. Thanks! |
…oint. - Merge logic contained in the handlePR module into the webhook controller, as the handlePR module was only a by-product of using the express-github-webhook module. - Improve error-handling so that an error raised when attempting to send notification emails does not prevent deletion of the source branch. - Improve error-handling such that multiple errors to be raised and reported back as part of webhook request processing. - In SubscriptionsManager, raise an error when attempting to send notifications if the target mailing list is not found. - Update unit tests.
|
Done! Updating I look forward to seeing this merged in and moving on to #391 and #393, as I've got a couple more PRs queued-up that are downstream of those two. Thanks! |
There was a problem hiding this comment.
@hispanic this looks awesome so far! Thanks for all the work you've done on this, I'm excited for us to get it merged in.
I left a few suggestion comments. Feel free respond to any of them with questions and we can discuss further.
|
Sorry if it looks like a ton of comments, but many of them are small changes. I tried to be thorough in my review |
|
No need to apologize. You're taking ownership and setting high standards. I appreciate that. I completely agree with the use of self-documenting function names. No concerns there. In general, I've never hesitated to add comments that help communicate insights that I've gathered while working on a piece of code - so as to spare future contributors time and frustration. I also freely comment to communicate motivations behind non-obvious deviations (e.g., foregoing the use of the ErrorHandler). But, I can see that erring on the side of terseness is preferred here, and that's fine. I can roll with it. I'll be pushing the requested changes shortly and will then go through and respond to the above comments. Thanks! |
…ments and some refactoring. - Add unit test.
|
Whoops. I missed a bunch of comments/changes that were collapsed. Will work on them. |
…ments and some refactoring.
|
I believe I've now addressed all of the requested changes. LMK. Thanks. |
config.js
Outdated
| @@ -84,6 +84,12 @@ const schema = { | |||
| default: null, | |||
| env: 'GITHUB_TOKEN' | |||
| }, | |||
| githubWebhookSecret: { | |||
There was a problem hiding this comment.
After thinking about this further, and digging into the GH docs a bit I realize that I had initially missed that this webhook secret is set on a per/repo basis.
With that in mind, I think it makes more sense for those values to be set in siteConfig.js and read as part of the per-repo config. This will require the value be encrypted, but we already do that with other values in that config. Thoughts? I think this is an important distinction so that one Staticman instance can serve multiple projects and/or users.
There was a problem hiding this comment.
Good catch. I just sort of followed along with the precedent set by githubToken being in the JSON config. However, that is for the bot, which can be shared. Might be worth considering renaming that property to githubBotToken. Anyway, I'll work on moving both secrets to the YAML site config. Thanks.
There was a problem hiding this comment.
OK, the configuration of the expected webhook secrets have been moved from the JSON config to the YAML siteConfig. I updated the unit tests and manually tested on Heroku against GitHub (endpoints v1 and v3) and GitLab (v3), using various permutations of secret configurations. Review away. :)
I hope I haven't discouraged you from leaving comments. Please don't hesitate to document things you come across on the project so others can learn from it too. We just have to be a bit picky about which comments we merge since they tend to build up over time and become clutter. Sometimes code comments really are necessary, but more often than not they identify a situation where we should refactor to make the code more clear. |
|
No problem, Alex. We're good. I'll toss them in and you just let me know whichever ones you feel are excessive. No offense taken. |
… of globally per Staticman install. - This allows for one Staticman install to be shared by multiple git repositories. - Configure for the webhook secret to be encrypted, as it could be housed in a publicly-accessible repository. - Add "property" parameter to webhook endpoint so we can retrieve the repo-resident site config. - In webhook test, fix syntax error that prevented GitLab variants from being executed. - Improve robustness of webhook tests by verifying that GitHub signature verification is performed when expected.
- In webhook test, fix syntax error that prevented GitLab variants from being executed. - Improve robustness of webhook tests by verifying that GitHub signature verification is performed when expected. - Properly resolve the mock promise returned by getSiteConfig()
|
@alexwaibel Were you notified about the commit I pushed several days ago? Or did you just get busy? |
Yeah, I've been meaning to review this again just haven't have time to get to it yet. Thanks for the reminder, I should have some time to give it another look soon. |
|
Sounds good. Just making sure. Thanks. |
| const path = require('path') | ||
| const config = require(path.join(__dirname, '/../config')) |
There was a problem hiding this comment.
| const path = require('path') | |
| const config = require(path.join(__dirname, '/../config')) | |
| const config = require('../config') |
nit: I don't think the path join is needed here
| } | ||
| } | ||
|
|
||
| const _calcIsVersion1WhenGitHubAssumed = function (req) { |
There was a problem hiding this comment.
| const _calcIsVersion1WhenGitHubAssumed = function (req) { | |
| const _shouldUseDefaultService = function (req) { |
nit: I think this would be a slightly more accurate name
| afterEach(() => { | ||
| mockGetReviewFn.mockClear() | ||
| mockDeleteBranchFn.mockClear() | ||
| mockCreateFn.mockClear() | ||
| mockSetConfigPathFn.mockClear() | ||
| mockGetSiteConfigFn.mockClear() | ||
| mockProcessMergeFn.mockClear() | ||
| mockCreateHmacFn.mockClear() | ||
| }) |
There was a problem hiding this comment.
| afterEach(() => { | |
| mockGetReviewFn.mockClear() | |
| mockDeleteBranchFn.mockClear() | |
| mockCreateFn.mockClear() | |
| mockSetConfigPathFn.mockClear() | |
| mockGetSiteConfigFn.mockClear() | |
| mockProcessMergeFn.mockClear() | |
| mockCreateHmacFn.mockClear() | |
| }) | |
| afterEach(() => { | |
| jest.clearAllMocks() |
Would this work instead?
| }) | ||
|
|
||
| describe('Webhook controller', () => { | ||
| test.each([ |
There was a problem hiding this comment.
IMO we only need to use test.each when there's multiple cases. Here we only test 'gitfoo' and nothing else and I don't see us needing to add more cases here in the future. Same applies to the other cases in this file where we only pass one value to test.each
| if (service === 'github') { | ||
| req.headers['x-github-event'] = 'Some Other Hook' | ||
| } else if (service === 'gitlab') { | ||
| req.headers['x-gitlab-event'] = 'Some Other Hook' | ||
| } |
There was a problem hiding this comment.
| if (service === 'github') { | |
| req.headers['x-github-event'] = 'Some Other Hook' | |
| } else if (service === 'gitlab') { | |
| req.headers['x-gitlab-event'] = 'Some Other Hook' | |
| } | |
| req.headers[`x-${service}-event`] = 'Some Other Hook' |
Can use fstrings here
| const _calcIsMergeRequestAccepted = function (review) { | ||
| return (review.state === 'merged' || review.state === 'closed') | ||
| } | ||
|
|
||
| const _calcIsMergeRequestStaticmanGenerated = function (review) { | ||
| return (review.sourceBranch.indexOf('staticman_') > -1) | ||
| } | ||
|
|
||
| const _calcIsMergeRequestBranchRequiresManualDelete = function (service) { | ||
| return (service === 'github') | ||
| } |
There was a problem hiding this comment.
'calc' in these function names doesn't add any info. Consider these two functions:
_calcIsMergeRequestAccepted()_isMergeRequestAccepted()
We should prefer the shorter of the two since it conveys the same meaning.
There was a problem hiding this comment.
I disagree. When you prefix a function with "is", you are implying a simple getter with no logic involved. That is not the case here, hence the addition of the "calc". If it's important to you, though, I can change it.
| } | ||
| } | ||
|
|
||
| const _handleWebhookGitHub = async function (req, service, staticman) { |
There was a problem hiding this comment.
There's still a lot of nested branches here. Same applies to Gitlab handler function. Right now the function first reads the event type, then verifies the authenticity, then calls the webhook event handler. I think it would be better to break this up and change the order around. Here's some pseudocode of how I imagine this working:
const _handleWebhookGithHub() {
let isAuthenticated = _requiresAuthentication() ? authenticateRequest() : true;
if (!isAuthenticated) {
return Promise.reject('Unable to authenticate webhook request')
}
const event = req.headers['x-github-event']
switch(event) {
case 'pull-request':
_handleMergeRequest()
break;
default:
errorsRaised.push(`No handler found for webhook event ${event}`)
}
}| if (_calcIsMergeRequestStaticmanGenerated(review)) { | ||
| if (_calcIsMergeRequestAccepted(review)) { |
There was a problem hiding this comment.
| if (_calcIsMergeRequestStaticmanGenerated(review)) { | |
| if (_calcIsMergeRequestAccepted(review)) { | |
| if (_calcIsMergeRequestStaticmanGenerated(review) && _calcIsMergeRequestAccepted(review)) { |
Nesting isn't necessary here
| const version = params.version | ||
| const username = params.username | ||
| const repository = params.repository | ||
| const branch = params.branch | ||
|
|
||
| let gitService = null | ||
| let mergeReqNbr = null | ||
| if (service === 'github') { | ||
| gitService = await _buildGitHubService(version, service, username, repository, branch, data) | ||
| mergeReqNbr = data.number | ||
| } else if (service === 'gitlab') { | ||
| gitService = await gitFactory.create('gitlab', { | ||
| version: version, | ||
| username: username, | ||
| repository: repository, | ||
| branch: branch | ||
| }) | ||
| mergeReqNbr = data.object_attributes.iid | ||
| } else { | ||
| errors.push('Unable to determine service.') | ||
| return Promise.reject(errors) | ||
| } |
There was a problem hiding this comment.
| const version = params.version | |
| const username = params.username | |
| const repository = params.repository | |
| const branch = params.branch | |
| let gitService = null | |
| let mergeReqNbr = null | |
| if (service === 'github') { | |
| gitService = await _buildGitHubService(version, service, username, repository, branch, data) | |
| mergeReqNbr = data.number | |
| } else if (service === 'gitlab') { | |
| gitService = await gitFactory.create('gitlab', { | |
| version: version, | |
| username: username, | |
| repository: repository, | |
| branch: branch | |
| }) | |
| mergeReqNbr = data.object_attributes.iid | |
| } else { | |
| errors.push('Unable to determine service.') | |
| return Promise.reject(errors) | |
| } | |
| const version = params.version | |
| const username = params.username | |
| const repository = params.repository | |
| const branch = params.branch | |
| let gitService = await _buildGitService(version, service, username, repository, branch, data) | |
| let mergeReqNbr = getMergeRequestNumber(service); | |
| ------------------------------------------------------------------------------------------ | |
| const _buildGitService = function (version, service, username, repository, data) { | |
| // Required for v1 webhook backwards compatibility | |
| username = username ? username : data.repository.owner.login | |
| repository = repository ? repository : data.repository.name | |
| branch = branch ? branch : data.pull_request.base.ref | |
| const serviceConfig = { | |
| version, | |
| username, | |
| repository, | |
| branch | |
| } | |
| return await gitFactory.create(service, serviceConfig); | |
| } | |
| const _getMergeRequestNumber = function (service, data) { | |
| return service === 'github' ? data.number : data.object_attributes.iid | |
| } |
We can do something like above and make a helper function to abstract out the git service creation from webhook handler. This way we don't have to manually add cases to the conditional in the event another service is eventually supported.
| expect(res.status.mock.calls[0][0]).toBe(400) | ||
|
|
||
| // Restore console.error | ||
| consoleSpy.mockRestore(); |
There was a problem hiding this comment.
This should probably be done in a .finally() just in case an exception is thrown. Same applies to other mock restores done in .then() blocks
|
Hey, Alex. I don't necessarily disagree with any issues/concerns you've raised here. However, it's all starting to veer into the territory of pushing stylistic preferences, rather than correcting functional deficiencies. (Full disclosure, I've been guilty of this at one or two points in my career - seeing something implemented differently than I would have done it and classifying it as a deficiency.) This PR is just the first of 5 or 6 I'd like to see merged in. If you are going to want to proceed in a similar manner on all of them, I think I'm going to shift my focus to a fork rather than these contributions. To reiterate, I'm not in disagreement on most of your requested changes. They all seem to have merit. And I'm definitely a fan of writing great code. But I'm probably an even bigger fan of using "good enough" code. After all, my ultimate goal of these changes is to deploy them and make use of them. Maybe later down the line, I can circle back around and get them buttoned-up to the degree that you'd prefer. Please LMK your thoughts. Again, I applaud you for striving to set the bar high. Thank you for your efforts. |
|
@alexwaibel If you'd like to discuss offline, I can email you at the address on your web site. LMK. Thanks. |
I am certainly guilty of pushing personal style preferences to some extent, feel free to disagree with any review comments at any time and we can have a follow up discussion. I'd argue though that the readability of your code is equally as important as whether or not it works right now. The important thing is, will it work years from now and when it breaks can someone without context go in and quickly understand and fix the code. Code rots fast and it's hard to keep a project working over a long period of time. Once you've gotten one PR through on a project, the rest become a lot easier. You'll know what to look for and expect and be able to get your changes through much quicker with fewer PR revisions. You mentioned previously that this is your first large OSS contribution. I think you'll find that with OSS projects, maintainers have to be very picky about what they merge in. The hardest part of maintaining a project is turning down people's ideas but if we took every proposed change, the project would become unmaintainable.
I fully understand if this is the route you wish to go. It is much easier to make personal patches to your own fork where there's less people impacted if a change goes wrong and there is no review process. However, I'm always open to working with you and others to get your changes merged into this repo so they can be enjoyed by everyone using Staticman. We don't have metrics running on this project, and feedback in the form of issues can take some time to roll in, so often it's hard to tell when something goes wrong. With a change of this size (>1000 lines), there's a good chance we missed something. This is what I'm hoping to minimize with a thorough review process. Overall, I hope you decide to stick with this PR and the others you have lined up, but I understand if you wish to fork. |
|
@hispanic sounds good. If you decide not to pick this PR up again, please let me know so I can wrap up the changes myself. I just introduced a rather large PR #396 so depending on when you get to this you may encounter a lot of merge conflicts, especially since I changed the project structure. If you get stuck resolving those, lmk and I can take a look. |
|
I'll pick up wrapping this one up. Thanks again for your contribution @hispanic |
|
Roger that. Welcome, Alex. |
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:
#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:
#219
However, it looks like @eduardoboucas added in support for sending email notifications (via webhook) as far back as 2016:
#42 (comment)
Not sure why the miss, but these modifications address it, regardless.
The commit comments already explain the changes, but I'll summarize here, file-by-file:
server.js
webhook.js
handlePR.js
GitHub.js
JSON config
Thoughts
This is my first PR submission of any real substance to an open source project. Please forgive any missteps.
Thank you for Staticman!