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

Support GitHub access tokens and cache API results #337

Merged
merged 2 commits into from
Jul 5, 2017

Conversation

toolness
Copy link
Contributor

@toolness toolness commented Jul 3, 2017

This fixes #272 by authenticating with GitHub if the GITHUB_ACCESS_TOKEN environment variable is set.

Notes

  • jekyll_get no longer fails silently; it now at least logs an error message.
  • It turned out that the fetching of the release notes was throwing an exception, because it was trying to decode the content key which didn't exist. I added another configuration parameter to each source called decode_content which specifies whether to attempt this decoding or not.
  • We now follow the lead of Load cached files rather than remote json 18F/jekyll-get#16 and always use cached data if it's available. Unlike that PR, we also always cache our data, because there doesn't seem any reason not to (and making it optional just introduces more branching logic which complicates the code).

To do (optional)

If we merge this PR, the following should either be fixed now or filed as issues to be addressed later.

  • I'd really like to add at least a few unit tests to this, at least for the stuff that's easy to test, but the unit test framework was added in Fix relative links in external markdown #336, which hasn't been merged yet. Filed as Add some unit tests for jekyll_get #346.
  • I think we should incorporate Load cached files rather than remote json 18F/jekyll-get#16 and load the cached JSON data if it exists. Aside from lowering the barrier to entry for developers by not making it quite so necessary for them to set GITHUB_ACCESS_TOKEN, it will also speed up incremental builds by around 0.5 seconds. We should also document it in the readme so developers don't get confused if they notice stale data on their local server, and so they know how to clear the cache if needed.

@toolness toolness changed the title Support GitHub access tokens Support GitHub access tokens and cache API results Jul 5, 2017
@donjo donjo merged commit 6fae3e6 into develop Jul 5, 2017
@donjo donjo deleted the github-access-token branch July 5, 2017 14:59
This was referenced Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants