[server] Allow setting ws-class on project level #14535

svenefftinge · 2022-11-08T19:19:41Z

Description

Introduces a project settings to specify the workspace class.

Related Issue(s)

How to test

Release Notes

Workspace classes can be set in the project settings

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide

werft-gitpod-dev-com · 2022-11-08T19:19:49Z

started the job as gitpod-build-sefftinge-allow-specification-of-10963.1 because the annotations in the pull request description changed
(with .werft/ from main)

svenefftinge · 2022-11-09T07:40:38Z

components/dashboard/src/settings/selectClass.tsx

@@ -8,29 +8,22 @@ import { useContext, useEffect, useState } from "react";
 import { getGitpodService } from "../service/service";


changes in this file are to extract the user-preference-specific code, making the component reusable in the project settings context.

svenefftinge · 2022-11-09T07:41:45Z

components/server/ee/src/container-module.ts

@@ -46,8 +46,6 @@ import { ChargebeeService } from "./user/chargebee-service";
 import { StripeService } from "./user/stripe-service";
 import { EligibilityService } from "./user/eligibility-service";
 import { AccountStatementProvider } from "./user/account-statement-provider";
-import { WorkspaceStarterEE } from "./workspace/workspace-starter";


WorkspaceStarterEE does no longer container any logic, so I deleted it and also removed the corresponding bindings.

svenefftinge · 2022-11-09T07:43:04Z

components/server/ee/src/prebuilds/prebuild-manager.ts

