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

When a global gh_token is configured, always use it #1118

Merged
merged 7 commits into from
Oct 30, 2017
Merged

When a global gh_token is configured, always use it #1118

merged 7 commits into from
Oct 30, 2017

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Oct 2, 2017

I ran into some unexpected behavior locally which we're also seeing in CI. The Github tests are flaky, and they sometimes pass or fail depending on the order in which they are run.

There are two issues here.

First, our token logic doesn't correctly distinguish between the standard 12,500 requests per hour, and the 30 requests per minute provided by the Search API. Once a token is used for a search request, the server thinks it's exhausted and stops using it. This should be fixed and tested, and while I'm glad the issue came up here, I would argue the place to test it would not be the service tests; rather some unit tests or integration tests of the github-auth logic. This issue is independent of #979. I opened #1119 to discuss.

Second, if I configure a global gh_token, I expect it to be used all the time. I expect to see predictable failures when that token is exhausted.

This pull request addresses the second issue. It forces the global gh_token to be used whenever one is provided.

@paulmelnikow paulmelnikow added developer-experience Dev tooling, test framework, and CI self-hosting Discussion, problems, features, and documentation related to self-hosting Shields labels Oct 2, 2017
@paulmelnikow
Copy link
Member Author

I'm waiting to merge this until I receive confirmation there is no gh_token set in production.

# Conflicts:
#	lib/github-auth.js
@paulmelnikow paulmelnikow added the operations Hosting, monitoring, and reliability for the production badge servers label Oct 21, 2017
@paulmelnikow
Copy link
Member Author

When I run github tests I need to check out this file, so I've committed it by mistake several times now. It's also blocking other pull requests, and the likelihood of merge conflicts makes me reluctant to start work on #1119. I've solved merge conflicts in it twice. I really would like to get this merged.

At the same time I want master to be deployable, so really need to know if there is a gh_token configured on the server before merging.

@espadrine could you check please?

# Conflicts:
#	lib/github-auth.js
@paulmelnikow
Copy link
Member Author

This has been superseded by #1205. If I'm to self-review that I need a few days away from the code, and don't want to hold this up in the meantime. So, if I can get word this is safe to merge I will merge it.

@paulmelnikow
Copy link
Member Author

Shields production servers do not use gh_token. I added a note to the self-hosting docs to alert those users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI operations Hosting, monitoring, and reliability for the production badge servers self-hosting Discussion, problems, features, and documentation related to self-hosting Shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant