Make cel2sql generic using View abstraction #495
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/XL
tekton-robot
added
the
needs-rebase
There was a problem hiding this comment.
I didn't understand why exactly these tests are exercising the conversions if, in practice, they aren't being performed by the records.go file. This seems counterintuitive for collaborators.
There was a problem hiding this comment.
I've initially moved to this file to avoid a cyclic dependency..
I've changed to different approach by having a test view that has all the fields that we need to test
| "github.com/google/go-cmp/cmp" | ||
| ) | ||
|
|
||
| func TestConvertRecordExpressions(t *testing.T) { |
There was a problem hiding this comment.
As mentioned previously, I recommend keeping these tests here instead of moving them to a different package.
|
/kind misc |
|
@RafaeLeal I left a few comments mostly about style and some parts of the code that I'd prefer to leave as before (tests, for example). You need to rebase the pr and write down release notes. |
tekton-robot
removed
the
needs-rebase
There was a problem hiding this comment.
/approve
The intent here is good - this lays a good foundation for making cel2sql an external library.
I recommend that the changes to convert_test.go be reverted, and replaced with only the necessary refactorings to convert the existing tests to use the new View type. The current set of changes make it difficult to verify that our test logic remains in place (and this PR doesn't break anything).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan 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 |
tekton-robot
added
the
approved
| ) | ||
|
|
||
| func TestConvertRecordExpressions(t *testing.T) { | ||
| func newTestView() *View { |
There was a problem hiding this comment.
Couldn't we use the actual view rather than building a new one for testing? Asking because by doing so, we might forget to configure the actual view properly and do that only in the test one or vice-versa and ultimately, we might end up not exercising the actual interpreter's logic.
| name: "contains string function", | ||
| in: `data.metadata.name.contains("foo")`, | ||
| want: "POSITION('foo' IN (data->'metadata'->>'name')) <> 0", | ||
| name: "able to match strings exactly", |
There was a problem hiding this comment.
I think that this "able to..." sounds redundant in the test descriptions. I'd recommend keeping the descriptions in the original style.
tekton-robot
added
the
needs-rebase
|
@RafaeLeal: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Issues go stale after 90d of inactivity. /lifecycle stale Send feedback to tektoncd/plumbing. |
Changes
This PR makes the cel2sql package more generic, introducing a new
Viewabstraction. This allows us to decouple what we expose for the Results' APIfiltersand the database structure.I believe it also makes changes in our database safer since we don't have to think of all kinds of filters that would use each column of the db table. The idea is to make the changes and support in the CEL
filtersmore deliberate.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes