-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[dashboard, server] Add global custom timeout preference #16503
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8b5649e
c98eae2
00c7986
9ee6e9d
93c093d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,7 +163,12 @@ import { IDEOptions } from "@gitpod/gitpod-protocol/lib/ide-protocol"; | |
import { PartialProject } from "@gitpod/gitpod-protocol/lib/teams-projects-protocol"; | ||
import { ClientMetadata } from "../websocket/websocket-connection-manager"; | ||
import { ConfigurationService } from "../config/configuration-service"; | ||
import { EnvVarWithValue, ProjectEnvVar } from "@gitpod/gitpod-protocol/lib/protocol"; | ||
import { | ||
AdditionalUserData, | ||
EnvVarWithValue, | ||
ProjectEnvVar, | ||
WorkspaceTimeoutSetting, | ||
} from "@gitpod/gitpod-protocol/lib/protocol"; | ||
import { InstallationAdminSettings, TelemetryData } from "@gitpod/gitpod-protocol"; | ||
import { Deferred } from "@gitpod/gitpod-protocol/lib/util/deferred"; | ||
import { InstallationAdminTelemetryDataProvider } from "../installation-admin/telemetry-data-provider"; | ||
|
@@ -494,6 +499,33 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |
return user; | ||
} | ||
|
||
public async maySetTimeout(ctx: TraceContext): Promise<boolean> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧡 Thanks @iQQBot ! |
||
const user = this.checkUser("maySetTimeout"); | ||
await this.guardAccess({ kind: "user", subject: user }, "get"); | ||
|
||
return await this.entitlementService.maySetTimeout(user, new Date()); | ||
} | ||
|
||
public async updateWorkspaceTimeoutSetting( | ||
ctx: TraceContext, | ||
setting: Partial<WorkspaceTimeoutSetting>, | ||
): Promise<void> { | ||
traceAPIParams(ctx, { setting }); | ||
if (setting.workspaceTimeout) { | ||
WorkspaceTimeoutDuration.validate(setting.workspaceTimeout); | ||
} | ||
|
||
const user = this.checkAndBlockUser("updateWorkspaceTimeoutSetting"); | ||
await this.guardAccess({ kind: "user", subject: user }, "update"); | ||
|
||
if (!(await this.entitlementService.maySetTimeout(user, new Date()))) { | ||
throw new Error("configure workspace timeout only available for paid user."); | ||
} | ||
|
||
AdditionalUserData.set(user, setting); | ||
await this.userDB.updateUserPartial(user); | ||
} | ||
|
||
public async sendPhoneNumberVerificationToken(ctx: TraceContext, rawPhoneNumber: string): Promise<void> { | ||
this.checkUser("sendPhoneNumberVerificationToken"); | ||
return this.verificationService.sendVerificationToken(formatPhoneNumber(rawPhoneNumber)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,7 @@ import { | |
Project, | ||
GitpodServer, | ||
IDESettings, | ||
WorkspaceTimeoutDuration, | ||
} from "@gitpod/gitpod-protocol"; | ||
import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics"; | ||
import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; | ||
|
@@ -1437,6 +1438,7 @@ export class WorkspaceStarter { | |
lastValidWorkspaceInstanceId, | ||
); | ||
const userTimeoutPromise = this.entitlementService.getDefaultWorkspaceTimeout(user, new Date()); | ||
const allowSetTimeoutPromise = this.entitlementService.maySetTimeout(user, new Date()); | ||
|
||
let featureFlags = instance.configuration!.featureFlags || []; | ||
|
||
|
@@ -1467,7 +1469,19 @@ export class WorkspaceStarter { | |
spec.setClass(instance.workspaceClass!); | ||
|
||
if (workspace.type === "regular") { | ||
spec.setTimeout(await userTimeoutPromise); | ||
const [defaultTimeout, allowSetTimeout] = await Promise.all([userTimeoutPromise, allowSetTimeoutPromise]); | ||
spec.setTimeout(defaultTimeout); | ||
if (allowSetTimeout) { | ||
if (user.additionalData?.workspaceTimeout) { | ||
try { | ||
let timeout = WorkspaceTimeoutDuration.validate(user.additionalData?.workspaceTimeout); | ||
spec.setTimeout(timeout); | ||
} catch (err) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to swallow any exception here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think yes, We have already verified once when the user submits the settings. and this only for some edge case e.g. we change rule, at this point in time we output this error and it doesn't change anything, i.e. not system error, we cannot improve it, it's just an annoying thing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd 🧡 to see this kind of reasoning backed into comments. 🙂 |
||
} | ||
if (user.additionalData?.disabledClosedTimeout === true) { | ||
spec.setClosedTimeout("0"); | ||
} | ||
} | ||
} | ||
spec.setAdmission(admissionLevel); | ||
const sshKeys = await this.userDB.trace(traceCtx).getSSHPublicKeys(user.id); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to communicate this rather than showing the
alert
box? It's rather un-friendly and has the potential of spamming the user if the error happens to trigger on a loop. @gtsiolis what's a good pattern to communicate an API error here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@easyCZ Yes:
However, I'd not block merging this as is for now, even if it's just a browser alert.
Thanks for the ping, @easyCZ! 🏓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alert is suitable for display in a modal, where it feels like there is no suitable place for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think we should really ever be using a native browser
alert()
, for reasons @easyCZ described, and it also communicates a very unpolished feel. Showing an inline<Alert>
message in the UI next to the place the button or form lives seems like a decent approach. If we had toast notifications, that would be nice as well, but we don't atm.