-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
enter replicasets from deployments #3001
base: master
Are you sure you want to change the base?
Conversation
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.
@rudrakhp Nice!
I don't think i can review the code, so i will keep it for @derailed 😄 .
IMO, you're right that the replicaset makes more sense on enter press than the pod list. But, we should keep the pod behavior in a different shortcut (like P key if not used) and not removing it. I'm sure it's being used.
Side note, in any case we need to update the docs on the new logic
I think the |
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.
@rudrakhp Thank you for this PR! Tho technically this is correct, I don't think we should alter the current behavior as this was a conscious decision to get to the pertinent information quickly aka show me the latest incarnation of the pods associated with this deployment.
That said, I think it makes sense to nav dp->rs so users can see the various versions at stake.
@rudrakhp Also the tests need some TLC |
22003ed
to
0fb7066
Compare
@derailed somehow this slipped my mind, updated the PR, do check! |
0fb7066
to
cdbd848
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.
@rudrakhp Thanks for this update!
internal/view/dp.go
Outdated
@@ -47,6 +47,7 @@ func (d *Deploy) bindKeys(aa *ui.KeyActions) { | |||
ui.KeyShiftR: ui.NewKeyAction("Sort Ready", d.GetTable().SortColCmd(readyCol, true), false), | |||
ui.KeyShiftU: ui.NewKeyAction("Sort UpToDate", d.GetTable().SortColCmd(uptodateCol, true), false), | |||
ui.KeyShiftL: ui.NewKeyAction("Sort Available", d.GetTable().SortColCmd(availCol, true), false), | |||
ui.KeyR: ui.NewKeyAction("Show Replicasets", d.replicaSetsCmd, true), |
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.
We need another key here as r
is already allocated to restart.
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.
@derailed mb, done!
Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
cdbd848
to
39a413a
Compare
Fixes #2951
Today on entering a deployment, pods associated with it are listed. Changed behaviour to list replicasets instead. Entering replicaset will list the related pods.
Ran
make build && ./execs/k9s
on a test cluster to validate behaviour.