Improve crictl inspect[pi] commands to allow filtering
#1553
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
approved
saschagrunert
force-pushed
the
inspect-filter
branch
2 times, most recently
from
b0d297a to
87dd0c4
Compare
saschagrunert
force-pushed
the
inspect-filter
branch
from
87dd0c4 to
65f389f
Compare
|
@kubernetes-sigs/cri-tools-maintainers PTAL |
saschagrunert
force-pushed
the
inspect-filter
branch
2 times, most recently
from
ffe0cc4 to
a2d56b8
Compare
| return fmt.Errorf("listing images: %w", err) | ||
| } | ||
| for _, img := range r.GetImages() { | ||
| ids = append(ids, img.GetId()) |
There was a problem hiding this comment.
do we need to deduplicate here?
There was a problem hiding this comment.
I'd say no, because right now it behaves in the same way to output two images when doing something like:
sudo crictl inspecti 324bc02ae1231 324bc02ae1231
| @@ -322,7 +374,7 @@ var listPodCommand = &cli.Command{ | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if err = ListPodSandboxes(runtimeClient, opts); err != nil { | |||
| if err = OutputPodSandboxes(runtimeClient, opts); err != nil { | |||
There was a problem hiding this comment.
if filtered by reference, what image reference will the output use as a main one? My understanding is that even with the filter, the output may show different reference as a "main" name of an image and tag one filtered by in a list of alternative names. Is it right? I wonder this is actually the case and whether we can either improve output or make it clear in docs so not to confuse users
There was a problem hiding this comment.
Hm, pods are not filterable by reference, are we speaking about crictl inspecti? My main goal was to mimic closely as possible the existing crictl pods, ps and images subcommand flags to avoid confusion.
| @@ -322,7 +374,7 @@ var listPodCommand = &cli.Command{ | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if err = ListPodSandboxes(runtimeClient, opts); err != nil { | |||
| if err = OutputPodSandboxes(runtimeClient, opts); err != nil { | |||
There was a problem hiding this comment.
in case of various filters, potential improvement over time may be to show filter-specific additional column. For since it may be time passed since that value in the filter, for example. This is just a nit.
The commands now allow filtering based on various fields or just to inspect all pods, containers or image. Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
force-pushed
the
inspect-filter
branch
from
a2d56b8 to
7c06bc4
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert, SergeyKanzhelev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
The commands now allow filtering based on various fields or just to inspect all pods, containers or images.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?