tektoncd / results

Long term storage of execution results.
Apache License 2.0
78 stars 74 forks source link

CEL Filter not working with tkn-result #569

Closed khrm closed 1 year ago

khrm commented 1 year ago

Running below command fails: `./tkn-results list default --filter="summary.status=='SUCCESS'"

Expected Behavior

ListResults: rpc error: code = InvalidArgument desc = error compiling CEL filters: ERROR: :1:15: found no matching overload for '==' applied to '(int, string)' | summary.status=='SUCCESS' | ..............^ Error: rpc error: code = InvalidArgument desc = error compiling CEL filters: ERROR: :1:15: found no matching overload for '==' applied to '(int, string)' | summary.status=='SUCCESS' | ..............^

/cc @avinal

khrm commented 1 year ago

@avinal Let's continue our discussion here regarding broken cel filter.

avinal commented 1 year ago

Note: this particular expression was working before a few updates. I am unable to pinpoint where it changed. Furthermore, only this expression is failing. All other filters I tested work fine. Somehow, the summary.status field is interpreted as int.

khrm commented 1 year ago

The issue might not be with the tkn-results, then. It might be with CEL-related changes in API.

khrm commented 1 year ago

These are the two PRs that we need to test for this error by running -1 commit:

544

495

avinal commented 1 year ago

I would also widen the subject from just tkn-results to REST and gRPC as well, I checked, it doesn't work there either. Yes this seems to be a CEL change.

avinal commented 1 year ago

/assign

alan-ghelardi commented 1 year ago

Yes, the summary.status is an integer because it is declared like this in the proto definition and mapped to an integer in the database. When I redesigned the search mechanism to support an interpreter from CEL to SQL, one of the things that I tried to do was preserving the types that people see when viewing the wire types in the search. I believe that this reduces cognitive load and the continuous mental mapping between things - if the summary.status is an integer I don't need to remember of referencing it as a string when searching.

In addition, there're a few constants to help people to reference the enum values instead of using the numeric values (the constants are the string representation of the enums in the proto format). Could you try the following instead? ./tkn-results list default --filter="summary.status==SUCCESS"

avinal commented 1 year ago

@alan-ghelardi that makes sense. I tested it and it works fine without strings. I will close this issue and update this detail in docs. Thank You

avinal commented 1 year ago

/close

tekton-robot commented 1 year ago

@avinal: You can't close an active issue/PR unless you authored it or you are a collaborator.

In response to [this](https://github.com/tektoncd/results/issues/569#issuecomment-1704948517): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
avinal commented 1 year ago

I must have messed up while testing and used quotes. I have added a clear paragraph to avoid this mistake by others.

khrm commented 1 year ago

/close Fixed