-
Notifications
You must be signed in to change notification settings - Fork 218
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
[OPIK-548] List LLM Provider api keys #864
[OPIK-548] List LLM Provider api keys #864
Conversation
} | ||
|
||
public static String encrypt(String data) { | ||
init(); | ||
try { |
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.
As key
is now nullable, I suggest validating it's not null to avoid NPE
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.
We set key at the application boot-up, so it will never be null.
I would rather not add additional checks here, since this scenario is not feasible. If you insist I could add it, but seems unnecassary.
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.
the annotations also serve as documentation and to guarantee new usage will respect the constraints. Basically all public methods should have 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.
@thiagohora I added annotations to public methods. But it's not related to what Ido is concerned. key
is a private field and is not a part of public api.
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionUtils.java
Show resolved
Hide resolved
...ackend/src/test/java/com/comet/opik/api/resources/v1/priv/LlmProviderApiKeyResourceTest.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/LlmProviderApiKeyService.java
Show resolved
Hide resolved
assertPage(actualProviderApiKeyPage, List.of()); | ||
|
||
// Create LLM Provider api key | ||
var expectedProviderApiKey = llmProviderApiKeyResourceClient.createProviderApiKey(providerApiKey, apiKey, |
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 think that in the case of list logic, it's worth to create several api keys
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.
It's impossible for now:) Requirements are that one workspace can have only 1 key per provider. And now we are going to support only 1 provider.
...ackend/src/test/java/com/comet/opik/api/resources/v1/priv/LlmProviderApiKeyResourceTest.java
Outdated
Show resolved
Hide resolved
d29484c
to
885b30c
Compare
99c547f
to
4aa28c9
Compare
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionUtils.java
Outdated
Show resolved
Hide resolved
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.
Great job!
Details
List LLM Provider api keys
Testing
Added corresponding integration tests
Documentation
https://www.notion.so/cometml/Online-Scoring-HLD-14d7124010a380508f32fdbd541e5705