Skip to content

Bug: Workspace Schedule: Days of week are not defaulted to settings defined at creation time via cli (new workspace) #1901

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Tracked by #1317
nadzeyav opened this issue May 30, 2022 · 3 comments · Fixed by #2006
Closed
Tracked by #1317

Bug: Workspace Schedule: Days of week are not defaulted to settings defined at creation time via cli (new workspace) #1901

nadzeyav opened this issue May 30, 2022 · 3 comments · Fixed by #2006
Assignees
@greyscaled
Labels
site Area: frontend dashboard
Milestone
Community MVP

Comments

@nadzeyav
Copy link

OS Information

Steps to Reproduce

  1. Create a workspace via cli: coder create [workspace-name] --template="[your template name]" --autostart-day-of-week MON-TUE --autostart-hour 5
  2. Login to Web UI
  3. Open created workspace -> schedule is displayed as specified at creation time
  4. Click Edit schedule

Expected

Monday and Tuesday checkboxes are checked

Actual

All week day checkboxes are unchecked

Logs

Screenshot

week-days.mp4

Notes
@nadzeyav nadzeyav changed the title Bug: Frontend: Workspace Schedule: For a new workpsace days of week are not defaulted to settings defined at creatiom time via cli Bug: Frontend: Workspace Schedule: For a new workspace days of week are not defaulted to settings defined at creatiom time via cli May 30, 2022
@ketang ketang changed the title Bug: Frontend: Workspace Schedule: For a new workspace days of week are not defaulted to settings defined at creatiom time via cli Bug: Frontend: Workspace Schedule: For a new workspace days of week are not defaulted to settings defined at creation time via cli May 30, 2022
@greyscaled
Copy link
Contributor

greyscaled commented May 31, 2022 •
edited
Loading

The issue here is that our FE conversion from the BE response only considers numbers for day-of-week, not MON, TUES etc. We can either make the FE convertor understand both formats, or unify the formats in the BE.

cc @johnstcn for opinion.

I'm of the opinion that the FE should understand both formats.

@johnstcn
Copy link
Member

According to the manual page for crontab names can be used only for the day-of-week and month fields. Since we do not (yet) allow setting the month field, I think adding support for day-of-week names is reasonable here.

@greyscaled
Copy link
Contributor

I'll take a look at tackling this problem by utilizing cron-parser instead of our homegrown parsing.

@misskniss misskniss mentioned this issue Jun 1, 2022
Closed
90 tasks
@misskniss misskniss changed the title Bug: Frontend: Workspace Schedule: For a new workspace days of week are not defaulted to settings defined at creation time via cli Bug: Workspace Schedule: For a new workspace days of week are not defaulted to settings defined at creation time via cli Jun 1, 2022
@misskniss misskniss assigned misskniss and unassigned misskniss Jun 1, 2022
@misskniss misskniss added site Area: frontend dashboard Community MVP 🏁 labels Jun 1, 2022
@misskniss misskniss added this to the Community MVP milestone Jun 2, 2022
@misskniss misskniss changed the title Bug: Workspace Schedule: For a new workspace days of week are not defaulted to settings defined at creation time via cli Bug: Workspace Schedule: Days of week are not defaulted to settings defined at creation time via cli (new workspace) Jun 2, 2022
@greyscaled greyscaled self-assigned this Jun 2, 2022
@greyscaled greyscaled mentioned this issue Jun 2, 2022
Closed
20 tasks
greyscaled added a commit that referenced this issue Jun 2, 2022
@greyscaled
fix: FE parsing of schedule with day strings
6fbece8 
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 `fields` representation of the parsed expression
was missing from their docs. I might open an issue or PR to update them!
@greyscaled greyscaled mentioned this issue Jun 2, 2022
Merged
greyscaled added a commit that referenced this issue Jun 3, 2022
@greyscaled
Merge branch 'main' into vapurrmaid/gh-1901/cron-parser
ddd6d5c
greyscaled added a commit that referenced this issue Jun 3, 2022
@greyscaled
fix: FE parsing of schedule with day strings (#2006)
37aff0c 
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 `fields` representation of the parsed expression
was missing from their docs. I might open an issue or PR to update them!
kylecarbs pushed a commit that referenced this issue Jun 10, 2022
@greyscaled @kylecarbs
fix: FE parsing of schedule with day strings (#2006)
bd4ba9d 
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 `fields` representation of the parsed expression
was missing from their docs. I might open an issue or PR to update them!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@greyscaled greyscaled

Labels
site Area: frontend dashboard
Projects
None yet
Milestone
Community MVP
Development

Successfully merging a pull request may close this issue.

fix: FE parsing of schedule with day strings coder/coder
4 participants
@misskniss @johnstcn @nadzeyav @greyscaled