fix(site): exclude workspace schedule settings for prebuilt workspaces #18826
Conversation
c6e4ff3 to
95ca5f7
Compare
site/src/pages/WorkspaceSettingsPage/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/WorkspaceSettingsPage/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'd suggest adding a story here to showcase the new behaviour.
There was a problem hiding this comment.
Addressed in 73f5476
There was a problem hiding this comment.
FE changes look good to me 👍. Just left a comment related to naming. Also, would be nice to add a story for this use case as Cian mentioned #18826 (comment).
coderd/workspaces.go
Outdated
| @@ -2231,6 +2231,12 @@ func convertWorkspace( | |||
| if latestAppStatus.ID == uuid.Nil { | |||
| appStatus = nil | |||
| } | |||
|
|
|||
| isPrebuild := false | |||
There was a problem hiding this comment.
A few nonblocking thoughts:
- Consider
isPrebuiltinstead ofisPrebuilt. I know is a small and subtle naming change but a Prebuild is not the same thing as a prebuilt workspace. - Also perhaps consider a comment or something to explain that this is for unclaimed prebuilt workspaces only. A claimed workspace can still be called prebuilt and the change in this PR does not affect those.
- should we have a
prebuilds.IsUnclaimedPrebuiltWorkspace(...)function to encapsulate and abstract this logic?
There was a problem hiding this comment.
Consider isPrebuilt instead of isPrebuilt. I know is a small and subtle naming change but a Prebuild is not the same thing as a prebuilt workspace.
The way I have been using these terms, is Prebuild = Prebuilt Workspace, but maybe I have been using it wrong. What is the difference between Prebuild and Prebuilt Workspace?
Also perhaps consider a comment or something to explain that this is for unclaimed prebuilt workspaces only. A claimed workspace can still be called prebuilt and the change in this PR does not affect those.
Added a comment in 0bde254, let me know if it works.
should we have a prebuilds.IsUnclaimedPrebuiltWorkspace(...) function to encapsulate and abstract this logic?
Yes, we can do that and reuse it in other parts of the code that already have this logic. I can address it in a separate PR. Edit: actually, there is a better solution that I completely forgot 🤦♀️ which is to use workspace.IsPrebuild(). This encapsulates that logic on workspace and improves readability: 0120c5b
I will create a follow up PR to add some tests to test this parameter.
…schedule-settings-for-prebuilds
…schedule-settings-for-prebuilds
Description
This PR updates the UI to avoid rendering workspace schedule settings (autostop, autostart, etc.) for prebuilt workspaces. Instead, it displays an informational message with a link to the relevant documentation.
Changes
IsPrebuildparameter toconvertWorkspaceto indicate whether the workspace is a prebuild.Relates with: #18762