Use probot-metadata for storing snooze config #19
Conversation
lib/freeze.js
Outdated
| })); | ||
| } | ||
|
|
||
| unfreeze(issue, props) { | ||
| async checkUnfreeze(issue) { | ||
| const kv = metadata(this.github, issue, process.env.APP_ID); |
There was a problem hiding this comment.
should metadata take context as a param so that it gets both .github and the app_id in one-fell swoop? (I assume context knows of app_id because it created .github...)
There was a problem hiding this comment.
Good point. I really like the the API being something like:
metadata(context).get(key)context doesn't actually know about APP_ID since it is handed the github client, but…every payload includes an installation.id, which can be used for namespacing.
lib/freeze.js
Outdated
| unfreeze(issue, props) { | ||
| async checkUnfreeze(issue) { | ||
| const kv = metadata(this.github, issue, process.env.APP_ID); | ||
| const props = await kv.get('snooze'); |
There was a problem hiding this comment.
should the APP_ID that we pass in above do the equivalent of this for us? would snooze ever have access to any values that weren't snooze?
There was a problem hiding this comment.
The APP_ID just acts as a namespace to ensure apps won't override data for eachother. At the moment, get requires a key parameter, but I like the idea of modifying it to support being called without a key and just return all values.
lib/freeze.js
Outdated
| const props = await kv.get('snooze'); | ||
|
|
||
| if (moment(props.unfreezeMoment) < moment()) { | ||
| return this.unfreeze(issue, props); |
There was a problem hiding this comment.
checkUnfreeze to me (YMMV, interested in your thoughts) implies a method that returns a result of the check. Does unfreezing here make this mutating? should this just return true/false, and we let the calling function call unfreeze?
There was a problem hiding this comment.
Yeah, the reason I moved this all inline was so the prop parsing could all be hidden. Otherwise, the caller of freeze needs to have access to the props. I can experiment with cleaning this up.
There was a problem hiding this comment.
That makes sense, and is cleaner to not deal with passing props around. Maybe rename it then? Check seems to he the wrong verb now. Just unfreeze sounds overkill, but I think it's better.
| state: 'open', | ||
| labels | ||
| })); | ||
| const {owner, repo, number} = issue; |
There was a problem hiding this comment.
what sort of black magic is this?!
There was a problem hiding this comment.
| await Promise.all(resp.data.items.map(issue => { | ||
| // Issue objects from the API don't include owner/repo params, so | ||
| // setting them here with `context.repo` so we don't have to worry | ||
| // about it later. :/ |
There was a problem hiding this comment.
isn't that weird? That's why i wrote the url parser to start with. Not sure if this is good/bad/something that we should advocate for upstream, but seems odd to me.
| @@ -35,29 +37,19 @@ perform: true | |||
| }})) | |||
| }, | |||
| issues: { | |||
| getComments: expect.createSpy().andReturn(Promise.resolve( | |||
There was a problem hiding this comment.
so metadata takes care of this, but we don't need to mock it?
There was a problem hiding this comment.
metadata actually stores all the data on the original issue body. This is mostly for performance reasons so you don't have to fetch all the comments for each issue to find data.
There was a problem hiding this comment.
Ah, so pulling the issue pulls the OP comment, which gets you the metadata. That's.... Awesome.
|
how do we handle older issues that may have had the previous format? I guess none really exist, but just checking. |
If there's not anyone using this in production, I wouldn't worry about it. If there was, we'd have to add conditional logic for the old format. |
|
@jbjonesjr I think this is ready. Let me know if you have any other feedback. |
This is an update to probot/reminders#1, which is a spike to use the new probot-metadata to store the snooze config for each issue.