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

Move lasso SQL cache in Steve #452

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

tomleb
Copy link
Contributor

@tomleb tomleb commented Jan 16, 2025

The first commit pulls all under lasso's pkg/cache/sql and moves it to pkg/sqlcache from lasso's commit 9e2b68739ccc04fe6c080fc0489391970d77dd29.

**Note: ** This (and backports) need to be merged before rancher/lasso#123.

@tomleb tomleb requested a review from a team as a code owner January 16, 2025 04:48
@tomleb
Copy link
Contributor Author

tomleb commented Jan 16, 2025

@moio Not yet ready, CI failing for a multitude of reasons like linting errors, stuff not compiling, etc. But should give an idea.

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.

All content moved from lasso differs only in paths which is exactly the expectation, so this LGTM overall!

Checked with

kdiff3 pkg/sqlcache ../lasso/pkg/cache/sql

(from a checkout of this PR and lasso main respectively)

There is only one difference I cannot explain as noted in the comment.

Apart from that, this is already good to merge FMPOV!

pkg/sqlcache/informer/listoption_indexer.go Outdated Show resolved Hide resolved
@moio
Copy link
Contributor

moio commented Jan 16, 2025

Added a couple of commits to make stuff compile, tests pass, and lint stop complaining.

@tomleb
Copy link
Contributor Author

tomleb commented Jan 16, 2025

@moio Not yet ready, CI failing for a multitude of reasons like linting errors, stuff not compiling, etc. But should give an idea.

All content moved from lasso differs only in paths which is exactly the expectation, so this LGTM overall!

Checked with

kdiff3 pkg/sqlcache ../lasso/pkg/cache/sql

(from a checkout of this PR and lasso main respectively)

There is only one difference I cannot explain as noted in the comment.

Apart from that, this is already good to merge FMPOV!

It appears I did not in fact base this PR on lasso https://github.com/rancher/lasso/tree/9e2b68739ccc04fe6c080fc0489391970d77dd29 but instead a local fork with an attempt at a fix for rancher/rancher#48453 (which I worked on until I realized it wasn't a blocker for 2.11, woops.

Let me ensure I'm using the right commit this time without local changes. Will force-push shortly.

@tomleb tomleb requested a review from moio January 16, 2025 13:02
@tomleb
Copy link
Contributor Author

tomleb commented Jan 16, 2025

Should be good now:

$ diff -bur pkg/sqlcache ../lasso/pkg/cache/sql 
$

moio
moio previously approved these changes Jan 16, 2025
@tomleb tomleb requested a review from ericpromislow January 16, 2025 14:49
@tomleb tomleb force-pushed the pkg-sqlcache branch 2 times, most recently from 9065bc3 to 5213f7f Compare January 16, 2025 18:13
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Looks fine. Diffs against existing lasso code are superficial

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