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

Adopt Secret to pgvector #402

Merged
merged 14 commits into from
Feb 14, 2024
Merged

Adopt Secret to pgvector #402

merged 14 commits into from
Feb 14, 2024

Conversation

davidsbatista
Copy link
Contributor

@davidsbatista davidsbatista commented Feb 13, 2024

PostgreSQL connection string, read from an environment variable with the Secret component:

  • adapted the tests to use the secret component
  • added a custom from_dict method to the doc store class
  • set some methods as staticmethod, since the self was not being used
  • change the coverage config so that it actually shows the coverage over code and not test
  • improved the documentation
  • fixed some typos

@davidsbatista davidsbatista requested a review from a team as a code owner February 13, 2024 08:36
@davidsbatista davidsbatista requested review from anakin87 and removed request for a team February 13, 2024 08:36
@github-actions github-actions bot added the type:documentation Improvements or additions to documentation label Feb 13, 2024
@anakin87
Copy link
Member

Hey! I'm not still an expert on the new Secret management.

I think your implementation should follow in some way that of UptrainEvaluator.

In the current PR, I see that the connection_string is resolved at initialization,
then the to_dict method includes it in serialization, leaking it.

I don't know what the best way to approach this is, and I would leave it to you to experiment/decide.
One possibility is to have a public connection_string that is serialized as Secret, without leaking it and another internal one, _connection_string, that is not serialized/is excluded by the to_dict method.

Let me know what you think and if you need help.

@davidsbatista
Copy link
Contributor Author

thanks for your comments @anakin87 - I did some changes

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Great job!

I left a comment with a doubt. if it makes sense, please consider it.

@davidsbatista davidsbatista merged commit 613e4ec into main Feb 14, 2024
7 checks passed
@davidsbatista davidsbatista deleted the adopt-secret-pgvector branch February 14, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration:pgvector type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants