Refactor [github] token persistence, again #1906
Conversation
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.
Generated by 🚫 dangerJS |
lib/token-persistence.spec.js
Outdated
| @@ -54,8 +44,8 @@ describe('Token persistence', function() { | |||
| }) | |||
| }) | |||
|
|
|||
| context('when shutting down', function() { | |||
| it('writes added tokens to the file', async function() { | |||
| context('when tokens are added', function() { | |||
There was a problem hiding this comment.
Shall we add a test or two for when tokens are removed?
| @@ -110,6 +113,7 @@ function setRoutes(server) { | |||
| }, 10000) | |||
| } | |||
| addGithubToken(data.token) | |||
| emitter.emit('token-added', data.token) | |||
There was a problem hiding this comment.
The listeners aren't actually using the token that is being passed around (as they call getAllTokenIds to get the whole list), but it probably doesn't hurt either to include it in the emitted event.
There was a problem hiding this comment.
Yea… and the next iteration of this, with a redis backend, will use that. I believe Redis can do inserts and deletes instead of rewriting the whole thing.
|
@PyvesB Can I get your 👍 on this? |
|
@paulmelnikow well I believe there's still that pending comment about the token removal test. But if you no longer think it's necessary, I'm happy to sign this off. 😉 |
|
Ah, right! Sorry, I missed that when I was reading it just now. |
lib/token-persistence.spec.js
Outdated
| clock.restore() | ||
| await sleep(200) | ||
| githubAuth.rmGithubToken(toRemove) | ||
| await persistence.noteTokenAdded(toRemove) |
There was a problem hiding this comment.
Shouldn't this be noteTokenRemoved?
There was a problem hiding this comment.
Ah thanks, I caught that in #1939 but forgot to fix it here.
Instead of saving tokens on a timer, save them when they change. 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 replaces json-autosave, which was a bit difficult to read and also hard to test, with fsos, the lower-level utility it’s built on.
The most important goal is to provide a different token-persistence backend and I don't want to get too sidetracked by the rest of the slog that is #1205. There are no tests that everything is wired together correctly, nor any that cover the token acceptor, so I suppose I could either add a relatively wide-bracket functional test here, or test this manually (at least for the moment).
Ref: #1848
Todo: