Uh oh!
There was an error while loading. Please reload this page.
WIP: Scheduler Stage 3 (Cron support) - #487
Conversation
| import {Row} from '../../layout.jsx'; | ||
| // react-select requires options to have `label` and `value` keys |
There was a problem hiding this comment.
thx a lot for adding this kind of comment!
| onChange={this.handleIntervalChange} | ||
| /> | ||
| </div> | ||
| <Row style={Object.assign({}, rowStyleOverride, { marginTop: '8px', borderTop: '1px solid #c8d4e3', paddingTop: '24px' })}> |
There was a problem hiding this comment.
linter fails here with "maximum line kength exceeded"
| /* eslint-disable no-multi-str */ | ||
| import {concat} from 'ramda'; | ||
| // Default to 1 week for `refreshInterval` for backwards compatability |
There was a problem hiding this comment.
Oh, I've just got it. This is to prevent old versions of falcon from running new scheduled queries as setInterval(runQuery, NaN). Could you add something like that to comment, please?
There was a problem hiding this comment.
I believe this is to address my concerns about what happens if an non-cron-supporting version of Falcon is started in an environment where there are persisted queries that have been created with a cronInterval.
n-riesco
left a comment
There was a problem hiding this comment.
Looks very good to me. Thanks for splitting the code into small files, easy to read!
I'm sure the styles are still work in progress. Here's a screen capture with issues I noticed:
| /* eslint-disable no-multi-str */ | ||
| import {concat} from 'ramda'; | ||
| // Default to 1 week for `refreshInterval` for backwards compatability |
There was a problem hiding this comment.
Oh, I've just got it. This is to prevent old versions of falcon from running new scheduled queries as setInterval(runQuery, NaN). Could you add something like that to comment, please?
| onChange={this.updateRefreshInterval} | ||
| /> | ||
| <div style={{width: '65%', minHeight: '108px'}}> | ||
| <CronPicker onChange={this.handleIntervalChange} /> |
There was a problem hiding this comment.
we should set initialModeId here (and be smart if the scheduled query uses the old format).
| @@ -206,34 +207,38 @@ export class PreviewModal extends Component { | |||
| </Row> | |||
| )} | |||
| {this.props.loggedIn && ( | |||
There was a problem hiding this comment.
how about we do something like this and show a LogIn button if needed?
{this.props.loggedIn ? (...edit/delete...) : (...login..)}Fix linting from Prettier get initial cron mode id from query Add login button to preview modal Move getInitialCronMode to cron-helpers Fix linting from Prettier get initial cron mode id from query Add login button to preview modal Move getInitialCronMode to cron-helpers
| return 'MONTHLY'; | ||
| } | ||
| } else if (refreshInterval) { | ||
| if (refreshInterval <= 60) { |
There was a problem hiding this comment.
refreshInterval <= 60 * (1 + 5) /2
| } else if (refreshInterval) { | ||
| if (refreshInterval <= 60) { | ||
| return 'MINUTE'; | ||
| } else if (refreshInterval <= 5 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 60 * (5 + 60) /2
| return 'MINUTE'; | ||
| } else if (refreshInterval <= 5 * 60) { | ||
| return 'FREQUENTLY'; | ||
| } else if (refreshInterval <= 60 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 60 * 60 * (1 + 24) /2
| return 'FREQUENTLY'; | ||
| } else if (refreshInterval <= 60 * 60) { | ||
| return 'HOURLY'; | ||
| } else if (refreshInterval <= 24 * 60 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 24 * 60 * 60 * (1 + 7) /2
| return 'HOURLY'; | ||
| } else if (refreshInterval <= 24 * 60 * 60) { | ||
| return 'DAILY'; | ||
| } else if (refreshInterval <= 7 * 24 * 60 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 24 * 60 * 60 * (7 + 30) /2
| return 'DAILY'; | ||
| } else if (refreshInterval <= 7 * 24 * 60 * 60) { | ||
| return 'WEEKLY'; | ||
| } else if (refreshInterval <= 30 * 24 * 60 * 60) { |
There was a problem hiding this comment.
The maximum refresh interval is 2147483647/1000 (see MDN)
mfix22
commented
Jul 11, 2018
|
Closing in favor of local branch at #488
This is a WIP PR that adds
cronIntervalsupport to the frontend 👍We will shortly add the backend support and merge it in with this branch. We just wanted to get this up for @n-riesco to review before doing so.
TODO: