-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: add Ollama API key support for Turbo mode #7425
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
Conversation
- Add ollamaApiKey field to ProviderSettings schema - Add ollamaApiKey to SECRET_STATE_KEYS for secure storage - Update Ollama and NativeOllama providers to use API key for authentication - Add UI field for Ollama API key (shown when custom base URL is provided) - Add test coverage for API key functionality This enables users to use Ollama Turbo with datacenter-grade hardware by providing an API key for authenticated Ollama instances or cloud services.
Remove non-existent ApiKeyField import and use standard VSCodeTextField with password type, matching other provider implementations
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.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
| onInput={handleInputChange("ollamaApiKey")} | ||
| placeholder={t("settings:placeholders.apiKey")} | ||
| className="w-full"> | ||
| <label className="block font-medium mb-1">{t("settings:providers.ollama.apiKey")}</label> |
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.
Critical issue: The translation keys and referenced here don't exist in the translation files. This will cause the UI to display missing translation keys instead of proper text.
You need to add these keys to and all other locale files:
| ollamaBaseUrl: "https://ollama.com", | ||
| ollamaApiKey: "test-api-key", | ||
| }) | ||
| expect(handlerWithApiKey).toBeInstanceOf(OllamaHandler) |
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 test verifies that the handler can be instantiated with an API key, but doesn't actually test that the API key is used in the Authorization header. Could we add an assertion to verify the header is properly set?
Consider mocking the OpenAI constructor to capture and verify the headers:
src/api/providers/native-ollama.ts
Outdated
| if (!this.client) { | ||
| try { | ||
| this.client = new Ollama({ | ||
| const clientOptions: any = { |
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.
Using type here bypasses TypeScript's type checking. Is this intentional? The Ollama client likely has a proper options interface we could use instead:
This would give us proper type safety while still allowing dynamic property assignment.
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.
solved
|
|
||
| const headers: Record<string, string> = {} | ||
| if (this.options.ollamaApiKey) { | ||
| headers["Authorization"] = `Bearer ${this.options.ollamaApiKey}` |
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.
Is passing the API key in both and necessary? The OpenAI client typically handles authentication via the parameter. The Authorization header in might be redundant unless Ollama's OpenAI-compatible endpoint specifically requires it.
Could we test if just using without the works?
| className="w-full"> | ||
| <label className="block font-medium mb-1">{t("settings:providers.ollama.baseUrl")}</label> | ||
| </VSCodeTextField> | ||
| {apiConfiguration?.ollamaBaseUrl && ( |
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 conditional rendering logic here is clever - only showing the API key field when a custom base URL is provided. Consider adding a comment explaining this UX decision to help future maintainers:
- Add providers.ollama.apiKey and providers.ollama.apiKeyHelp to all 18 language files - Support for authenticated Ollama instances and cloud services - Relates to PR #7425
- Replace 'any' type with proper OllamaOptions (Config) type - Import Config type from ollama package for better type checking
daniel-lxs
left a comment
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.
LGTM
This PR implements support for Ollama Turbo mode by adding API key authentication, following the approach used in Cline PR #5400.
Summary
Enables users to utilize Ollama Turbo's datacenter-grade hardware execution from Roo Code by providing an API key for authenticated Ollama instances or cloud services.
Changes
ollamaApiKeyfield to ProviderSettings schemaollamaApiKeyto SECRET_STATE_KEYS for secure storageImplementation Details
Testing
Related Issue
Fixes #7147
References
cc @daniel-lxs
Important
Adds API key support for Ollama Turbo mode, updating provider settings and UI, while ensuring backward compatibility and adding test coverage.
ollamaApiKeyfield toProviderSettingsschema andSECRET_STATE_KEYSfor secure storage.OllamaandNativeOllamaproviders to use API key for authentication via Bearer token.ollama.spec.ts.This description was created by
for 00628f8. You can customize this summary. It will automatically update as commits are pushed.