-
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
[gitpod-cli] New cmd gp tasks stop
🧪🔬
#12116
Conversation
gp tasks stop
gp tasks stop
🧪🔬
986a6df
to
fa495b3
Compare
fa495b3
to
893175d
Compare
func stopTask(ctx context.Context, terminalClient supervisor.TerminalServiceClient, terminalAlias string) { | ||
terminal, err := terminalClient.Get(context.Background(), &supervisor.GetTerminalRequest{Alias: terminalAlias}) | ||
if err != nil { | ||
panic(err) |
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'm not sure about usage of panic, usually we are using logger with fatal exit. Although in cli it should not be so bad, could you check and align with how we do in other commands please
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.
Some commands use panic
others use fail
func fail(msg string) {
fmt.Fprintln(os.Stderr, msg)
os.Exit(-1)
}
I think they are similar and both OK for CLI, but we should consider using a single approach. I will create an issue 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.
it seem even in this file we use log.Fatalf
somewhere. Let's use log
everywhere?
6a993f9
to
94ede01
Compare
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.
Hello hello, @andreafalzetti!
This feature is honestly great. I love it and it works swimmingly. Here's some things I took note of:
- I accidentally did
gp stop a01cefb2-edc6-4e3a-9d5c-9f6ae97b01a1
instead ofgp tasks stop a01cefb2-edc6-4e3a-9d5c-9f6ae97b01a1
, which I am embarrassed about 😆. Probably nothing actionable but I think it may happen to others as well. - When one provides an incorrect ID for a task (maybe someone who is not really familiar with task IDs or they copy-paste incorrectly), it looks like we don't handle that properly:
$ gp tasks stop andrea panic: rpc error: code = NotFound desc = terminal not found goroutine 1 [running]: github.com/gitpod-io/gitpod/gitpod-cli/cmd.stopTask({0xb96a70?, 0xc00003a080?}, {0xb99c78, 0xc00026d080}, {0x7ffd7ed43406, 0x6}) /tmp/build/components-gitpod-cli--app.6087b2bc176a15865c74375746932db6c25af9c2/cmd/tasks-stop.go:27 +0x105 github.com/gitpod-io/gitpod/gitpod-cli/cmd.glob..func20(0xf731a0?, {0xc00026cc80, 0x1, 0x1?}) /tmp/build/components-gitpod-cli--app.6087b2bc176a15865c74375746932db6c25af9c2/cmd/tasks-stop.go:125 +0x567 github.com/spf13/cobra.(*Command).execute(0xf731a0, {0xc00026cc50, 0x1, 0x1}) /home/gitpod/go-packages/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:856 +0x663 github.com/spf13/cobra.(*Command).ExecuteC(0xf718a0) /home/gitpod/go-packages/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:960 +0x39d github.com/spf13/cobra.(*Command).Execute(...) /home/gitpod/go-packages/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:897 github.com/gitpod-io/gitpod/gitpod-cli/cmd.Execute() /tmp/build/components-gitpod-cli--app.6087b2bc176a15865c74375746932db6c25af9c2/cmd/root.go:35 +0x325 main.main() /tmp/build/components-gitpod-cli--app.6087b2bc176a15865c74375746932db6c25af9c2/main.go:12 +0x17
@andreafalzetti sorry for putting you on wrong track, we should actually use supervisor API for that [1] as our other clients do [2] |
gp tasks stop
🧪🔬gp tasks stop
🧪🔬
Is it ready for review, or will there be more changes related to @akosyakov's last comment? |
Moved to draft, sorry 🙏 I will refactor as soon as I have some spare time, considering that it's low-priority (not scheduled as part of our weekly work) |
d781e8b
to
8d79756
Compare
8d79756
to
913e5c6
Compare
@@ -12,7 +12,7 @@ require ( | |||
github.com/google/shlex v0.0.0-20181106134648-c34317bd91bf | |||
github.com/google/tcpproxy v0.0.0-20180808230851-dfa16c61dad2 | |||
github.com/gorilla/handlers v1.5.1 | |||
github.com/manifoldco/promptui v0.3.2 | |||
github.com/manifoldco/promptui v0.9.0 |
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 newer version allows hiding the message after selection using HideSelected
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 ran go mod tidy
that's why these changes
913e5c6
to
fd20cb9
Compare
}) | ||
|
||
if err != nil { | ||
fmt.Printf("The selected task was not found: %s.\nMake sure to use the correct task ID.\nUse 'gp tasks list' to obtain the task id or run 'gp tasks stop' to select the desired task\n", args[0]) |
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 it too verbose, what do you guys think?
I like the idea from @filiptronicek to provide more info because the task ID could be confusing for some users.
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 like it this way, I think it makes for a message that users without an idea about task IDs can understand.
Ready for review again |
When there is no tasks defined in That is because supervisor will create one default task here
@akosyakov What this default task works for? If it's necessary, we can leave a comment here, if not, we can remove it to make the task stop works better |
I cannot remember already, probably to have at least on terminal if there are not task terminals. You can try to remove it and see how it works. I would probably try it in another PR thought? |
fd20cb9
to
d6ec849
Compare
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-afalz-gp-tasks-stop.13 |
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
Nice works in gp tasks
👍
d6ec849
to
d66d836
Compare
@mustard-mh It's a fair point, I've updated the error message with "The selected task was not found or already stopped". We could look how to offer a completelly different message in the future, if the task exists but stopped. |
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.
Nice
Description
I would like to introduce this new cmd
gp tasks stop
to stop 1 or all workspace tasks. It can be useful when someone wants to run some new tasks and don't care about all pre-defined ones or quickly kill some long-living process running in another task, for example.I am not sure if the approach is too naive, but it seems to work.
Related Issue(s)
There is no real issue as this is more of a "weekend idea". Perhaps it could be considered slightly related to inner loop.
How to test
gp tasks stop
,gp tasks stop <id>
andgp tasks stop --all
Release Notes
Documentation
Will create issue/PR if the idea moves forward
Werft options: