[server] simplify workspace classes #15091
Conversation
|
started the job as gitpod-build-se-simplify-ws-classes.1 because the annotations in the pull request description changed |
svenefftinge
force-pushed
the
se/simplify-ws-classes
branch
from
13a7715 to
afb4a52
Compare
github-actions
bot
added
team: devx
team: webapp
roboquat
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
| WorkspaceClasses.validate(config.workspaceClasses); | ||
| if (config.workspaceClasses.filter((c) => c.isDefault).length !== 1) { | ||
| log.error( | ||
| "Exactly one default workspace class needs to be configured: " + |
There was a problem hiding this comment.
Nit: Could be clarified as "Exactly one workspace class should be configured as default: "
| @@ -282,7 +282,6 @@ yq w -i "${INSTALLER_CONFIG_PATH}" experimental.webapp.workspaceClasses[1].categ | |||
| yq w -i "${INSTALLER_CONFIG_PATH}" experimental.webapp.workspaceClasses[1].displayName "Small" | |||
| yq w -i "${INSTALLER_CONFIG_PATH}" experimental.webapp.workspaceClasses[1].description "Small workspace class (20GB disk)" | |||
| yq w -i "${INSTALLER_CONFIG_PATH}" experimental.webapp.workspaceClasses[1].powerups "2" | |||
| yq w -i "${INSTALLER_CONFIG_PATH}" experimental.webapp.workspaceClasses[1].isDefault "false" | |||
| yq w -i "${INSTALLER_CONFIG_PATH}" experimental.webapp.workspaceClasses[1].deprecated "false" | |||
There was a problem hiding this comment.
Whoops, did you mean to remove .deprecated instead of .isDefault here?
There was a problem hiding this comment.
Many thanks @svenefftinge! This second PR also looks good 👍 (modulo the rebase and previous comments to be addressed)
Also, holding due to one question: The UX generally works really well and is quite intuitive -- set class on project, get class on project workspaces 👍 not having a global user preference simplifies user mental model a lot. 💯
The only place where I'm a bit puzzled is workspaces based on prebuilds:
- When I start a workspace, it always gets the class currently defined on project-level (regardless of any prebuild class)
- This means that, when I switch from one class to another, various workspaces might get a different class to the prebuild they're based on
- I guess this is generally fine (very simple and predictable behavior), but it could lead to problems e.g. when the prebuild backup size is larger than the current workspace class allows, right?
Anyway, this is 99% fine by me, and my point is mostly food for thought, but I'm raising it in case this wasn't intended or if we want to do anything about the error cases where you can't start a workspace due to a specified class that is not compatible with the prebuild's class.
/hold
simplified by - removing user-level setting - removing deprecation - removing ws-class inheritence between ws sessions
svenefftinge
force-pushed
the
se/simplify-ws-classes
branch
from
afb4a52 to
9cce255
Compare
Yes, the workspace start would fail and should tell you that the class is incompatible. It currently doesn't provide a great error message, but this situation would be a rare corner case, because the rebuild would need to have created a backup bigger than 20GB which is super rare. |
|
/unhold |
Based on & includes #15090
Description
This PR reduces complexity about what workspace classes is used by
Related Issue(s)
Fixes #15080
Related https://github.com/gitpod-io/ops/pull/7076
How to test
Create a project and try the workspace class selection in its settings.
Release Notes
Documentation
Werft options:
If enabled this will build
install/previewValid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh