Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: workspace view for schedules#991

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

Merged
greyscaled merged 2 commits intomainfromvapurrmaid/274/auto-on-off-components
Apr 14, 2022

Conversation

greyscaled
Copy link
Contributor

Summary:

This adds the client-side implementation to match the types introduced
in#879 and#844 as well as a card in the Workspaces page to present
workspace the data.

Details:

  • Added a convenient line break in the example schedule.Weekly
  • Added missingjson:"" annotations in codersdk/workspaces.go
  • Installed cronstrue for displaying human-friendly cron strings
  • Adjusted/Added client-side types to match codersdk/workspaces.go
  • Added new component WorkspaceSchedule.tsx

Next Steps:

The WorkspaceSchedule.tsx card only presents data (on purpose). In order
to make it PUT/modify data, a few changes will be made:

  • a form for updating workspace schedule will be created
  • the form will wrapped in a dialog or modal
  • the WorkspaceSchedule card will have a way of opening the modal which
    will likely be generalized up to WorkspaceSection.tsx

Impact:

This is user-facing

This does not fully resolve either#274 or#275 (I may further decompose
that work to reflect reality and keep things in small deliverable
increments), but adds significant progress towards both.

Summary:This adds the client-side implementation to match the types introducedin#879 and#844 as well as a card in the Workspaces page to presentworkspace the data.Details:* Added a convenient line break in the example schedule.Weekly* Added missing `json:""` annotations in codersdk/workspaces.go* Installed cronstrue for displaying human-friendly cron strings* Adjusted/Added client-side types to match codersdk/workspaces.go* Added new component WorkspaceSchedule.tsxNext Steps:The WorkspaceSchedule.tsx card only presents data (on purpose). In orderto make it PUT/modify data, a few changes will be made:- a form for updating workspace schedule will be created- the form will wrapped in a dialog or modal- the WorkspaceSchedule card will have a way of opening the modal whichwill likely be generalized up to WorkspaceSection.tsxImpact:This is user-facingThis does not fully resolve either#274 or#275 (I may further decomposethat work to reflect reality and keep things in small deliverableincrements), but adds significant progress towards both.
@greyscaledgreyscaled requested a review froma team as acode ownerApril 13, 2022 07:47
@greyscaledgreyscaled self-assigned thisApr 13, 2022
@codecov
Copy link

codecovbot commentedApr 13, 2022
edited
Loading

Codecov Report

Merging#991 (8dbd904) intomain (ce49966) willincrease coverage by0.23%.
The diff coverage is70.58%.

@@            Coverage Diff             @@##             main     #991      +/-   ##==========================================+ Coverage   66.53%   66.77%   +0.23%==========================================  Files         240      242       +2       Lines       14588    14639      +51       Branches      115      119       +4     ==========================================+ Hits         9706     9775      +69+ Misses       3892     3882      -10+ Partials      990      982       -8
FlagCoverage Δ
unittest-go-macos-latest53.35% <ø> (-0.14%)⬇️
unittest-go-postgres-66.57% <ø> (+0.29%)⬆️
unittest-go-ubuntu-latest56.06% <ø> (-0.09%)⬇️
unittest-go-windows-202252.93% <ø> (+0.05%)⬆️
unittest-js61.95% <70.58%> (+0.38%)⬆️
Impacted FilesCoverage Δ
coderd/autostart/schedule/schedule.go83.33% <ø> (ø)
codersdk/workspaces.go63.63% <ø> (ø)
site/src/test_helpers/handlers.ts24.13% <0.00%> (-3.87%)⬇️
site/src/api/index.ts62.16% <25.00%> (-10.26%)⬇️
...ite/src/components/Workspace/WorkspaceSchedule.tsx75.00% <75.00%> (ø)
site/src/components/Workspace/Workspace.tsx100.00% <100.00%> (ø)
site/src/test_helpers/entities.ts100.00% <100.00%> (ø)
site/src/util/schedule.ts100.00% <100.00%> (ø)
peerbroker/listen.go84.16% <0.00%> (-3.34%)⬇️
peer/channel.go83.23% <0.00%> (-2.32%)⬇️
... and11 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatece49966...8dbd904. Read thecomment docs.

