fix: event --filter volume=vol-name should compare the event name with volume name

[vyasgun]

Fixes: #18618

Does this PR introduce a user-facing change?

Yes

None

[flouthoc]

Not sure but shouldn't it handle additional case for Name, instead of replacing ID with Name

diff --git a/libpod/events/filters.go b/libpod/events/filters.go
index 9057c9b79..5ed09b461 100644
--- a/libpod/events/filters.go
+++ b/libpod/events/filters.go
@@ -52,6 +52,9 @@ func generateEventFilter(filter, filterValue string) (func(e *Event) bool, error
                       if e.Type != Volume {
                               return false
                       }
+                       if e.Name == filterValue {
+                               return true
+                       }
                       return strings.HasPrefix(e.ID, filterValue)
               }, nil
       case "TYPE":

Again I have not tried above patch, just a hunch.

[edsantiago]

I'm not sure what's the best way to fix this, and I'm really uncomfortable with the Prefix thing... but I'm uncomfortablest with NO-TESTS. Here's a test, suitable for 090-events.bats. It passes with your PR, fails on main.

@test "events - volume events" {
    local vname=v$(random_string 10)
    run_podman volume create $vname
    run_podman volume rm $vname

    run_podman events --since=1m --stream=false --filter volume=$vname
    notrunc_results="$output"
    assert "${lines[0]}" =~ ".* volume create $vname"
    assert "${lines[1]}" =~ ".* volume remove $vname"

    # Prefix test
    run_podman events --since=1m --stream=false --filter volume=${vname:0:5}
    assert "$output" = "$notrunc_results"
}

[vyasgun]

@flouthoc The ID is not being set in the code for volume events so it is always blank. In the pod and image cases also, the name is being used for comparison before ID.

[vyasgun]

@edsantiago @flouthoc The prefix check is being used in all other cases but with IDs. I don't see an ID field in the Volume type so I added it for the Name field but it sounds better to change it to a == check instead.

Thanks for the feedback, I'll add some tests.

[flouthoc]

@vyasgun

If two volumes are created with name

* volume1: hello
* volume2: helloworld

Wouldn't filter --filter=hello will filter event both for volume hello and helloworld ?

My idea with the patch suggested here #18619 (review) was if user specifies a Name it should always do explicit match and if ID is selected then maybe fallback to prefix match. Since docs says either ID or Name https://docs.podman.io/en/latest/markdown/podman-events.1.html

But again I have not played with filter and the part of code being discussed here enough so I could be missing bits.

[flouthoc]

Ignore my previous comment it seems libpod.Volumes don't have an ID, they only have Name so maybe

diff --git a/libpod/events/filters.go b/libpod/events/filters.go
index 9057c9b79..4a836d817 100644
--- a/libpod/events/filters.go
+++ b/libpod/events/filters.go
@@ -52,7 +52,10 @@ func generateEventFilter(filter, filterValue string) (func(e *Event) bool, error
                        if e.Type != Volume {
                                return false
                        }
-                       return strings.HasPrefix(e.ID, filterValue)
+                       if e.Name == filterValue {
+                               return true
+                       }
+                       return false
                }, nil
        case "TYPE":
                return func(e *Event) bool {

[vyasgun]

@flouthoc I have changed it to == in my recent commit.

[flouthoc]

LGTM other than adding tests as suggested by @edsantiago

[vrothberg]

Let's also record the id here id=$(output) and filter it as well.

[vrothberg]

@flouthoc The ID is not being set in the code for volume events so it is always blank. In the pod and image cases also, the name is being used for comparison before ID.

Ah, OK. It looks like name and ID are the same.

How does Docker behave when filtering for name or ID?

@vyasgun can you squash the two commits into one in the next push?

[vyasgun]

@vrothberg docker is doing a prefix match when filtering for "volume=volume-name".

$ docker events --filter volume=abc
2023-05-22T18:10:39.330449408+05:30 volume destroy abcd (driver=local)

In Podman, the ID is not being set for volume events since the volume object does not have an ID. Should it be set to the volume name? Or is it better to leave it blank?

[vrothberg]

In Podman, the ID is not being set for volume events since the volume object does not have an ID. Should it be set to the volume name? Or is it better to leave it blank?

We can leave it blank but we need to be compatible with Docker. So I think we need to keep the prefix check but compare with .Name isntead of .ID.

[vrothberg]

LGTM, thanks!

[TomSweeneyRedHat]

LGTM
I'll rerun the failing test, it looks like a flake

[vrothberg]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg, vyasgun

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:

• OWNERS [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment