Allow filtering containers by command #24791
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arsenalzp The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
arsenalzp
force-pushed
the
issue_24664
branch
from
63e80c1 to
7a8043c
Compare
|
the change LGTM, but you'll need to update the man pages too |
Thank you!
Could you please me an advice how to fix it? |
Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> add a test, improve logic of command filter Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> improve a test Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> improve test, update a man page Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
arsenalzp
force-pushed
the
issue_24664
branch
from
7a8043c to
899381e
Compare
| // ExternalContainerFilter is a function to determine whether a container list is included | ||
| // in command output. Container lists to be outputted are tested using the function. | ||
| // A true return will include the container list, a false return will exclude it. | ||
| type ExternalContainerFilter func(*contEntities.ListContainer) bool |
There was a problem hiding this comment.
Why is this part of libpod? There is not a single field that depends on any libpod type so I don't think this should belong here.
There was a problem hiding this comment.
Yes, you're right, that's my failure I used Runtime receiver, I wanted to move filtering logic into separate function.
Let me keep it as separate non-exported function or do you want to put filtering logic into Runtime.GetContainers() function back?
| // Apply container filters on bunch of external container lists | ||
| func (r *Runtime) ApplyExternalContainersFilters(containersList []*contEntities.ListContainer, filters ...ExternalContainerFilter) []contEntities.ListContainer { | ||
| ctrsFiltered := make([]contEntities.ListContainer, 0, len(containersList)) | ||
|
|
||
| for _, ctr := range containersList { | ||
| include := true | ||
| for _, filter := range filters { | ||
| include = include && filter(ctr) | ||
| } | ||
|
|
||
| if include { | ||
| ctrsFiltered = append(ctrsFiltered, *ctr) | ||
| } | ||
| } | ||
|
|
||
| return ctrsFiltered |
There was a problem hiding this comment.
Same here, this does not depend on the libpod runtime in anyway so I don't think we should have this in libpod unless @mheon really want this here.
There was a problem hiding this comment.
Is there a reason it needs to be a Runtime method? I don't see any use of Runtime in here. If it doesn't, it should be moved.
There was a problem hiding this comment.
Yes, you're right, that's my failure I used Runtime receiver, I wanted to move filtering logic into separate function.
Let me keep it as separate non-exported function or do you want to put filtering logic into Runtime.GetContainers() function back?
| if options.External { | ||
| generatedExtFunc, err := filters.GenerateExternalContainerFilterFuncs(k, v, runtime) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| filterExtFuncs = append(filterExtFuncs, generatedExtFunc) | ||
| } |
There was a problem hiding this comment.
This will cause regressions, GenerateExternalContainerFilterFuncs() will error on unknown options which means podman ps --external --filter ... will break all other existing filters which does not seem right.
I think design wise it would go long way if we could abstract the same filters for both normal and external container so we do not need the different code handling filters at all.
There was a problem hiding this comment.
This will cause regressions, GenerateExternalContainerFilterFuncs() will error on unknown options which means
podman ps --external --filter ...will break all other existing filters which does not seem right.I think design wise it would go long way if we could abstract the same filters for both normal and external container so we do not need the different code handling filters at all.
I was thinking to implement GenerateFilerFunc as based on generics, however some types which are used in the logic can't be implemented as generics ones.
Do you want me to implement all remaining filters features for external containers?
| @@ -61,6 +61,8 @@ Valid filters are listed below: | |||
| | pod | [Pod] name or full or partial ID of pod | | |||
| | network | [Network] name or full ID of network | | |||
| | until | [DateTime] container created before the given duration or time. | | |||
| | command | [Command] the command the container is executing | | |||
There was a problem hiding this comment.
This should likely highlight that it only matches argv[0] not the full command like some would expect.
There was a problem hiding this comment.
Yes, you are right, I did this commend in the code, however forgot to do it in manual pages.
This PR is intended to implement feature requested in issue #24664
I decided to implement filtering of commands to all kind of containers: for external and for managed by
podman, I guess, this approach is better than just implementing this feature for a single sort of container.If you don't want to accept this approach then I will put it away and implement filtering for external container only.
Does this PR introduce a user-facing change?