-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[dashboard, server] Add global custom timeout preference #16503
Conversation
started the job as gitpod-build-pd-timeout-16089.6 because the annotations in the pull request description changed |
314b33a
to
ebf57a4
Compare
@loujaybee Could you help me think of some documents or desc in preference? |
@gtsiolis Could you help review the design here? should we add a new menu? i.e. not under |
Something need discuss
|
Looking at this now! 👀 |
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.
Thanks for adding this, @iQQBot! 🔮
Added below some UX comments based on the following design.
To also answer your question about adding a menu in #16503 (comment), I don't think this needs a separate menu in user settings for now. ✔️
Inactivity Timeout |
---|
Approving to unblock merging, but holding in case someone from the @gitpod-io/engineering-webapp
needs to take a closer look at the code changes. 🏓
/hold
<CheckBox | ||
title="Disabled Close Timeout" | ||
desc={<span></span>} | ||
checked={disabledClosedTimeout} | ||
onChange={(e) => actuallySetDisabledClosedTimeout(e.target.checked)} | ||
/> |
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.
question: What does this do? It seems to not work. Do we need 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.
what do you mean not work?
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 feature is used to disable the default close timeout, which means you will no longer have the five minute fast timeout limit when closing your browser or disconnecting from ssh
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.
Ah, it seems to work now. I could not toggle the checkbox when I tried earlier. 🤷
Still, what does this checkbox do? Do we need 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.
explain in the previous message, is it make sense?
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.
Thanks @iQQBot! Just highlighting some copy changes from the suggestion above to simplify and improve readability. What do you think?
BEFORE | AFTER |
---|---|
💡 tip: Also, using font-semibold
instead of bold
can improve the rendering here.
❗ issue: This is also missing some validation states where a user enters a blank value or an invalid value but relying on the browser alert sounds ok for this iteration. Could you open a follow-up issue for doing proper form validation in this case?
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 5m close timeout only for gitpod.io
. Other installation may have different value, and it set in workspace cluster, so server actually don't know it
And this 30m default inactive timeout is depend user plan, e.g. free user got 30m, paid user got 60m....
So if we want display default value and valid value in server side, we may need some new api 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.
Just highlighting some copy changes from the suggestion above to simplify and improve readability. What do you think?
I'm not a fan of the "keep workspace running" copy. It feels very ambiguous.
Could we go with:
"Close workspaces on editor disconnect (5 minutes)" (default = selected)
Or similar?
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 5m close timeout only for gitpod.io. Other installation may have different value, and it set in workspace cluster, so server actually don't know it
In Dedicated, this type of policy would likely be controlled through the organisation settings. Can we not safely assume 5 minutes for now as the general default until we have also this configuration at the admin/organisation level? Dedicated will have very little infra level variance, unless exposed through the product.
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.
The workspace inactivity timeout copy + behaviour looks good, the editor timeout needs some refinement. I have created an internal RFC to resolve some questions about the naming of the editor timeout copy text and naming. My intention is to get feedback and close that decision in the next 24 hours to unblock this PR.
However, given this PR is solving two distinct product issues:
- Introduce global user timeout default #16200
- Move the "close timeout" (IDE + editor timeout) to a user preference. #16089
If we are keen to merge it, we should consider either commenting out/disabling the editor timeout part of this PR + raising a separate PR for the editor timeout, whilst we resolve naming + copy decisions. Or wait until that decision and merge both at once.
Big thanks for @gtsiolis |
5737f15
to
655c2b9
Compare
@iQQBot Left a few comments above. Also, the preview env is not working atm 😢 |
59edcca
to
c6b20e1
Compare
@geropl Thanks for your review, I already addressed your feedback, and the preview env is works now, you can have a try. |
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.
Thanks for adapting this @iQQBot ! 🙏 - Would be good if we could leave a message for the disabled setting, and/or simplify the placeholder message. However, both of these could be done in a follow up PR, I'd be happy to ship this as it currently is.
Added @gitpod-io/engineering-webapp as a reviewer, but a re-review from @geropl or @easyCZ (as they already have context would also be good!) 🙏 |
Looking at this (again) now! 👀 |
908770a
to
f242605
Compare
@@ -494,6 +499,33 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |||
return user; | |||
} | |||
|
|||
public async supportConfigureWorkspaceTimeout(ctx: TraceContext): Promise<boolean> { |
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.
The support
puts me off a bit: We don't use that word, yet, anywhere. So using it here means adding uncertainty. What about sticking to can
or may
instead? 🤔
Also, we happen to make (nearly) exactly the same method over here. Maybe it's worth re-using/combining the two? 🤷
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.
the method signature is not same, and it is only in enterprise
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.
the method signature is not same
The signature is effectively identical: If we make maySetTimeout
public, it's basically maySetWorkspaceTimeout(ctx: TraceContext)
, where we get the user
on the first or second line.
Anyway, want to push more for the name. 🤗
and it is only in enterprise
I agree that's super confusing, and we should change that very soon: it' the code we use for all distribution modes: there is 0️⃣ difference, we ship that "ee" code to everyone.
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 made some changes, could you check this commit?
I can't test in the preview env: How to proceed @iQQBot ? Have you or somebody else tested the current version? |
Could you try again? |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
🧡 Thanks @iQQBot !
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.
Code LGTM, but cannot test due to (unrelated) preview problems 👍
/unhold |
Description
[dashboard, server] Add global custom timeout preference
Related Issue(s)
Closes #16200
How to test
kubectl describe pod ws-{instanceID}
to check your workspace timeout and closed timeout.Release Notes
Documentation
Build Options:
Experimental feature to run the build with GitHub Actions (and not in Werft).
leeway-target=components:all
Run Leeway with
--dont-test
Publish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh