feat: honour LLMMAN_HOST and document using Gollama with llmman - #239
Open
ericcurtin wants to merge 1 commit into
Open
ericcurtin wants to merge 1 commit into
ericcurtin wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A precedence change was introduced but isn’t fully covered by tests (and the README wording should reflect the full env-var precedence).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes Gollama easier to use with llmman by adding a new environment-variable fallback for the API host and documenting the intended workflow, while preserving existing Ollama precedence and defaults.
Changes:
- Update API URL resolution to fall back to
LLMMAN_HOSTwhenOLLAMA_HOSTis unset (afterOLLAMA_API_URL). - Add a
TestLoadConfigcase coveringLLMMAN_HOST. - Document how to point Gollama at llmman and which features/endpoints are expected to work.
File summaries
| File | Description |
|---|---|
config/config.go |
Adds LLMMAN_HOST fallback in getAPIUrl() while keeping OLLAMA_API_URL / OLLAMA_HOST precedence. |
config/config_test.go |
Adds a new LLMMAN_HOST test case and ensures env cleanup includes LLMMAN_HOST. |
README.md |
Documents running Gollama against llmman via -h or LLMMAN_HOST, plus known limitations. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+156
to
157
| }, | ||
| { |
|
|
||
| #### Using with llmman | ||
|
|
||
| [llmman](https://github.com/llmmanorg/llmman) is a local model runner that serves the Ollama API (alongside OpenAI- and Anthropic-compatible ones) on port 17434. Point Gollama at it with `-h`, or set `LLMMAN_HOST` (used when `OLLAMA_HOST` is not set): |
| for _, env := range []string{"OLLAMA_HOST", "LLMMAN_HOST"} { | ||
| if host := os.Getenv(env); host != "" { | ||
| // Check if the host already starts with http:// or https:// | ||
| if len(host) >= 7 && host[:7] == "http://" || len(host) >= 8 && host[:8] == "https://" { |
llmman (https://github.com/llmmanorg/llmman) serves the Ollama API on port 17434, so Gollama already works against it when given the URL. Fall back to LLMMAN_HOST when OLLAMA_HOST is unset so llmman-only users need not set Ollama's variable or pass -h. OLLAMA_HOST keeps precedence. Add a config test and a short README section noting what still needs Ollama (run via the ollama CLI, Modelfile editing, --ollama-dir).
There was a problem hiding this comment.
🟢 Approval recommended
Changes are small, backward-compatible, and include a targeted test case for the new LLMMAN_HOST behavior.
Review details
Suppressed comments (2)
config/config.go:64
- The scheme check relies on operator precedence; adding parentheses makes the intent unambiguous and reduces risk of accidental logic changes during future edits.
if len(host) >= 7 && host[:7] == "http://" || len(host) >= 8 && host[:8] == "https://" {
README.md:178
- This sentence says LLMMAN_HOST is used when OLLAMA_HOST is not set, but OLLAMA_API_URL also takes precedence; consider mentioning both to avoid confusing readers who already use OLLAMA_API_URL.
[llmman](https://github.com/llmmanorg/llmman) is a local model runner that serves the Ollama API (alongside OpenAI- and Anthropic-compatible ones) on port 17434. Point Gollama at it with `-h`, or set `LLMMAN_HOST` (used when `OLLAMA_HOST` is not set):
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
55
to
+56
| // getAPIUrl determines the API URL based on environment variables. | ||
| // OLLAMA_HOST takes precedence over LLMMAN_HOST (llmman serves the Ollama API on 17434). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
llmman is a local model runner that serves the Ollama API on port 17434; Gollama already works against it when pointed at that URL.
config/config.go:getAPIUrl()falls back toLLMMAN_HOSTwhenOLLAMA_HOSTis unset (same scheme handling, folded into a two-entry loop).OLLAMA_API_URLandOLLAMA_HOSTkeep precedence; default URL unchanged.config/config_test.go: newTestLoadConfigcase forLLMMAN_HOST; env var is unset before each case.README.md: short "Using with llmman" section, notingEnter(run) shells out to theollamaCLI and that Modelfile editing and--ollama-dirrely on Ollama's store.Testing:
gofmt -l .,go build ./...,go vet ./...,go test ./...all pass, including the newLLMMAN_HOSTcase.