Allow to select multiple resources and add support to Delete/Kill all together #354
Conversation
internal/ui/table.go
Outdated
| @@ -326,6 +337,9 @@ func (v *Table) buildRow(row int, data resource.TableData, sk string, pads MaxyP | |||
| c.SetExpansion(1) | |||
| c.SetAlign(align) | |||
| c.SetTextColor(f(data.Namespace, data.Rows[sk])) | |||
| if m { | |||
| c.SetBackgroundColor(tcell.ColorDarkGray) //FIXME | |||
There was a problem hiding this comment.
@derailed could you please suggest a better approach to define this highlight color? I'm thinking to add a new attribute to "skin", but this could cause some incompatibility with custom user skins.
There was a problem hiding this comment.
@bkmeneguello Thank you for this PR! I am not sure about this feature. I somewhat see the motivation if pods come and go as the results of jobs running and being completed and hence would alter the selection. Perhaps a better approach here would be to track the actual selection and not the selection index in the table. Which I think this what the mark would do for you. Another approach would be to use the filter or a managing resource ie look at pods by deployment or other to only select the resources you care to manipulate by eliminating other resources noise? The deal here is all the commands would now have to deal with multiple selected items potentially which is currently not part of the design. I am not saying this feature is not worth investigating but as a cluster admin I don't find myself having to bulk manipulate resources very often. It might be good for you to detail the use cases here so we can further weight out this effort. ie what would describe or logs on multiple selected resources would actually look like?
There was a problem hiding this comment.
@derailed I hope I can to demonstrate the usefulness of this feature witch I'm currently using daily basis.
- I'm not tracking indices, but resource keys, so if the table is sorted or filtered the selected items keep the same (marks is a string slice).
- My use case is a Jenkins Kubernetes cluster, so pods are created all the time and are short-lived, and sometimes they fail by some reason, so I need to kill them and Jenkins will create another one. But eventually between the time I focus a pod and hit ctrl+k the screen is updated and I kill the wrong pod. Another cause is when I have a large pod failure and need to kill dozens of pods (suffering with the previous problem).
- Not all command should deal with multiple selection. To this moment I only identified delete/kill, but some more could be added, like taint nodes. To prevent confusing users I tried to hide commands which don't support multiple selection when something is marked, but the actions aren't refreshed (I think is a bug) and didn't investigated further. I think it could be one more option of KeyAction and filter actions automatically.
I hope I made my point clear. This tool is awesome and this feature is really a must have for me. I don't know if you are a Htop user, but it's the same processes used there to mark processes to invoke "kill"
There was a problem hiding this comment.
@bkmeneguello Thank you so much for the clarification! I get it now and agree with you on the usefulness. I do think the logic your are using for selecting resources is the correct approach and we should generalize it as the correct way to select. I'll also take a closer look and see what's up with the refresh and will assist to push this thru. As for the mark color I think we can introduce a new skin for it as K9s is still very much pre-release. I very much appreciate your kind words, support and this effort to improve K9s for all of us!
Thank you Bruno!!
There was a problem hiding this comment.
@derailed, all basic functionality is working. It's ready for review.
bkmeneguello
changed the title
There was a problem hiding this comment.
That's awesome @bkmeneguello, sorry for the late review. I'll try to contact Fernand so we can get this merged :)
I haven't tried this yet but will do it tomorrow and see if we are missing anything here!
internal/resource/list.go
Outdated
| @@ -66,6 +66,7 @@ type ( | |||
| Rows RowEvents | |||
| NumCols map[string]bool | |||
| Namespace string | |||
| Marks []string | |||
There was a problem hiding this comment.
What are your thoughts about changing this to a map[string]struct{} instead of a slice? It would be simpler to mark, unmark and check if something is marked.
| v.refresh() | ||
| } | ||
| for _, res := range sel { | ||
| v.app.Flash().Infof("Delete resource %s %s", v.list.GetName(), res) |
There was a problem hiding this comment.
These "flashes" could go on very fast, right?
I think it would be better to the user receiving a message like: Deleting 10 pods than receiving 10 lightning fast messages about deleting a single resource.
| } | ||
| for _, res := range sel { | ||
| v.app.Flash().Infof("Delete resource %s %s", v.list.GetName(), res) | ||
| if err := v.list.Resource().Delete(res, true, false); err != nil { |
There was a problem hiding this comment.
Should we fail fast here? I meaning, if one delete fail should we keep trying to delete others?
There was a problem hiding this comment.
I don't think so, because if someone select multiple resources we should make the best effort to delete them all. If something fails, the resources will still be selected and the user can take further actions upon them.
Allow to select multiple resources and add support to Delete/Kill all together
Fixes: #353