@greyscaled
Copy link
ContributorAuthor

@johnstcn and@presleyp - I assigned you as UI reviewers on Chromatic.

The "overview" of this PR on chromatic where you can approve is here:https://www.chromatic.com/pullrequest?appId=624de63c6aacee003aa84340&number=991

The individual changes/diffs for the latest build are here:https://www.chromatic.com/build?appId=624de63c6aacee003aa84340&number=312

Please note that the latter URL changes on each build whereas the former is static for this PR. If you wish to navigate to the most recent build you can either:

  1. Click theDetails link for theUI Tests check OR
  2. Go to theUI Review url (https://www.chromatic.com/pullrequest?appId=624de63c6aacee003aa84340&number=991) and scroll to the bottom where it will show the latest build and a button likeGo to build xyz

I'm trying to make comments like these until we've had more experience with the Chromatic workflow, sorry if superfluous!

* @example
* expandScheduleCronString("30 9 1-5") // -> "30 9 * * 1-5"
*/
export const expandScheduleCronString = (schedule: string): string => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(non-blocking): Would it simplify the frontend code to alter the backend to support a full valid 5-value cron string?

Copy link
ContributorAuthor

@greyscaledgreyscaledApr 13, 2022
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good question, hmm. Only if the backend was validating that we always received a full valid 5-value cron string (with optional timezone). The only reason it would help is so that I know I can pass these strings directly from the backend into JS cron libraries for validation and display.

Given that right now I have to first prepare them by using thisexpandScheduleCronString, which is a simple and well-tested function, I'm not overly concerned about it, but I do think a db migration for it that would have to expand stored values in the same way would be more complicated.

One situationI don't want is a user being able to set a 5-value cron string via CLI or API that sets day of month and month values. While the backend may ignore them, the FE would accidentally produce a confusing human-readable string for them like this:

At 09:30 on day-of-month 1 and on every day-of-week from Monday through Friday in February.

instead of

At 09:30 on Monday through Friday.

I plan on making the C/U form a set of widgets that under the hood produce a cron string. So imagine an input for time and another with the 7 days of the week that you select/deselect.

Conceptually, the form will look like:

time (TIMEZONE)[ 9:30                  c ]days[ ] Sun [x] Mon [x] Tues [x] Weds [x] Thurs [x] Fri [ ] SatYour workspace will auto-[start/stop] at 09:30 on Monday through Friday.

EDIT: Overall, my opinion is that the backend remains with the 3-value crontab as the cost is simply just this function we're looking at right now, which I think is acceptable. It doesn't seem like we fully bypass cost any which way unless the backend only supports 5-value cron strings and validates thatday-of-month andmonth are*.

johnstcn reacted with thumbs up emoji
Copy link
Member

@johnstcnjohnstcnApr 13, 2022
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Gotcha. Opened#993 for this; I can make the necessary frontend changes if needed.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think the only changes that would be needed in a PR targeting#993 would be removing this function, which is ezpz 🎉

johnstcn reacted with hooray emoji
* DEFAULT_TIMEZONE is the default timezone that crontab assumes unless one is
* specified.
*/
const DEFAULT_TIMEZONE = "UTC"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

// Must be kept in sync with backend Workspace struct
/**
* @remarks Keep in sync with codersdk/workspaces.go
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nice

@greyscaledgreyscaled merged commit42e9956 intomainApr 14, 2022
@greyscaledgreyscaled deleted the vapurrmaid/274/auto-on-off-components branchApril 14, 2022 00:35
@greyscaled
Copy link
ContributorAuthor

This resolved#1059

@missknissmisskniss added this to theV2 Beta milestoneMay 15, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@presleyppresleyppresleyp approved these changes

Assignees

@greyscaledgreyscaled

Labels
None yet
Projects
None yet
Milestone
V2 Beta
Development

Successfully merging this pull request may close these issues.

4 participants
@greyscaled@presleyp@johnstcn@misskniss

[8]ページ先頭

©2009-2025 Movatter.jp