-
-
Notifications
You must be signed in to change notification settings - Fork 102
feat: add support for s3vectors resources #775
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
base: main
Are you sure you want to change the base?
Conversation
c7f8113 to
75225f7
Compare
75225f7 to
f43a11e
Compare
|
Thanks for the contribution. The only thing I think I'm on the fence about is if it should be |
| Resource: &S3VectorsBucket{}, | ||
| Lister: &S3VectorsBucketLister{}, | ||
| DependsOn: []string{ | ||
| S3VectorsIndexResource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the hierarchy implemented:
S3VectorsBucket depends on S3VectorsIndex, which depends on S3VectorsVector.
Is it possible for an S3VectorsVector to exist without an S3VectorsIndex?
If so, the Bucket should also depend on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also say that the dependency hierarchy is definitely experimental and a bit buggy, it can cause some stalled removes. Love that you are using it, or might be, would appreciate any feedback to issues you encounter.
|
I took some time off that is why this has sat. I'll run my tests locally and then get it merged soon. Thanks. |
|
Please rebase. |
S3vectorsBucket,S3vectorsIndexandS3vectorsVectorfixes: #777