Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework GitHub acceptor and move to its own module #2021

Merged
merged 14 commits into from
Nov 9, 2018
Merged

Rework GitHub acceptor and move to its own module #2021

merged 14 commits into from
Nov 9, 2018

Conversation

paulmelnikow
Copy link
Member

Continuing to merge the work from #1205.

@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Aug 29, 2018
@paulmelnikow paulmelnikow changed the title [WIP] Move GitHub acceptor to own module [WIP] Rework [GitHub] acceptor and move its to own module Aug 29, 2018
@shields-ci
Copy link

shields-ci commented Aug 29, 2018

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 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
@paulmelnikow
Copy link
Member Author

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
const serverSecrets = require('../../../lib/server-secrets')
const secretIsValid = require('../../../lib/sys/secret-is-valid')

function sendTokenToAllServers(token) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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",
Copy link
Member Author

@paulmelnikow paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

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

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) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test.

end('')
})

server.route(/^\/github-auth\/done$/, (data, match, end, ask) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test.

@paulmelnikow 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 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
PyvesB previously approved these changes Nov 8, 2018
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

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

@paulmelnikow
Copy link
Member Author

paulmelnikow commented 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 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
@PyvesB
Copy link
Member

PyvesB commented Nov 8, 2018

Is such a big diff in the lock file expected?

@paulmelnikow
Copy link
Member Author

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
Copy link
Member

chris48s commented Nov 9, 2018

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.

@paulmelnikow paulmelnikow merged commit 3eac8eb into badges:master Nov 9, 2018
@shields-deployment
Copy link

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:

@paulmelnikow paulmelnikow deleted the github-acceptor branch November 9, 2018 20:14
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
Labels
core Server, BaseService, GitHub auth, Shared helpers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants