Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for transform functions #79

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

MbolotSuse
Copy link
Contributor

Related to one of the tasks from rancher/rancher#45635. Adds the option for a TransformFunc which will be can be passed in when the Cache is created.

@MbolotSuse MbolotSuse requested a review from a team as a code owner June 13, 2024 15:10
@MbolotSuse MbolotSuse requested a review from tomleb June 13, 2024 15:10
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any tests that uses a TransformFunc. Can that be added so we have some confidence that we're calling SetTransform, etc?

moio
moio previously approved these changes Aug 20, 2024
Copy link
Contributor

@moio moio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @tomleb: this is good to merge as soon as at least one test touches the newly added parameter.

@MbolotSuse
Copy link
Contributor Author

@tomleb / @moio I'll see what I can do. Right now the tests are a bit mock-reliant, and since we can't test functions for equality (see issues like: golang/mock#324) there may be a limit to what I can do.

@MbolotSuse
Copy link
Contributor Author

@moio / @tomleb I gave it a try. I had to do a few sub-optimal/hacky things:

  • I had to move the cached.SharedIndexInformer call to a "newInformer" call.
    • The SharedInformer seems to have a method to set the transformer, but no method to get the transformer afterward. So it was either this or do some sort of unsafe get to pull the method out after create.
  • I can't use mockgen to create a SharedIndexInformer since we use UnsafeSet to override the indexer (which won't exist on a mock).
    • This required that I manually make quick mock to save the transform function
  • Since I can't (afaik) compare function equality, I need to call the function that ends up stored on the mock informer to make sure that it's the same function that we set before.

Please take a look and let me know if you have some other ideas (or if you would prefer that I not add the tests in favor of the first version of the code).

@MbolotSuse MbolotSuse requested review from tomleb and moio August 26, 2024 21:46
moio
moio previously approved these changes Aug 27, 2024
pkg/cache/sql/informer/informer.go Show resolved Hide resolved
Adds support for a transform function, which will edit an object before
it is stored in the cache. Used primarily to add abitrary new fields that
can be filtered/sorted on.
@MbolotSuse MbolotSuse merged commit d79536c into rancher:master Aug 28, 2024
1 check passed
moio added a commit to moio/rancher that referenced this pull request Oct 2, 2024
This includes various fixes to the UI Server-Side Pagination experimental
feature:

https://ranchermanager.docs.rancher.com/how-to-guides/advanced-user-guides/enable-experimental-features/ui-server-side-pagination

- rancher/lasso#86
- rancher/lasso#90
- rancher/lasso#85
- rancher/lasso#92
- rancher/lasso#79
- rancher/lasso#97
- rancher/lasso#94
- rancher/lasso#98
- rancher/lasso#108
- rancher/lasso#99

It also includes improvements to code collecting metrics:
- rancher/lasso#89
- rancher/lasso#95

And to test code:
- rancher/lasso#100
- rancher/lasso#101

Signed-off-by: Silvio Moioli <silvio@moioli.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants