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

Optionally persist [Github] tokens in Redis #1939

Merged
merged 22 commits into from
Aug 19, 2018
Merged

Optionally persist [Github] tokens in Redis #1939

merged 22 commits into from
Aug 19, 2018

Conversation

paulmelnikow
Copy link
Member

This is a fairly simple addition of a Redis-backed TokenPersistence. When GithubConstellation is initialized, it will create a FsTokenPersistence or a RedisTokenPersistence based on configuration. Have added tests of the Redis backend as an integration test, and ensured the server starts up correctly when a REDIS_URL is configured.

This is based off #1906 and the diff will clean up when that lands. Tests are failing due to #1938.

Ref: #1848

paulmelnikow and others added 12 commits August 12, 2018 11:49
Save the tokens things change instead of on a timer. Use EventEmitter to keep the components loosely coupled.

This is easier to reason about, much easier to test, and better supports adapting to backends which support atomic operations.

This switches from json-autosave, which was a bit difficult to read and also hard to test, to fsos, the lower-level utility it’s built on.
@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Aug 17, 2018
@paulmelnikow paulmelnikow requested a review from PyvesB August 17, 2018 22:58
@PyvesB
Copy link
Member

PyvesB commented Aug 18, 2018

I will review this when #1906 is merged. 😉

@shields-ci
Copy link

shields-ci commented Aug 18, 2018

Warnings
⚠️

This PR added helper modules in lib/ but not accompanying tests. Generally helper modules should have their own tests.

Messages
📖

✨ Thanks for your contribution to Shields, @paulmelnikow!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

# Conflicts:
#	lib/token-persistence.js
#	lib/token-persistence.spec.js
#	services/github/github-constellation.js
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 overall good to me, well done! 👍

CONTRIBUTING.md Outdated
@@ -130,6 +130,10 @@ e.g. **[Travis] Fix timeout issues**

When changing other code, please add unit tests.

To run the integration tests, you must have redis installed and in your PATH.
Use `brew install redis`, `yum install redis`, etc. Ths test runner will
Copy link
Member

Choose a reason for hiding this comment

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

Small typo: "Ths".

})
}

beforeEach(function() {})
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this should be deleted? 😄

@paulmelnikow
Copy link
Member Author

Thanks for the review, and all the reviews on this track! We’ve reached the end, for the moment. There’s a bit of unmerged work left in #1205, but this closes out the Github part of #1848.

@paulmelnikow paulmelnikow merged commit a16d436 into badges:master Aug 19, 2018
@shields-deployment
Copy link

This pull request was merged to master branch. Now this change is waiting for deployment.
Deploys usually happen every few weeks. After deployment changes are copied to gh-pages branch.

This badge displays deployment status:

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.

3 participants