Skip to content
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

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

Merged
merged 1 commit into from
Nov 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions components/dashboard/src/projects/ProjectSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* See License-AGPL.txt in the project root for license information.
*/

import { useContext } from "react";
import { useContext, useEffect, useState } from "react";
import { useLocation } from "react-router";
import { Project, ProjectSettings, Team } from "@gitpod/gitpod-protocol";
import CheckBox from "../components/CheckBox";
Expand All @@ -14,6 +14,8 @@ import { PageWithSubMenu } from "../components/PageWithSubMenu";
import PillLabel from "../components/PillLabel";
import { ProjectContext } from "./project-context";
import { FeatureFlagContext } from "../contexts/FeatureFlagContext";
import SelectWorkspaceClass from "../settings/selectClass";
import { BillingMode } from "@gitpod/gitpod-protocol/lib/billing-mode";

export function getProjectSettingsMenu(project?: Project, team?: Team) {
const teamOrUserSlug = !!team ? "t/" + team.slug : "projects";
Expand Down Expand Up @@ -48,6 +50,14 @@ export function ProjectSettingsPage(props: { project?: Project; children?: React
export default function () {
const { showPersistentVolumeClaimUI } = useContext(FeatureFlagContext);
const { project, setProject } = useContext(ProjectContext);
const [teamBillingMode, setTeamBillingMode] = useState<BillingMode | undefined>(undefined);
const { teams } = useContext(TeamsContext);
const team = getCurrentTeam(useLocation(), teams);
useEffect(() => {
if (team) {
getGitpodService().server.getBillingModeForTeam(team.id).then(setTeamBillingMode);
}
}, [team]);

if (!project) return null;

Expand All @@ -59,6 +69,15 @@ export default function () {
setProject({ ...project, settings: newSettings });
};

const setWorkspaceClass = async (value: string) => {
if (!project) {
return value;
}
const before = project.settings?.workspaceClasses?.regular;
updateProjectSettings({ workspaceClasses: { prebuild: value, regular: value } });
return before;
};

return (
<ProjectSettingsPage project={project}>
<h3>Prebuilds</h3>
Expand Down Expand Up @@ -142,8 +161,12 @@ export default function () {

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

Choose a reason for hiding this comment

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

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
Screenshot 2022-11-09 at 6 16 05 PM Screenshot 2022-11-09 at 6 15 57 PM

<SelectWorkspaceClass
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Nice component reuse! 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

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

workspaceClass={project.settings?.workspaceClasses?.regular}
enabled={BillingMode.canSetWorkspaceClass(teamBillingMode)}
setWorkspaceClass={setWorkspaceClass}
/>
Comment on lines +164 to +168
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

{!BillingMode.canSetWorkspaceClass(teamBillingMode) && <h3 className="mt-12">Workspaces</h3>}
<CheckBox
title={
<span>
Expand Down
20 changes: 19 additions & 1 deletion components/dashboard/src/settings/Preferences.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import SelectIDE from "./SelectIDE";
import SelectWorkspaceClass from "./selectClass";
import { PageWithSettingsSubMenu } from "./PageWithSettingsSubMenu";
import { BillingMode } from "@gitpod/gitpod-protocol/lib/billing-mode";
import { WorkspaceClasses } from "@gitpod/gitpod-protocol";

type Theme = "light" | "dark" | "system";

Expand Down Expand Up @@ -49,13 +50,30 @@ export default function Preferences() {
}
};

const setWorkspaceClass = async (value: string) => {
const additionalData = user?.additionalData || {};
const prevWorkspaceClass = additionalData?.workspaceClasses?.regular;
const workspaceClasses = (additionalData?.workspaceClasses || {}) as WorkspaceClasses;
workspaceClasses.regular = value;
workspaceClasses.prebuild = value;
additionalData.workspaceClasses = workspaceClasses;
if (value !== prevWorkspaceClass) {
await getGitpodService().server.updateLoggedInUser({ additionalData });
}
return prevWorkspaceClass;
};

return (
<div>
<PageWithSettingsSubMenu title="Preferences" subtitle="Configure user preferences.">
<h3>Editor</h3>
<p className="text-base text-gray-500 dark:text-gray-400">Choose the editor for opening workspaces.</p>
<SelectIDE location="preferences" />
<SelectWorkspaceClass enabled={BillingMode.canSetWorkspaceClass(userBillingMode)} />
<SelectWorkspaceClass
workspaceClass={user?.additionalData?.workspaceClasses?.regular}
enabled={BillingMode.canSetWorkspaceClass(userBillingMode)}
setWorkspaceClass={setWorkspaceClass}
/>
Comment on lines +72 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +72 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

<h3 className="mt-12">Theme</h3>
<p className="text-base text-gray-500 dark:text-gray-400">Early bird or night owl? Choose your side.</p>
<div className="mt-4 space-x-3 flex">
Expand Down
22 changes: 7 additions & 15 deletions components/dashboard/src/settings/selectClass.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,25 @@
* See License-AGPL.txt in the project root for license information.
*/

import { useContext, useEffect, useState } from "react";
import { useEffect, useState } from "react";
import { getGitpodService } from "../service/service";
Copy link
Member Author

Choose a reason for hiding this comment

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

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

import { UserContext } from "../user-context";
import { trackEvent } from "../Analytics";
import { WorkspaceClasses } from "@gitpod/gitpod-protocol";
import WorkspaceClass from "../components/WorkspaceClass";
import { SupportedWorkspaceClass } from "@gitpod/gitpod-protocol/lib/workspace-class";

interface SelectWorkspaceClassProps {
enabled: boolean;
workspaceClass?: string;
setWorkspaceClass: (value: string) => Promise<string | undefined>;
}

export default function SelectWorkspaceClass(props: SelectWorkspaceClassProps) {
const { user } = useContext(UserContext);

const [workspaceClass, setWorkspaceClass] = useState<string>(user?.additionalData?.workspaceClasses?.regular || "");
const [workspaceClass, setWorkspaceClass] = useState<string | undefined>(props.workspaceClass);
const actuallySetWorkspaceClass = async (value: string) => {
const additionalData = user?.additionalData || {};
const prevWorkspaceClass = additionalData?.workspaceClasses?.regular || "";
const workspaceClasses = (additionalData?.workspaceClasses || {}) as WorkspaceClasses;
workspaceClasses.regular = value;
workspaceClasses.prebuild = value;
additionalData.workspaceClasses = workspaceClasses;
if (value !== prevWorkspaceClass) {
await getGitpodService().server.updateLoggedInUser({ additionalData });
const previousValue = await props.setWorkspaceClass(value);
if (previousValue !== value) {
trackEvent("workspace_class_changed", {
previous: prevWorkspaceClass,
previous: previousValue,
current: value,
});
setWorkspaceClass(value);
Expand Down
4 changes: 3 additions & 1 deletion components/gitpod-protocol/src/teams-projects-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* See License-AGPL.txt in the project root for license information.
*/

import { PrebuiltWorkspaceState } from "./protocol";
import { PrebuiltWorkspaceState, WorkspaceClasses } from "./protocol";
import { v4 as uuidv4 } from "uuid";
import { DeepPartial } from "./util/deep-partial";
import { WebhookEvent } from "./webhook-event";
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

export interface Project {
Expand Down
5 changes: 0 additions & 5 deletions components/server/ee/src/container-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member Author

Choose a reason for hiding this comment

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

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

import { WorkspaceStarter } from "../../src/workspace/workspace-starter";
import { UserDeletionService } from "../../src/user/user-deletion-service";
import { BlockedUserFilter } from "../../src/auth/blocked-user-filter";
import { EMailDomainService, EMailDomainServiceImpl } from "./auth/email-domain-service";
Expand Down Expand Up @@ -105,9 +103,6 @@ export const productionEEContainerModule = new ContainerModule((bind, unbind, is
bind(UserDeletionServiceEE).toSelf().inSingletonScope();
rebind(UserDeletionService).to(UserDeletionServiceEE).inSingletonScope();

// workspace management
rebind(WorkspaceStarter).to(WorkspaceStarterEE).inSingletonScope();

// acounting
bind(AccountService).to(AccountServiceImpl).inSingletonScope();
bind(SubscriptionService).toSelf().inSingletonScope();
Expand Down
11 changes: 8 additions & 3 deletions components/server/ee/src/prebuilds/prebuild-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Copy link
Member Author

@svenefftinge svenefftinge Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

❓ Would the PR still work without this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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. 🧘

excludeFeatureFlags: ["full_workspace_backup"],
});
}
Expand All @@ -273,7 +273,12 @@ export class PrebuildManager {
}
}

async retriggerPrebuild(ctx: TraceContext, user: User, workspaceId: string): Promise<StartPrebuildResult> {
async retriggerPrebuild(
ctx: TraceContext,
user: User,
project: Project | undefined,
workspaceId: string,
): Promise<StartPrebuildResult> {
const span = TraceContext.startSpan("retriggerPrebuild", ctx);
span.setTag("workspaceId", workspaceId);
try {
Expand All @@ -297,7 +302,7 @@ export class PrebuildManager {
if (workspace.projectId) {
projectEnvVars = await this.projectService.getProjectEnvironmentVariables(workspace.projectId);
}
await this.workspaceStarter.startWorkspace({ span }, workspace, user, [], projectEnvVars);
await this.workspaceStarter.startWorkspace({ span }, workspace, user, project, [], projectEnvVars);
return { prebuildId: prebuild.id, wsid: workspace.id, done: false };
} catch (err) {
TraceContext.setError({ span }, err);
Expand Down
5 changes: 4 additions & 1 deletion components/server/ee/src/workspace/gitpod-server-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,10 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
// queued for long than a minute? Let's retrigger
console.warn("Retriggering queued prebuild.", prebuiltWorkspace);
try {
await this.prebuildManager.retriggerPrebuild(ctx, user, workspaceID);
const project = prebuiltWorkspace.projectId
? await this.projectDB.findProjectById(prebuiltWorkspace.projectId)
: undefined;
await this.prebuildManager.retriggerPrebuild(ctx, user, project, workspaceID);
} catch (err) {
console.error(err);
}
Expand Down
39 changes: 0 additions & 39 deletions components/server/ee/src/workspace/workspace-starter.ts
View file
Open in desktop

This file was deleted.

8 changes: 7 additions & 1 deletion components/server/src/workspace/gitpod-server-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,10 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
throw new ResponseError(ErrorCodes.PERMISSION_DENIED, "Cannot (re-)start a deleted workspace.");
}
const userEnvVars = this.userDB.getEnvVars(user.id);
let projectEnvVarsPromise = this.internalGetProjectEnvVars(workspace.projectId);
const projectEnvVarsPromise = this.internalGetProjectEnvVars(workspace.projectId);
const projectPromise = workspace.projectId
? this.projectDB.findProjectById(workspace.projectId)
: Promise.resolve(undefined);
Comment on lines +722 to +724
Copy link
Member

Choose a reason for hiding this comment

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

await syntax?

Copy link
Member

Choose a reason for hiding this comment

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

This is an parallelization strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


await mayStartPromise;

Expand All @@ -727,6 +730,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
ctx,
workspace,
user,
await projectPromise,
await userEnvVars,
await projectEnvVarsPromise,
{
Expand Down Expand Up @@ -1199,6 +1203,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
ctx,
workspace,
user,
project,
await envVars,
await projectEnvVarsPromise,
);
Expand Down Expand Up @@ -2980,6 +2985,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
let user = this.checkAndBlockUser("getSupportedWorkspaceClasses");
let selectedClass = await WorkspaceClasses.getConfiguredOrUpgradeFromLegacy(
user,
undefined,
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not passing the project here?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a project context

this.config.workspaceClasses,
this.entitlementService,
);
Expand Down
6 changes: 5 additions & 1 deletion components/server/src/workspace/workspace-classes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { WorkspaceDB } from "@gitpod/gitpod-db/lib";
import { User, Workspace } from "@gitpod/gitpod-protocol";
import { Project, User, Workspace } from "@gitpod/gitpod-protocol";
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
import { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing";
import { EntitlementService } from "../billing/entitlement-service";
Expand Down Expand Up @@ -159,9 +159,13 @@ export namespace WorkspaceClasses {
*/
export async function getConfiguredOrUpgradeFromLegacy(
user: User,
project: Project | undefined,
classes: WorkspaceClassesConfig,
entitlementService: EntitlementService,
): Promise<string> {
if (project?.settings?.workspaceClasses?.regular) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

geropl marked this conversation as resolved.
Show resolved Hide resolved
return project?.settings?.workspaceClasses?.regular;
}
if (user.additionalData?.workspaceClasses?.regular) {
return user.additionalData?.workspaceClasses?.regular;
}
Expand Down
1 change: 1 addition & 0 deletions components/server/src/workspace/workspace-starter.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ async function execute(builder: WorkspaceClassTestBuilder, expectedClass: string
workspace,
previousInstance,
user,
undefined,
Copy link
Member

Choose a reason for hiding this comment

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

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

entitlementService,
config,
workspaceDb,
Expand Down
Loading