- Notifications
You must be signed in to change notification settings - Fork928
fix: FE parsing of schedule with day strings#2006
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Resolves:#1901Summary:We had a homegrown parser that only understood numbers, not strings likeMON or TUES. We replace the homegrown parser with cron-parser.Details:This was nearly a straight drop-in.Impact:Much less code/maintenance burden :DWhat I learned:Don't trust the README, sometimes you just gotta read the code or importit and try it out. The `fields` representation of the parsed expressionwas missing from their docs. I might open an issue or PR to update them!
| @@ -93,7 +94,7 @@ export const formValuesToTTLRequest = (values: WorkspaceScheduleFormValues): Typ | |||
| export const workspaceToInitialValues = (workspace: TypesGen.Workspace): WorkspaceScheduleFormValues => { | |||
| const schedule = workspace.autostart_schedule | |||
| constttl = workspace.ttl_ms ? workspace.ttl_ms / (1000 * 60 * 60) : 0 | |||
| constttlHours = workspace.ttl_ms ?Math.round(workspace.ttl_ms / (1000 * 60 * 60)) : 0 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
REVIEW: This is technically a separate change/uncaught previously.
Basically if you set your ttl via CLI to something like 1.5 hours, we'd show the 1.5 initially in the form, but you were forced to change it to an integer.
This feels safer until we have a form that allows minutes in addition to hours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BE = 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this feels good!
Resolves:#1901Summary:We had a homegrown parser that only understood numbers, not strings likeMON or TUES. We replace the homegrown parser with cron-parser.Details:This was nearly a straight drop-in.Impact:Much less code/maintenance burden :DWhat I learned:Don't trust the README, sometimes you just gotta read the code or importit and try it out. The `fields` representation of the parsed expressionwas missing from their docs. I might open an issue or PR to update them!
Uh oh!
There was an error while loading.Please reload this page.
Resolves:#1901
Summary:
We had a homegrown parser that only understood numbers, not strings like
MON or TUES. We replace the homegrown parser with cron-parser.
Details:
This was nearly a straight drop-in.
Impact:
Much less code/maintenance burden :D
What I learned:
Don't trust the README, sometimes you just gotta read the code or import
it and try it out. The
fieldsrepresentation of the parsed expressionwas missing from their docs. I might open an issue or PR to update them!