Introduce 'interval' to control the scheduler interval #167
Conversation
There was a problem hiding this comment.
Thanks for the PR @dwijnand! No worries about the newbness, we've all been there!
The biggest weirdness about this change is that it happens outside the scope of lib/stale.js which has a method for combining the config with the defaults nicely using the lib/schema.js
I'd also love to see some tests that demonstrate this functionality, but not sure the best way to go about it. @bkeepers might have an idea?
lib/schema.js
Outdated
|
|
||
| interval: Joi.number().integer().min(1).max(24) | ||
| .error(() => '"interval" must be an integer between 1 and 24') | ||
| .description('The number of hours to schedule a run, 1-24. Default is 1') |
There was a problem hiding this comment.
Right now this isn't actually used.
README.md
Outdated
| @@ -56,6 +56,9 @@ markComment: > | |||
| # Limit the number of actions per hour, from 1-30. Default is 30 | |||
| limitPerRun: 30 | |||
|
|
|||
| # The number of hours to schedule a run, 1-24. Default is 1 | |||
There was a problem hiding this comment.
Maybe # The number of hours per day to schedule a run, 1-24. Default is 1 is slightly more clear?
|
Thanks for the speedy review, @hiimbex! Hopefully we can figure out how to test this and verify it works, meanwhile I've pushed changes for the easy parts. |
|
Any thoughts, @bkeepers? I'd love to be able to tweak this behaviour in stale bot! |
|
@hiimbex How do you feel about accepting this as is, and forward-fixing any issues if they crop up? I'd be happy to submit a follow-up PR with a test for this, once we have some idea how to test it. |
|
@dwijnand I still this needs updating since as I mentioned, the schema isn't actually used in the index.js, only from stale, so the changes to the schema and it's test should be removed since they're never used. |
|
Done! Despite it not being used, I thought it would be good to have for consistency. But I don't mind dropping it. |
| let interval = 1 | ||
| if (config && config.interval) { | ||
| interval = config.interval | ||
| } |
There was a problem hiding this comment.
There would need to be some validations for the values of config.interval here, otherwise I could set a value of 0.00001 and it would trigger this every 3 milliseconds, which would likely cause some problems.
| // Visit all repositories to mark and sweep stale issues | ||
| let scheduler = createScheduler(app, { | ||
| interval: interval * 60 * 60 * 1000 | ||
| }) |
There was a problem hiding this comment.
Schedulers currently scan all repositories for all installations. So this would create a duplicate scheduler for every single repository, but that scheduler is not isolated to the single repository.
|
Damn... that makes this more complicated. 😞 |
Fixes #165
Hey! So I'm extremely noob here, but this seems to be passing
npm testfor me locally.. Is it right? Help very welcome!View rendered README.md