@@ -259,7 +259,7 @@ export class PrebuildManager {
            } else {
                span.setTag("starting", true);
                const projectEnvVars = await projectEnvVarsPromise;
-                await this.workspaceStarter.startWorkspace({ span }, workspace, user, [], projectEnvVars, {
+                await this.workspaceStarter.startWorkspace({ span }, workspace, user, project, [], projectEnvVars, {


I'm adding the optional project to the call graph of workspace starts in various places, so that we fetch the project once and pass it down instead of repeatedly asking the DB.

Why is that important in the context of this PR?
Also, triggers "premature optimization?" reflex.

❓ Would the PR still work without this change?

It's not only because of reducing db queries but also a principle of good software design to explicitly pass what is needed as an argument rather than fetching data internally from a global state (the DB). I.e. more functional and easier to understand when you see the signature.

also a principle of good software design to explicitly pass what is needed as an argument rather than fetching data internally from a global state (the DB)

That is a bit too vague and general for my taste to count as an argument, honestly. Then we'd need to talk service/layer boundaries first IMO, and sometimes there are good arguments against it as well.

But let's not hold up this PR any longer, I think the change is good overall. If we disagree in style we should settle that outside of PRs. 🧘

svenefftinge · 2022-11-09T07:45:54Z

components/server/src/workspace/workspace-classes.ts

        classes: WorkspaceClassesConfig,
        entitlementService: EntitlementService,
    ): Promise<string> {
+        if (project?.settings?.workspaceClasses?.regular) {


(1/3) use of the new project setting (precedence over user setting)

svenefftinge · 2022-11-09T07:46:06Z

components/server/src/workspace/workspace-starter.ts

                        config,
                        entitlementService,
                    );
                    workspaceClass = WorkspaceClasses.selectClassForRegular(prebuildClass, userClass, config);
+                } else if (project?.settings?.workspaceClasses?.regular) {


(2/3) use of the new project setting (precedence over user setting)

svenefftinge · 2022-11-09T07:46:16Z

components/server/src/workspace/workspace-starter.ts

                } else if (user.additionalData?.workspaceClasses?.regular) {
                    workspaceClass = user.additionalData?.workspaceClasses?.regular;
                }
            }

            if (workspace.type == "prebuild") {
-                if (user.additionalData?.workspaceClasses?.prebuild) {
+                if (project?.settings?.workspaceClasses?.prebuild) {


(3/3) use of the new project setting (precedence over user setting)

easyCZ · 2022-11-09T10:02:26Z

components/server/src/workspace/workspace-starter.spec.ts

@@ -326,6 +326,7 @@ async function execute(builder: WorkspaceClassTestBuilder, expectedClass: string
        workspace,
        previousInstance,
        user,
+        undefined,


Should there be also a test which does pass in a Project?

components/server/src/workspace/workspace-classes.ts

easyCZ · 2022-11-09T10:04:01Z

components/server/src/workspace/gitpod-server-impl.ts

@@ -2980,6 +2985,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
        let user = this.checkAndBlockUser("getSupportedWorkspaceClasses");
        let selectedClass = await WorkspaceClasses.getConfiguredOrUpgradeFromLegacy(
            user,
+            undefined,


Why are we not passing the project here?

relevant: https://github.com/gitpod-io/gitpod/pull/14535/files#r1017953380

We don't have a project context

easyCZ · 2022-11-09T10:04:49Z

components/server/src/workspace/gitpod-server-impl.ts

+        const projectPromise = workspace.projectId
+            ? this.projectDB.findProjectById(workspace.projectId)
+            : Promise.resolve(undefined);


await syntax?

This is an parallelization strategy.

Yes, I'm creating a promise here and await it below, especially after the mayStartPromise which takes the longest time.

easyCZ · 2022-11-09T10:08:32Z

components/gitpod-protocol/src/teams-projects-protocol.ts

@@ -21,6 +21,8 @@ export interface ProjectSettings {
    allowUsingPreviousPrebuilds?: boolean;
    // how many commits in the commit history a prebuild is good (undefined and 0 means every commit is prebuilt)
    prebuildEveryNthCommit?: number;
+    // preferred workspace classes
+    workspaceClasses?: WorkspaceClasses;


This is bit of a smell. Why is it plural workspaceClasses when we're not passing in a list?

Currently we look to only have a single preference, could we go with a list here (which is initially only 1 item long) and allow for the "preferences" to be a list?

I can imagine wanting to specify a priority list of classes I want to use. This also facilitates gradual migration away from one WS class to another over time - by changing the priorities (ordering)

I think it make senses to reuse the shape and naming we already have in place. Although I agree WorkspaceClassConfig or similar would have been a better choice in the first place.

Besides that I copied the shape from user settings, I don't think it is smelly. As it represents multiple (currently two) workspace classes.

geropl · 2022-11-09T13:51:04Z

Wanted to test, but noticed the build was failing.

gtsiolis · 2022-11-09T16:04:35Z

Looking at this now! 👀

svenefftinge · 2022-11-09T16:34:23Z

Wanted to test, but noticed the build was failing.

Fixed and deployed it

gtsiolis

UX LGTM! Thanks for the ping[1], @svenefftinge! 🏓

Left some non-blocking comments that can be tackled separately in follow-up issues if needed.

Also, good call on moving this to the workspace settings section.

gtsiolis · 2022-11-09T16:34:17Z

components/dashboard/src/projects/ProjectSettings.tsx

+                    <SelectWorkspaceClass
+                        workspaceClass={project.settings?.workspaceClasses?.regular}
+                        enabled={BillingMode.canSetWorkspaceClass(teamBillingMode)}
+                        setWorkspaceClass={setWorkspaceClass}
+                    />


thought: It's a bit confusing that preview environments contain two quite different classes from what's on Gitpod.io and the order is also reversed, but that's out of the scope of these changes, see relevant discussion (internal).

gtsiolis · 2022-11-09T16:34:51Z

components/dashboard/src/projects/ProjectSettings.tsx

@@ -142,8 +161,12 @@ export default function () {

            {showPersistentVolumeClaimUI && (
                <>
-                    <br></br>
-                    <h3 className="mt-12">Workspaces</h3>
+                    <SelectWorkspaceClass


praise: Nice component reuse! 😁

gtsiolis · 2022-11-09T16:37:34Z

components/dashboard/src/projects/ProjectSettings.tsx

@@ -142,8 +161,12 @@ export default function () {

            {showPersistentVolumeClaimUI && (
                <>
-                    <br></br>
-                    <h3 className="mt-12">Workspaces</h3>


suggestion: What do you think of moving the workspaces settings to the top of the general project settings here now that selecting a workspace class is a bit more prominent as it affects both workspaces and prebuilds?

BEFORE AFTER

gtsiolis · 2022-11-09T16:39:28Z

components/dashboard/src/projects/ProjectSettings.tsx

@@ -142,8 +161,12 @@ export default function () {

            {showPersistentVolumeClaimUI && (
                <>
-                    <br></br>
-                    <h3 className="mt-12">Workspaces</h3>
+                    <SelectWorkspaceClass


thought: Feels good to see we're working towards making Teams & Projects more useful.

gtsiolis · 2022-11-09T16:44:14Z

components/dashboard/src/settings/Preferences.tsx

+                <SelectWorkspaceClass
+                    workspaceClass={user?.additionalData?.workspaceClasses?.regular}
+                    enabled={BillingMode.canSetWorkspaceClass(userBillingMode)}
+                    setWorkspaceClass={setWorkspaceClass}
+                />


issue: The duplicate preference in user settings and project settings now poses a question on which preference overrides which. The workspace class scope could be something worth documenting if it's unclear but the project setting should override the user setting, correct?

Settings scope for workspace classes
Your project setting overrides the user setting.

gtsiolis · 2022-11-09T16:46:08Z

components/dashboard/src/settings/Preferences.tsx

+                <SelectWorkspaceClass
+                    workspaceClass={user?.additionalData?.workspaceClasses?.regular}
+                    enabled={BillingMode.canSetWorkspaceClass(userBillingMode)}
+                    setWorkspaceClass={setWorkspaceClass}
+                />


suggestion: Once this is merged, it could be worth updating the existing and soon outdated docs.

You can configure the workspace class that should be used for your workspaces in your user preferences.

geropl

Tested and works.

👍 For George's comment about updating our docs ✨

svenefftinge requested a review from a team November 8, 2022 19:19

roboquat added the release-note label Nov 8, 2022

roboquat added the size/M label Nov 8, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 8, 2022

svenefftinge force-pushed the sefftinge/allow-specification-of-10963 branch from 2e8bf4a to a24635e Compare November 8, 2022 19:43

roboquat added size/L and removed size/M labels Nov 8, 2022

svenefftinge force-pushed the sefftinge/allow-specification-of-10963 branch from a24635e to 5ea3bfd Compare November 8, 2022 19:59

svenefftinge added the needs visual design label Nov 8, 2022

This was referenced Nov 8, 2022

Allow specification of workspace class in project settings #10963

Closed

Support specifying workspace class in gitpod.yml #14257

Closed

svenefftinge force-pushed the sefftinge/allow-specification-of-10963 branch from 5ea3bfd to 8fe883a Compare November 9, 2022 07:36

svenefftinge commented Nov 9, 2022

View reviewed changes

easyCZ reviewed Nov 9, 2022

View reviewed changes

gtsiolis self-requested a review November 9, 2022 14:40

[server] Allow setting ws-class on project level

984be6b

svenefftinge force-pushed the sefftinge/allow-specification-of-10963 branch from 8fe883a to 984be6b Compare November 9, 2022 16:02

gtsiolis approved these changes Nov 9, 2022

View reviewed changes

geropl approved these changes Nov 10, 2022

View reviewed changes

roboquat merged commit 08be419 into main Nov 10, 2022

roboquat deleted the sefftinge/allow-specification-of-10963 branch November 10, 2022 08:45

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 10, 2022

jldec mentioned this pull request Nov 19, 2022

SelectWorkspaceClass in ProjectSettings should not depend on PVC feature flag #14800

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server] Allow setting ws-class on project level #14535

[server] Allow setting ws-class on project level #14535

svenefftinge commented Nov 8, 2022 •

edited

Loading

werft-gitpod-dev-com bot commented Nov 8, 2022

svenefftinge Nov 9, 2022

svenefftinge Nov 9, 2022

svenefftinge Nov 9, 2022 •

edited

Loading

geropl Nov 9, 2022

svenefftinge Nov 9, 2022

geropl Nov 10, 2022

svenefftinge Nov 9, 2022

svenefftinge Nov 9, 2022

svenefftinge Nov 9, 2022

easyCZ Nov 9, 2022

easyCZ Nov 9, 2022

geropl Nov 9, 2022

svenefftinge Nov 9, 2022

easyCZ Nov 9, 2022

geropl Nov 9, 2022

svenefftinge Nov 9, 2022

easyCZ Nov 9, 2022

geropl Nov 9, 2022

svenefftinge Nov 9, 2022

geropl commented Nov 9, 2022

gtsiolis commented Nov 9, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Nov 9, 2022

gtsiolis left a comment

gtsiolis Nov 9, 2022

gtsiolis Nov 9, 2022

gtsiolis Nov 9, 2022

gtsiolis Nov 9, 2022

gtsiolis Nov 9, 2022

gtsiolis Nov 9, 2022

geropl left a comment

		@@ -8,29 +8,22 @@ import { useContext, useEffect, useState } from "react";
		import { getGitpodService } from "../service/service";

[server] Allow setting ws-class on project level #14535

[server] Allow setting ws-class on project level #14535

Conversation

svenefftinge commented Nov 8, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

werft-gitpod-dev-com bot commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenefftinge Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geropl commented Nov 9, 2022

gtsiolis commented Nov 9, 2022 • edited by werft-gitpod-dev-com bot Loading

svenefftinge commented Nov 9, 2022

gtsiolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geropl left a comment

Choose a reason for hiding this comment

svenefftinge commented Nov 8, 2022 •

edited

Loading

svenefftinge Nov 9, 2022 •

edited

Loading

gtsiolis commented Nov 9, 2022 •

edited by werft-gitpod-dev-com bot

Loading