Tolerate platform env vars and make Ready() wait for all servers - #91
Merged
Conversation
Ready() used to become readable when the first server started listening, so app.Listener could be observed nil or racy by consumers waiting on Ready() while the memcached server was still starting. Track the servers enabled in the Config and close the ready channel only after all of them are listening. Starting a server without a Config (e.g. Serve) keeps the previous behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
Unparseable values in the non-prefixed environment variables (e.g. PORT or VERSION commonly set by platforms) aborted the startup since invalid values became fatal. Warn and ignore them instead, keeping fail-fast for the KATSUBUSHI_ prefixed names, and never apply environment variables to -version. Port flags also accept a URL like tcp://10.0.0.1:11212 and use its port number, so KATSUBUSHI_PORT injected by Kubernetes/Docker service links works as intended. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: fujiwara <fujiwara.shunichiro@gmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request adjusts startup/config ergonomics and readiness signaling in katsubushi to better tolerate platform-injected environment variables and to make App.Ready() reliable when multiple servers are enabled.
Changes:
- Environment variable handling now fails fast only for
KATSUBUSHI_-prefixed variables; invalid legacy/non-prefixed variables are warned and ignored, and-versionis no longer settable via env. - Port flags (
-port,-http-port,-grpc-port,-debug-port) accepttcp://host:port-style values and extract the port number. Ready()now becomes readable only after all servers enabled in the first-providedConfigare listening; tests updated to cover the multi-server readiness behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents new env-var parsing behavior, -version env restriction, and URL-form port flag support. |
| http.go | Registers required servers from Config and marks HTTP readiness explicitly. |
| grpc.go | Registers required servers from Config and marks gRPC readiness explicitly. |
| config.go | Adds server-name constants and Config.enabledServers() to define readiness requirements. |
| cmd/katsubushi/main.go | Implements portValue flag type, updates env-to-flag semantics, and prevents env-setting -version. |
| cmd/katsubushi/main_test.go | Adds tests for legacy env-var invalid tolerance, URL-form ports, and skipping -version env. |
| app.go | Reworks readiness tracking to wait for all required servers before closing Ready() channel. |
| app_test.go | Adds a test asserting Ready() waits for all configured servers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merged
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.
What
Environment variable handling
PORT,VERSION) are warned and ignored instead of aborting the startup.KATSUBUSHI_prefixed variables still fail fast.-versionis no longer settable via environment variables.-port,-http-port,-grpc-port,-debug-port) accept a URL liketcp://10.0.0.1:11212and use its port number.Ready()
Ready()becomes readable when all the servers enabled in theConfigare listening, instead of the first one. Starting a server without aConfig(e.g.Serve) keeps the previous behavior.Why
The unreleased fail-fast for invalid environment variable values (#89) made harmless platform-injected variables fatal: a generic
VERSIONrefused to start the process, and Kubernetes/Docker service links injectKATSUBUSHI_PORT=tcp://...for a service namedkatsubushi. Accepting the URL form makes the service-link value work as intended.Since #89 made
Ready()fire on the first server, consumers waiting onReady()to readapp.Listenercould observe it nil or race with its assignment when multiple protocol servers run together.🤖 Generated with Claude Code