Rework GitHub acceptor and move to its own module #2021

paulmelnikow · 2018-08-29T22:31:36Z

Continuing to merge the work from #1205.

shields-ci · 2018-08-29T22:33:24Z

	Warnings
⚠️	This PR modified helper functions in `lib/` but not accompanying tests. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

paulmelnikow · 2018-10-27T17:39:13Z

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 · 2018-10-27T18:14:08Z

services/github/auth/acceptor.js

+const serverSecrets = require('../../../lib/server-secrets')
+const secretIsValid = require('../../../lib/sys/secret-is-valid')
+
+function sendTokenToAllServers(token) {


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.

paulmelnikow · 2018-10-27T18:14:32Z

services/github/auth/acceptor.js

+    })
+  })
+
+  server.route(/^\/github-auth\/add-token$/, (data, match, end, ask) => {


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.

paulmelnikow · 2018-10-27T19:35:46Z

package.json

@@ -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",


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.

paulmelnikow · 2018-10-27T20:15:43Z

services/github/auth/acceptor.js

+function setRoutes(server) {
+  const baseUrl = process.env.BASE_URL || 'https://img.shields.io'
+
+  server.route(/^\/github-auth$/, (data, match, end, ask) => {


Added a test.

paulmelnikow · 2018-10-27T20:15:47Z

services/github/auth/acceptor.js

+    end('')
+  })
+
+  server.route(/^\/github-auth\/done$/, (data, match, end, ask) => {


Added a test.

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

Looks good to me! Should package-lock.json be updated as well?

paulmelnikow · 2018-11-08T19:13:51Z

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.

PyvesB · 2018-11-08T19:28:18Z

Is such a big diff in the lock file expected?

paulmelnikow · 2018-11-08T19:38:52Z

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.

chris48s · 2018-11-09T19:19:16Z

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.

shields-deployment · 2018-11-09T20:14:04Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

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.

paulmelnikow added 2 commits August 29, 2018 18:27

First pass: move github acceptor to own module

2673372

Helpers first

dca054c

paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Aug 29, 2018

paulmelnikow changed the title ~~[WIP] Move GitHub acceptor to own module~~ [WIP] Rework [GitHub] acceptor and move its to own module Aug 29, 2018

paulmelnikow changed the title ~~[WIP] Rework [GitHub] acceptor and move its to own module~~ [WIP] Rework [GitHub] acceptor and move to its own module Aug 29, 2018

Merge branch 'master' into github-acceptor

22a5e36

paulmelnikow added 2 commits October 27, 2018 15:20

Add a test for the OAuth start endpoint

35845c9

Can’t use `fetch` because there isn’t a way not to follow redirects

Test OAuth done route

edd7157

paulmelnikow commented Oct 27, 2018

View reviewed changes

Tests should pass when server secrets are undefined

b0e37f8

paulmelnikow changed the title ~~[WIP] Rework [GitHub] acceptor and move to its own module~~ Rework [GitHub] acceptor and move to its own module Oct 27, 2018

paulmelnikow mentioned this pull request Oct 27, 2018

Add some error messages for the developer when .service.js is malformed #1894

Merged

paulmelnikow mentioned this pull request Nov 1, 2018

Start on the Github rewrite, with [GithubPullRequestCheckState] #2253

Merged

paulmelnikow added 2 commits November 4, 2018 18:37

Merge branch 'master' into github-acceptor

4925255

# Conflicts: # lib/github-auth.js # package-lock.json

lint + prettier

1769b38

PyvesB previously approved these changes Nov 8, 2018

View reviewed changes

paulmelnikow changed the title ~~Rework [GitHub] acceptor and move to its own module~~ Rework GitHub acceptor and move to its own module Nov 8, 2018

paulmelnikow added 2 commits November 8, 2018 14:17

Merge branch 'master' into github-acceptor

71f7ec4

Commit package-lock changes

d67f73e

paulmelnikow dismissed PyvesB’s stale review via d67f73e November 8, 2018 19:17

Merge branch 'master' into github-acceptor

b6e01ca

paulmelnikow added 2 commits November 9, 2018 14:48

Merge branch 'master' into github-acceptor

bde1f6e

Update package-lock

4797e9c

chris48s approved these changes Nov 9, 2018

View reviewed changes

Merge branch 'master' into github-acceptor

45afa15

paulmelnikow merged commit 3eac8eb into badges:master Nov 9, 2018

paulmelnikow deleted the github-acceptor branch November 9, 2018 20:14

paulmelnikow mentioned this pull request Jan 11, 2019

Cleaning up the GitHub auth #2733

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework GitHub acceptor and move to its own module #2021

Rework GitHub acceptor and move to its own module #2021

paulmelnikow commented Aug 29, 2018

shields-ci commented Aug 29, 2018 •

edited

Loading

paulmelnikow commented Oct 27, 2018

paulmelnikow Oct 27, 2018

paulmelnikow Oct 27, 2018

paulmelnikow Oct 27, 2018 •

edited

Loading

paulmelnikow Oct 27, 2018

paulmelnikow Oct 27, 2018

PyvesB left a comment

paulmelnikow commented Nov 8, 2018 •

edited

Loading

PyvesB commented Nov 8, 2018

paulmelnikow commented Nov 8, 2018

chris48s commented Nov 9, 2018

shields-deployment bot commented Nov 9, 2018

Rework GitHub acceptor and move to its own module #2021

Rework GitHub acceptor and move to its own module #2021

Conversation

paulmelnikow commented Aug 29, 2018

shields-ci commented Aug 29, 2018 • edited Loading

paulmelnikow commented Oct 27, 2018

paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

paulmelnikow Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

PyvesB left a comment

Choose a reason for hiding this comment

paulmelnikow commented Nov 8, 2018 • edited Loading

PyvesB commented Nov 8, 2018

paulmelnikow commented Nov 8, 2018

chris48s commented Nov 9, 2018

shields-deployment bot commented Nov 9, 2018

shields-ci commented Aug 29, 2018 •

edited

Loading

paulmelnikow Oct 27, 2018 •

edited

Loading

paulmelnikow commented Nov 8, 2018 •

edited

Loading