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.

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

Add gp timeout show command #10782

Merged
merged 1 commit into from
Jun 24, 2022
Merged

Add gp timeout show command #10782

merged 1 commit into from
Jun 24, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Jun 21, 2022

Description

Add a gp timeout show command to the gp CLI to go along with the recently added gp timeout extend command (see #10619).

Related Issue(s)

How to test

  1. Create a workspace in preview environment
  2. Run gp timeout show
  3. Output should be (value depends on user's plan):
60m
  1. Extend the workspace timeout with gp timeout extend.
  2. Rerun gp timeout show. Output should be:
180m
  1. Open another workspace, and extend its timeout gp timeout extend
  2. Back to first one, gp timeout show should back to original one

Release Notes

Add command `gp timeout show` to show the timeout of current workspace

Documentation

We'll need to add something to https://github.com/gitpod-io/website/pull/2218 either before or after it's merged.

Werft options:

  • /werft with-preview

@akosyakov akosyakov requested a review from mustard-mh June 21, 2022 07:58
@akosyakov
Copy link
Member

@andrew-farries Don't forget about docs update. At least file an issue.

@andrew-farries andrew-farries force-pushed the af/add-timeout-show-command branch from 4c40e14 to 3dffeed Compare June 21, 2022 08:02
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Jun 21, 2022

@mustard-mh Should we add the docs for this to your existing docs PR (https://github.com/gitpod-io/website/pull/2218) or would you rather I create a new one?

@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 21, 2022

@mustard-mh Should we add the docs for this to your existing docs PR (https://github.com/gitpod-io/website/pull/2218) or would you rather I create a new one?

@andrew-farries You can do it after previous one https://github.com/gitpod-io/website/pull/2218 merged, I can ping you after that

Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

I tested with it. As we can see in image below,

default value is long
after extend 180m
extend another workspace, this workspace will reset to default long

If we can't format every output time to 180m, maybe we can turn 180m to extended?

PS. As a user I prefer 180m formated duration🙂

If we will keep it, it's good for me too, but we should explain more in doc

export const WORKSPACE_TIMEOUT_DEFAULT_SHORT = "short";
export const WORKSPACE_TIMEOUT_DEFAULT_LONG = "long";
export const WORKSPACE_TIMEOUT_EXTENDED = "extended";
export const WORKSPACE_TIMEOUT_EXTENDED_ALT = "180m"; // for backwards compatibility since the IDE uses this
export const WorkspaceTimeoutValues = [
WORKSPACE_TIMEOUT_DEFAULT_SHORT,
WORKSPACE_TIMEOUT_DEFAULT_LONG,
WORKSPACE_TIMEOUT_EXTENDED,
WORKSPACE_TIMEOUT_EXTENDED_ALT,
] as const;

image

@mustard-mh
Copy link
Contributor

#10782 (review) What do you think/prefer? @loujaybee

Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

If we want to turn all output into 180m formatted, maybe we need a map/func in protocol, since we have multiple similar code like this below. Or just hard code here too

public workspaceTimeoutToDuration(timeout: WorkspaceTimeoutDuration): string {
switch (timeout) {
case WORKSPACE_TIMEOUT_DEFAULT_SHORT:
return "30m";
case WORKSPACE_TIMEOUT_DEFAULT_LONG:
return "60m";
case WORKSPACE_TIMEOUT_EXTENDED:
case WORKSPACE_TIMEOUT_EXTENDED_ALT:
return "180m";
}
}

public durationToWorkspaceTimeout(duration: string): WorkspaceTimeoutDuration {
switch (duration) {
case "30m":
return WORKSPACE_TIMEOUT_DEFAULT_SHORT;
case "60m":
return WORKSPACE_TIMEOUT_DEFAULT_LONG;
case "180m":
return WORKSPACE_TIMEOUT_EXTENDED_ALT;
default:
return WORKSPACE_TIMEOUT_DEFAULT_SHORT;
}
}

@andrew-farries andrew-farries requested a review from a team June 21, 2022 10:44
@andrew-farries andrew-farries marked this pull request as draft June 21, 2022 10:47
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jun 21, 2022
@andrew-farries andrew-farries force-pushed the af/add-timeout-show-command branch 2 times, most recently from eb64445 to 745e845 Compare June 21, 2022 11:30
@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 21, 2022

Notice that if we need changes from server, this PR will be approved before next WebApp deploy, or it will block IDE deployment 🙏

@andrew-farries
Copy link
Contributor Author

We can get the raw time duration from server, without conversion to short or extended. This behaves as expected:

gitpod /workspace/dotfiles (main) $ gp timeout show
60m
gitpod /workspace/dotfiles (main) $ gp timeout extend
gitpod /workspace/dotfiles (main) $ gp timeout show
180m

@andrew-farries andrew-farries marked this pull request as ready for review June 21, 2022 12:01
Copy link
Member

@filiptronicek filiptronicek left a comment

Choose a reason for hiding this comment

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

Thanks a lot @andrew-farries for the new command 🧡! I tested it and ti works as advertised.

Maybe we could add to the minutes just a "status" which would indicate whether the timeout is the default one or the extended one? Something like

$ gp timeout show
60m (Default)
$ gp timeout extend
$ gp timeout show
180m (Extended)

@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 22, 2022

Thanks a lot @andrew-farries for the new command 🧡! I tested it and ti works as advertised.

Maybe we could add to the minutes just a "status" which would indicate whether the timeout is the default one or the extended one? Something like

$ gp timeout show
60m (Default)
$ gp timeout extend
$ gp timeout show
180m (Extended)

@filiptronicek Remember that 60m is not default for all users, value of default is depend on Plan of user/team.

cc @andrew-farries

Image
image

@andrew-farries
Copy link
Contributor Author

Remember that 60m is not default for all users, value of default is depend on Plan of user/team.

I suggest we leave the output as is for now, ie without (Default) or (Extended).

@andrew-farries
Copy link
Contributor Author

/hold

@filiptronicek
Copy link
Member

Remember that 60m is not default for all users, value of default is depend on Plan of user/team.

@mustard-mh I agree, thanks for bringing this up. I wanted to say, that as a user, I would personally like to check gp timeout show just to see how many minutes my timeout is and whether I have successfully for example extended it for this repo. For checking the subscription, there could be something like gp billing (not sure if suggested before).

I suggest we leave the output as is for now, ie without (Default) or (Extended).

@andrew-farries I see where you're coming from, it could perhaps be in some way programmatically consumed afterwards as well, but I would prefer to see a bit more context about my timeout, if we have it available. We could also ship it as is and afterwards provide a flag or similar to provide such information.

@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 22, 2022

If a user cannot extend timeout, output will be like this @filiptronicek

Only Unleashed plan can extend timeout 🙈

Image
image

@andrew-farries andrew-farries force-pushed the af/add-timeout-show-command branch 6 times, most recently from 33f998f to 62eea1d Compare June 22, 2022 13:24
Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

Tested and code LGTM

/hold

I'll add them to PR description too

@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 22, 2022

@mustard-mh Should we add the docs for this to your existing docs PR (gitpod-io/website#2218) or would you rather I create a new one?

@andrew-farries You can do it after previous one gitpod-io/website#2218 merged, I can ping you after that

@andrew-farries Previous doc PR has been merged, you can start it or ask @filiptronicek to help with it 👍

@andrew-farries
Copy link
Contributor Author

@loujaybee loujaybee requested a review from a team June 23, 2022 09:31
@mustard-mh
Copy link
Contributor

Thank you @andrew-farries , could you split this PR into two PRs, to unblock deployment? server will approve first, and then gp-cli.

See more context in internal chat

@andrew-farries
Copy link
Contributor Author

could you split this PR into two PRs, to unblock deployment? server will approve first, and then gp-cli.

I've done this. This PR now contains just the gp change and #10896 has the server change.

@andrew-farries andrew-farries force-pushed the af/add-timeout-show-command branch 2 times, most recently from 133b2eb to 0e1e454 Compare June 24, 2022 13:26
@andrew-farries andrew-farries force-pushed the af/add-timeout-show-command branch from 0e1e454 to c984c1c Compare June 24, 2022 13:28
@geropl geropl self-assigned this Jun 24, 2022
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Thx, works as expected! 👍

image
nit: I think the long in the beginning might be a hint towards a conversion bug in server. But as it's a display issue only: 👍

@jeanp413
Copy link
Member

Some feedback, not really important, but I would expect some more user friendly output other than just a number, same with extend a message telling me what was the side effect of running this command

Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM

@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 24, 2022

/unhold

Some feedback, not really important, but I would expect some more user friendly output other than just a number, same with extend a message telling me what was the side effect of running this command

I see where you're coming from, it could perhaps be in some way programmatically consumed afterwards as well, but I would prefer to see a bit more context about my timeout, if we have it available. We could also ship it as is and afterwards provide a flag or similar to provide such information.

You can do it in another PR if you want, to print more friendly 🙂 cc @jeanp413 @filiptronicek

i.e. extend success don't print anything🙈, we can show current ws timeout

@roboquat roboquat merged commit 3225597 into main Jun 24, 2022
@roboquat roboquat deleted the af/add-timeout-show-command branch June 24, 2022 16:07
@mustard-mh mustard-mh removed the team: webapp Issue belongs to the WebApp team label Jun 24, 2022
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: IDE IDE change is running in production deployed Change is completely running in production release-note size/M team: IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants