Rework GitHub acceptor and move to its own module #2021
Merged
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
paulmelnikow
changed the title
Generated by 🚫 dangerJS |
paulmelnikow
changed the title
|
This really should have a test. I'm eager to get it in. |
Can’t use `fetch` because there isn’t a way not to follow redirects
paulmelnikow
commented
Oct 27, 2018
| const serverSecrets = require('../../../lib/server-secrets') | ||
| const secretIsValid = require('../../../lib/sys/secret-is-valid') | ||
|
|
||
| function sendTokenToAllServers(token) { |
There was a problem hiding this comment.
This code is difficult to test, and enabling #1939 should obviate the need for it, so I don't think it's worth writing a new test.
| }) | ||
| }) | ||
|
|
||
| server.route(/^\/github-auth\/add-token$/, (data, match, end, ask) => { |
There was a problem hiding this comment.
This endpoint is obviated when #1939 is turned on; so I'm not going to take the time to write a new test for it.
| @@ -150,6 +150,7 @@ | |||
| "eslint-plugin-standard": "^4.0.0", | |||
| "fetch-ponyfill": "^6.0.0", | |||
| "fs-readfile-promise": "^3.0.1", | |||
| "got": "^9.2.2", | |||
There was a problem hiding this comment.
You can't use whatwg-fetch to test redirects, because they can't be disabled.
I don't have a strong preference between axios or got to supplant request and fetch. I had an easier time with got's documentation, so went with that for the moment. So far this is only used in test code.
| function setRoutes(server) { | ||
| const baseUrl = process.env.BASE_URL || 'https://img.shields.io' | ||
|
|
||
| server.route(/^\/github-auth$/, (data, match, end, ask) => { |
| end('') | ||
| }) | ||
|
|
||
| server.route(/^\/github-auth\/done$/, (data, match, end, ask) => { |
paulmelnikow
changed the title
paulmelnikow
added a commit
that referenced
this pull request
Nov 1, 2018
The GitHub service family is the largest, and as yet untouched by our service rewrite. I thought I would start the process by tackling one service. This pull request has a few things going on: 1. Rename pull-request-status to pull-request-check-state. We have another badge called pull request status. It seems like the checks are called one thing in the UI and another thing in the API, which is unfortunate. If other folks have strong feelings about the name, I’ll defer. 2. Move its tests and tighten up the syntax. 3. Move its badge examples including the doc string. 4. Add a new helper `errorMessagesFor` to use in the new services in place of `githubCheckErrorResponse`. It seems like we didn’t really use the `errorMessages` parameter to `githubCheckErrorResponse`, so I pared this down. I’m not sure if this is the function we’ll ultimately want, but it seems like a good place to start. 5. Pulled fetch code I _know_ we use in other places into `github-common-fetch`. As in the PR I just opened for azure-devops, this takes a functional approach to the shared code, which is more direct, nimble, and easy to reason about than inheritance. 6. Created `GithubAuthService` which functions identically to BaseJsonService, except for one thing, which is that it uses the token pool. I accomplished this by adding a `_requestFetcher` property to BaseService, which is initialized to `sendAndCacheRequest` in the constructor, and can be overridden in subclasses. Since we weren’t using `_sendAndCacheRequest` directly except in BaseService and tests, I removed that property. I like this approach to patching in the GitHub auth because it’s very simple and creates no new API exposure. However, the way we’re doing the dependency injection feels a bit odd. Maybe the eventual refactor of request-handler would be a godo time to revisit this. 7. The GitHub requests go through many, many layers of indirection at this point. Later on it would be good to shave some of these off, perhaps once the legacy GitHub services have been converted, or when all the services are done and we can take another look at the base service hierarchy. The work in #2021 and #1205 is also related.
# Conflicts: # lib/github-auth.js # package-lock.json
PyvesB
previously approved these changes
Nov 8, 2018
|
Thanks for reviewing! Yea, not sure why package-lock didn't get updated. I will do that now. Service test failures are unrelated (not sure the service even calls the code that's in flight here) so I'm going to turn them off. |
paulmelnikow
changed the title
|
Is such a big diff in the lock file expected? |
|
Yea, it's a consistent problem with dependabot. The churn occurs every time a package-lock is written by the npm CLI after it's been written by dependabot, or vice versa. I'm not sure we've reported it. It sure would be nice to fix because it makes it basically impossible to review the changes in that file. |
|
The lockfile optional thing is a reported issue on dependabot dependabot/feedback#197 but required an upstream fix: npm/cli#76 As I understand it, once we (and dependabot) are updated to a version on npm with that patch, this should stop happening. |
chris48s
approved these changes
Nov 9, 2018
paulmelnikow
added a commit
that referenced
this pull request
Nov 9, 2018
The GitHub service family is the largest, and as yet untouched by our service rewrite. I thought I would start the process by tackling one service. This pull request has a few things going on: 1. Rename pull-request-status to pull-request-check-state. We have another badge called pull request status. It seems like the checks are called one thing in the UI and another thing in the API, which is unfortunate. If other folks have strong feelings about the name, I’ll defer. 2. Move its tests and tighten up the syntax. 3. Move its badge examples including the doc string. 4. Add a new helper `errorMessagesFor` to use in the new services in place of `githubCheckErrorResponse`. It seems like we didn’t really use the `errorMessages` parameter to `githubCheckErrorResponse`, so I pared this down. I’m not sure if this is the function we’ll ultimately want, but it seems like a good place to start. 5. Pull fetch code I _know_ we use in other places into `github-common-fetch`. As in the PR I just opened for azure-devops, this takes a functional approach to the shared code, which is more direct, nimble, and easy to reason about than inheritance. 6. Create `GithubAuthService` which functions identically to BaseJsonService, except for one thing, which is that it uses the token pool. I accomplished this by adding a `_requestFetcher` property to BaseService, which is initialized to `sendAndCacheRequest` in the constructor, and can be overridden in subclasses. Since we weren’t using `_sendAndCacheRequest` directly except in BaseService and tests, I removed that property. I like this approach to patching in the GitHub auth because it’s very simple and creates no new API exposure. However, the way we’re doing the dependency injection feels a bit odd. Maybe the eventual refactor of request-handler would be a godo time to revisit this. The GitHub requests go through many, many layers of indirection at this point. Later on it would be good to shave some of these off, perhaps once the legacy GitHub services have been converted, or when all the services are done and we can take another look at the base service hierarchy. The work in #2021 and #1205 is also related.
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.
Continuing to merge the work from #1205.