From f9e5b1ae70105c902e401a050dede79b939be4ad Mon Sep 17 00:00:00 2001 From: Etai Lev Ran Date: Wed, 8 Jul 2026 14:35:48 +0300 Subject: [PATCH 1/2] config: split ServerOptions into CharonOptions and ProxyOptions (#74, #75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the single config.ServerOptions with two focused types: CharonOptions – listen, storage (data dir, TTL, max-*, chain depth), worker interval, telemetry (service name + exporter URL) ProxyOptions – listen, inference backend/apikey/timeout, charon URL, telemetry (service name + exporter URL) Each type has its own New*, AddFlags, Complete, and Validate. The YAML config file structure (proxy.*, charon.*, telemetry.*) is unchanged so existing config files continue to work. Migrate cmd/charon and cmd/proxy to use their respective types. Remove the grab-bag ServerOptions and the TODO comment it left behind. Closes #74 Resolves TODO from #75 (config split task) Signed-off-by: Etai Lev Ran --- cmd/charon/main.go | 8 +- cmd/charon/main_test.go | 5 +- cmd/proxy/main.go | 16 ++- internal/config/bytesize_test.go | 2 +- internal/config/config_test.go | 65 ++++++---- internal/config/options.go | 207 +++++++++++++++++++------------ internal/config/options_test.go | 110 ++++++++++------ 7 files changed, 250 insertions(+), 163 deletions(-) diff --git a/cmd/charon/main.go b/cmd/charon/main.go index 8e229c0..fe9b25f 100644 --- a/cmd/charon/main.go +++ b/cmd/charon/main.go @@ -29,7 +29,7 @@ func main() { func run() error { log := slog.New(slog.NewJSONHandler(os.Stdout, nil)) - opts := config.NewServerOptions() + opts := config.NewCharonOptions() fs := flag.NewFlagSet("charon", flag.ExitOnError) opts.AddFlags(fs) _ = fs.Parse(os.Args[1:]) @@ -64,7 +64,7 @@ func run() error { } defer func() { _ = svc.Close() }() - tp, err := telemetry.Init(context.Background(), opts.Telemetry.CharonService, opts.Telemetry.ExporterURL) + tp, err := telemetry.Init(context.Background(), opts.Telemetry.ServiceName, opts.Telemetry.ExporterURL) if err != nil { log.Error("init tracer", "err", err) return err @@ -78,10 +78,10 @@ func run() error { } h := server.NewHandler(svc, log).WithMaxBodyBytes(int64(opts.MaxPayload)) - srv := server.NewServerWithRegistry(opts.CharonListen, h, log, reg, tp) + srv := server.NewServerWithRegistry(opts.Listen, h, log, reg, tp) go func() { - log.Info("starting charon", "addr", opts.CharonListen) + log.Info("starting charon", "addr", opts.Listen) if err := srv.Start(); err != nil && err != http.ErrServerClosed { log.Error("charon server error", "err", err) } diff --git a/cmd/charon/main_test.go b/cmd/charon/main_test.go index 713816c..0821a61 100644 --- a/cmd/charon/main_test.go +++ b/cmd/charon/main_test.go @@ -10,9 +10,8 @@ import ( ) func TestConfigDefaults(t *testing.T) { - opts := config.NewServerOptions() - require.False(t, opts.ProxyEnabled) - require.Equal(t, ":8080", opts.ProxyListen) + opts := config.NewCharonOptions() + require.Equal(t, ":8081", opts.Listen) require.Equal(t, 30, opts.TTLDays) } diff --git a/cmd/proxy/main.go b/cmd/proxy/main.go index 1cd2e2e..d9fdab3 100644 --- a/cmd/proxy/main.go +++ b/cmd/proxy/main.go @@ -26,10 +26,8 @@ func main() { func run() error { log := slog.New(slog.NewJSONHandler(os.Stdout, nil)) - opts := config.NewServerOptions() + opts := config.NewProxyOptions() fs := flag.NewFlagSet("proxy", flag.ExitOnError) - // TODO: split config.ServerOptions into CharonOptions + ProxyOptions so this - // binary doesn't silently accept (and ignore) Charon storage/TTL flags. opts.AddFlags(fs) _ = fs.Parse(os.Args[1:]) if err := opts.Complete(fs); err != nil { @@ -41,7 +39,7 @@ func run() error { return err } - tp, err := telemetry.Init(context.Background(), opts.Telemetry.ProxyService, opts.Telemetry.ExporterURL) + tp, err := telemetry.Init(context.Background(), opts.Telemetry.ServiceName, opts.Telemetry.ExporterURL) if err != nil { log.Error("init tracer", "err", err) return err @@ -54,17 +52,17 @@ func run() error { }() } - timeout := time.Duration(opts.InferenceTimeoutSeconds) * time.Second - infClient := inference.New(opts.ProxyBackend, opts.ProxyAPIKey, timeout) - charonClient := charon.New(opts.ProxyCharonURL, timeout) + timeout := time.Duration(opts.TimeoutSeconds) * time.Second + infClient := inference.New(opts.Backend, opts.APIKey, timeout) + charonClient := charon.New(opts.CharonURL, timeout) h := NewHandler(charonClient, infClient, log) mux := http.NewServeMux() RegisterHandlers(mux, h) - srv := server.NewServerFromMux(opts.ProxyListen, mux, log, tp) + srv := server.NewServerFromMux(opts.Listen, mux, log, tp) go func() { - log.Info("starting proxy", "addr", opts.ProxyListen) + log.Info("starting proxy", "addr", opts.Listen) if err := srv.Start(); err != nil && err != http.ErrServerClosed { log.Error("proxy server error", "err", err) } diff --git a/internal/config/bytesize_test.go b/internal/config/bytesize_test.go index b935e20..ddc70b0 100644 --- a/internal/config/bytesize_test.go +++ b/internal/config/bytesize_test.go @@ -79,7 +79,7 @@ func TestByteSizeUnmarshalErrors(t *testing.T) { } func TestByteSizeInConfig(t *testing.T) { - opts := config.NewServerOptions() + opts := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) opts.AddFlags(fs) require.NoError(t, fs.Parse([]string{"--config", "testdata/bytesize.yaml"})) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7ddaa59..8e192ca 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -12,53 +12,64 @@ import ( ) func TestLoadDefaults(t *testing.T) { - opts := config.NewServerOptions() + charon := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) - opts.AddFlags(fs) + charon.AddFlags(fs) require.NoError(t, fs.Parse(nil)) - require.NoError(t, opts.Complete(fs)) + require.NoError(t, charon.Complete(fs)) - // Proxy defaults - assert.False(t, opts.ProxyEnabled, "proxy must be off by default") - assert.Equal(t, ":8080", opts.ProxyListen) - assert.Equal(t, "http://127.0.0.1:8081", opts.ProxyCharonURL) - assert.Equal(t, "http://localhost:11434", opts.ProxyBackend) - assert.Equal(t, 120, opts.InferenceTimeoutSeconds) - // Charon defaults - assert.Equal(t, ":8081", opts.CharonListen) - assert.Equal(t, "./data", opts.DataDir) - assert.Equal(t, 30, opts.TTLDays) - assert.Equal(t, time.Hour, opts.WorkerTTLInterval) + assert.Equal(t, ":8081", charon.Listen) + assert.Equal(t, "./data", charon.DataDir) + assert.Equal(t, 30, charon.TTLDays) + assert.Equal(t, time.Hour, charon.WorkerTTLInterval) + assert.Equal(t, "charon", charon.Telemetry.ServiceName) + + proxy := config.NewProxyOptions() + fs2 := flag.NewFlagSet("test", flag.ContinueOnError) + proxy.AddFlags(fs2) + require.NoError(t, fs2.Parse(nil)) + require.NoError(t, proxy.Complete(fs2)) + + assert.Equal(t, ":8080", proxy.Listen) + assert.Equal(t, "http://localhost:11434", proxy.Backend) + assert.Equal(t, "http://127.0.0.1:8081", proxy.CharonURL) + assert.Equal(t, 120, proxy.TimeoutSeconds) + assert.Equal(t, "charon-proxy", proxy.Telemetry.ServiceName) } func TestCharonURLDerivedFromListen(t *testing.T) { // When charon.listen is set in the file and proxy.charon_url is not, - // ProxyCharonURL is auto-derived from the file's charon.listen. - opts := config.NewServerOptions() + // CharonURL is auto-derived from the file's charon.listen. + proxy := config.NewProxyOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) - opts.AddFlags(fs) + proxy.AddFlags(fs) require.NoError(t, fs.Parse([]string{"--config", "testdata/config.yaml"})) - require.NoError(t, opts.Complete(fs)) - assert.Equal(t, "http://127.0.0.1:9090", opts.ProxyCharonURL) + require.NoError(t, proxy.Complete(fs)) + assert.Equal(t, "http://127.0.0.1:9090", proxy.CharonURL) } func TestLoadFromFile(t *testing.T) { - opts := config.NewServerOptions() + proxy := config.NewProxyOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) - opts.AddFlags(fs) + proxy.AddFlags(fs) require.NoError(t, fs.Parse([]string{"--config", "testdata/config.yaml"})) - require.NoError(t, opts.Complete(fs)) + require.NoError(t, proxy.Complete(fs)) + assert.Equal(t, ":0", proxy.Listen) - assert.Equal(t, ":0", opts.ProxyListen) - assert.Equal(t, 30, opts.TTLDays) + charon := config.NewCharonOptions() + fs2 := flag.NewFlagSet("test", flag.ContinueOnError) + charon.AddFlags(fs2) + require.NoError(t, fs2.Parse([]string{"--config", "testdata/config.yaml"})) + require.NoError(t, charon.Complete(fs2)) + assert.Equal(t, 30, charon.TTLDays) } func TestLoadStrictRejectsUnknownFields(t *testing.T) { - opts := config.NewServerOptions() + charon := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) - opts.AddFlags(fs) + charon.AddFlags(fs) require.NoError(t, fs.Parse([]string{"--config", "testdata/invalid.yaml"})) - err := opts.Complete(fs) + err := charon.Complete(fs) require.Error(t, err) assert.Contains(t, err.Error(), "parse config") } diff --git a/internal/config/options.go b/internal/config/options.go index de2580d..3b5c1ad 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -107,25 +107,29 @@ func loadFileConfig(path string) (fileConfig, error) { return fc, nil } -// ServerOptions holds the full configuration for the charon server command. -type ServerOptions struct { - // Config file path — set by --config flag. - ConfigFile string +// --------------------------------------------------------------------------- +// TelemetryOptions (shared) +// --------------------------------------------------------------------------- + +// TelemetryOptions holds OpenTelemetry settings. +type TelemetryOptions struct { + ExporterURL string // OTLP HTTP endpoint; empty = disabled + ServiceName string // identifies this binary in traces +} - // Proxy settings. - ProxyEnabled bool - ProxyListen string - ProxyBackend string // inference base URL - ProxyAPIKey string - ProxyCharonURL string +// --------------------------------------------------------------------------- +// CharonOptions +// --------------------------------------------------------------------------- - // Inference sub-settings (config-file only — deep tuning knobs). - InferenceTimeoutSeconds int +// CharonOptions holds configuration for the Charon response-storage server. +type CharonOptions struct { + // Config file path — set by --config flag. + ConfigFile string - // Charon internal API. - CharonListen string + // Listen address for the Charon internal API. + Listen string - // Storage. + // Storage settings. DataDir string // TTLDays is the maximum age of a stored response. Maps to chainstore.Config.TTL. TTLDays int @@ -135,58 +139,34 @@ type ServerOptions struct { MaxContextBytes ByteSize // WorkerTTLInterval is how often the background TTL reaper runs (not the TTL itself). - // Maps to chainstore.Config.TTLInterval. WorkerTTLInterval time.Duration - // Telemetry settings. Telemetry TelemetryOptions } -// TelemetryOptions holds OpenTelemetry settings for the server. -type TelemetryOptions struct { - ExporterURL string // OTLP HTTP endpoint; empty = disabled - CharonService string // default "charon" - ProxyService string // default "charon-proxy" -} - -// NewServerOptions returns a ServerOptions pre-filled with built-in defaults. -func NewServerOptions() *ServerOptions { - return &ServerOptions{ - ProxyListen: ":8080", - ProxyBackend: "http://localhost:11434", - InferenceTimeoutSeconds: 120, - CharonListen: ":8081", - DataDir: "./data", - TTLDays: 30, - WorkerTTLInterval: time.Hour, - Telemetry: TelemetryOptions{ - CharonService: "charon", - ProxyService: "charon-proxy", - }, +// NewCharonOptions returns a CharonOptions pre-filled with built-in defaults. +func NewCharonOptions() *CharonOptions { + return &CharonOptions{ + Listen: ":8081", + DataDir: "./data", + TTLDays: 30, + WorkerTTLInterval: time.Hour, + Telemetry: TelemetryOptions{ServiceName: "charon"}, } } -// AddFlags registers CLI flags on fs. Only commonly-overridden options are -// exposed as flags; deep tuning knobs remain config-file-only. -func (o *ServerOptions) AddFlags(fs *flag.FlagSet) { +// AddFlags registers CLI flags on fs for the Charon server. +func (o *CharonOptions) AddFlags(fs *flag.FlagSet) { fs.StringVar(&o.ConfigFile, "config", "", "path to config file") - fs.StringVar(&o.CharonListen, "listen", o.CharonListen, "charon internal API listen address") - fs.StringVar(&o.ProxyListen, "proxy-listen", o.ProxyListen, "proxy server listen address") - fs.BoolVar(&o.ProxyEnabled, "proxy", o.ProxyEnabled, "enable the proxy layer") - fs.StringVar(&o.ProxyBackend, "backend", o.ProxyBackend, "inference backend base URL") + fs.StringVar(&o.Listen, "listen", o.Listen, "charon internal API listen address") fs.StringVar(&o.DataDir, "storage-data-dir", o.DataDir, "data directory for Pebble storage") fs.StringVar(&o.Telemetry.ExporterURL, "telemetry-exporter-url", o.Telemetry.ExporterURL, "OTLP HTTP exporter URL (e.g. http://localhost:4318); empty disables tracing") } // Complete loads the config file (if --config was set) and merges file values // into the options struct. CLI flags take precedence over config-file values. -// Complete also resolves derived values such as ProxyCharonURL. -func (o *ServerOptions) Complete(fs *flag.FlagSet) error { +func (o *CharonOptions) Complete(fs *flag.FlagSet) error { if o.ConfigFile == "" { - // No config file — derive ProxyCharonURL from built-in defaults and return. - if o.ProxyCharonURL == "" { - o.ProxyCharonURL = deriveCharonURL(o.CharonListen) - } return nil } @@ -195,33 +175,17 @@ func (o *ServerOptions) Complete(fs *flag.FlagSet) error { return err } - // Collect which flags were explicitly set on the command line. setFlags := make(map[string]bool) fs.Visit(func(f *flag.Flag) { setFlags[f.Name] = true }) - // Merge file values into options, skipping fields covered by explicit flags. - if !setFlags["proxy"] { - o.ProxyEnabled = fc.Proxy.Enabled - } - if !setFlags["proxy-listen"] { - o.ProxyListen = fc.Proxy.Listen - } - if !setFlags["backend"] { - o.ProxyBackend = fc.Proxy.Inference.BaseURL - } - // Always take API key from file (not a CLI flag). - o.ProxyAPIKey = fc.Proxy.Inference.APIKey - // Deep inference knobs are config-file-only. - o.InferenceTimeoutSeconds = fc.Proxy.Inference.TimeoutSeconds - if !setFlags["listen"] { - o.CharonListen = fc.Charon.Listen + o.Listen = fc.Charon.Listen } if !setFlags["storage-data-dir"] { o.DataDir = fc.Charon.Storage.DataDir } - // All remaining storage fields are config-file-only. + // Storage fields are config-file-only. o.TTLDays = fc.Charon.Storage.TTLDays o.MaxResponses = fc.Charon.Storage.MaxResponses o.MaxPayload = fc.Charon.Storage.MaxPayload @@ -231,27 +195,108 @@ func (o *ServerOptions) Complete(fs *flag.FlagSet) error { // Worker settings are config-file-only. o.WorkerTTLInterval = fc.Charon.Workers.TTLInterval - // Derive ProxyCharonURL: file value takes precedence, then auto-derive. - if fc.Proxy.CharonURL != "" { - o.ProxyCharonURL = fc.Proxy.CharonURL - } else { - o.ProxyCharonURL = deriveCharonURL(o.CharonListen) + if !setFlags["telemetry-exporter-url"] { + o.Telemetry.ExporterURL = fc.Telemetry.ExporterURL + } + o.Telemetry.ServiceName = fc.Telemetry.CharonService + + return nil +} + +// Validate checks CharonOptions for invalid combinations. It performs no I/O. +func (o *CharonOptions) Validate() error { + return nil +} + +// --------------------------------------------------------------------------- +// ProxyOptions +// --------------------------------------------------------------------------- + +// ProxyOptions holds configuration for the proxy server. +type ProxyOptions struct { + // Config file path — set by --config flag. + ConfigFile string + + // Listen address for the proxy HTTP server. + Listen string + + // Inference backend settings. + Backend string // base URL + APIKey string + TimeoutSeconds int + + // CharonURL is the Charon internal API endpoint the proxy calls. + // Auto-derived from config file proxy.charon_url or Charon's listen address. + CharonURL string + + Telemetry TelemetryOptions +} + +// NewProxyOptions returns a ProxyOptions pre-filled with built-in defaults. +func NewProxyOptions() *ProxyOptions { + return &ProxyOptions{ + Listen: ":8080", + Backend: "http://localhost:11434", + TimeoutSeconds: 120, + CharonURL: "http://127.0.0.1:8081", + Telemetry: TelemetryOptions{ServiceName: "charon-proxy"}, + } +} + +// AddFlags registers CLI flags on fs for the proxy server. +func (o *ProxyOptions) AddFlags(fs *flag.FlagSet) { + fs.StringVar(&o.ConfigFile, "config", "", "path to config file") + fs.StringVar(&o.Listen, "listen", o.Listen, "proxy server listen address") + fs.StringVar(&o.Backend, "backend", o.Backend, "inference backend base URL") + fs.StringVar(&o.CharonURL, "charon-url", o.CharonURL, "charon internal API base URL") + fs.StringVar(&o.Telemetry.ExporterURL, "telemetry-exporter-url", o.Telemetry.ExporterURL, "OTLP HTTP exporter URL (e.g. http://localhost:4318); empty disables tracing") +} + +// Complete loads the config file (if --config was set) and merges file values +// into the options struct. CLI flags take precedence over config-file values. +func (o *ProxyOptions) Complete(fs *flag.FlagSet) error { + if o.ConfigFile == "" { + return nil + } + + fc, err := loadFileConfig(o.ConfigFile) + if err != nil { + return err + } + + setFlags := make(map[string]bool) + fs.Visit(func(f *flag.Flag) { setFlags[f.Name] = true }) + + if !setFlags["listen"] { + o.Listen = fc.Proxy.Listen + } + if !setFlags["backend"] { + o.Backend = fc.Proxy.Inference.BaseURL + } + // API key and timeout are config-file-only. + o.APIKey = fc.Proxy.Inference.APIKey + o.TimeoutSeconds = fc.Proxy.Inference.TimeoutSeconds + + if !setFlags["charon-url"] { + if fc.Proxy.CharonURL != "" { + o.CharonURL = fc.Proxy.CharonURL + } else { + o.CharonURL = deriveCharonURL(fc.Charon.Listen) + } } - // Telemetry settings are config-file-only except exporter URL which has a flag. if !setFlags["telemetry-exporter-url"] { o.Telemetry.ExporterURL = fc.Telemetry.ExporterURL } - o.Telemetry.CharonService = fc.Telemetry.CharonService - o.Telemetry.ProxyService = fc.Telemetry.ProxyService + o.Telemetry.ServiceName = fc.Telemetry.ProxyService return nil } -// Validate checks ServerOptions for invalid combinations. It performs no I/O. -func (o *ServerOptions) Validate() error { - if o.ProxyEnabled && o.ProxyBackend == "" { - return fmt.Errorf("proxy enabled but proxy backend (inference base URL) is empty") +// Validate checks ProxyOptions for invalid combinations. It performs no I/O. +func (o *ProxyOptions) Validate() error { + if o.Backend == "" { + return fmt.Errorf("proxy backend (inference base URL) is empty") } return nil } diff --git a/internal/config/options_test.go b/internal/config/options_test.go index 0f77d34..5fb162b 100644 --- a/internal/config/options_test.go +++ b/internal/config/options_test.go @@ -13,76 +13,57 @@ import ( "github.com/elevran/charon/internal/config" ) -func TestServerOptionsDefaultsWithNoConfig(t *testing.T) { - opts := config.NewServerOptions() +// --------------------------------------------------------------------------- +// CharonOptions tests +// --------------------------------------------------------------------------- + +func TestCharonOptionsDefaultsWithNoConfig(t *testing.T) { + opts := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) opts.AddFlags(fs) require.NoError(t, fs.Parse(nil)) require.NoError(t, opts.Complete(fs)) - assert.Equal(t, ":8081", opts.CharonListen) - assert.Equal(t, ":8080", opts.ProxyListen) - assert.False(t, opts.ProxyEnabled) - assert.Equal(t, "http://localhost:11434", opts.ProxyBackend) - assert.Equal(t, "http://127.0.0.1:8081", opts.ProxyCharonURL, "ProxyCharonURL must be auto-derived") + assert.Equal(t, ":8081", opts.Listen) assert.Equal(t, "./data", opts.DataDir) assert.Equal(t, 30, opts.TTLDays) assert.Equal(t, time.Hour, opts.WorkerTTLInterval) + assert.Equal(t, "charon", opts.Telemetry.ServiceName) } -func TestServerOptionsCompleteLoadsFile(t *testing.T) { - opts := config.NewServerOptions() +func TestCharonOptionsCompleteLoadsFile(t *testing.T) { + opts := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) opts.AddFlags(fs) require.NoError(t, fs.Parse([]string{"--config", "testdata/config.yaml"})) require.NoError(t, opts.Complete(fs)) - // config.yaml sets proxy.listen: ":0" and charon.listen: ":9090" - assert.Equal(t, ":0", opts.ProxyListen) - assert.Equal(t, ":9090", opts.CharonListen) - // ProxyCharonURL should be auto-derived from charon.listen ":9090" - assert.Equal(t, "http://127.0.0.1:9090", opts.ProxyCharonURL) + assert.Equal(t, ":9090", opts.Listen) + assert.Equal(t, "./data", opts.DataDir) } -func TestServerOptionsCLIOverridesFile(t *testing.T) { - opts := config.NewServerOptions() +func TestCharonOptionsCLIOverridesFile(t *testing.T) { + opts := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) opts.AddFlags(fs) - // --listen overrides charon.listen from the file (":9090") require.NoError(t, fs.Parse([]string{"--config", "testdata/config.yaml", "--listen", ":7777"})) require.NoError(t, opts.Complete(fs)) - assert.Equal(t, ":7777", opts.CharonListen, "CLI flag must take precedence over config file") -} - -func TestServerOptionsValidateOK(t *testing.T) { - opts := config.NewServerOptions() - require.NoError(t, opts.Validate()) -} - -func TestServerOptionsValidateProxyEnabledRequiresBackend(t *testing.T) { - opts := config.NewServerOptions() - opts.ProxyEnabled = true - opts.ProxyBackend = "" - err := opts.Validate() - require.Error(t, err) - assert.Contains(t, err.Error(), "proxy backend") + assert.Equal(t, ":7777", opts.Listen, "CLI flag must take precedence over config file") } -func TestServerOptionsValidateProxyEnabledWithBackendOK(t *testing.T) { - opts := config.NewServerOptions() - opts.ProxyEnabled = true - // ProxyBackend defaults to "http://localhost:11434" — should be OK. +func TestCharonOptionsValidateOK(t *testing.T) { + opts := config.NewCharonOptions() require.NoError(t, opts.Validate()) } -func TestServerOptionsDataDirCLIOverridesFile(t *testing.T) { +func TestCharonOptionsDataDirCLIOverridesFile(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "cfg.yaml") err := os.WriteFile(cfgPath, []byte("charon:\n storage:\n data_dir: /tmp/test-data\n"), 0600) require.NoError(t, err) - opts := config.NewServerOptions() + opts := config.NewCharonOptions() fs := flag.NewFlagSet("test", flag.ContinueOnError) opts.AddFlags(fs) require.NoError(t, fs.Parse([]string{"--config", cfgPath, "--storage-data-dir", "./my-data"})) @@ -90,3 +71,56 @@ func TestServerOptionsDataDirCLIOverridesFile(t *testing.T) { assert.Equal(t, "./my-data", opts.DataDir) } + +// --------------------------------------------------------------------------- +// ProxyOptions tests +// --------------------------------------------------------------------------- + +func TestProxyOptionsDefaultsWithNoConfig(t *testing.T) { + opts := config.NewProxyOptions() + fs := flag.NewFlagSet("test", flag.ContinueOnError) + opts.AddFlags(fs) + require.NoError(t, fs.Parse(nil)) + require.NoError(t, opts.Complete(fs)) + + assert.Equal(t, ":8080", opts.Listen) + assert.Equal(t, "http://localhost:11434", opts.Backend) + assert.Equal(t, "http://127.0.0.1:8081", opts.CharonURL) + assert.Equal(t, 120, opts.TimeoutSeconds) + assert.Equal(t, "charon-proxy", opts.Telemetry.ServiceName) +} + +func TestProxyOptionsCompleteLoadsFile(t *testing.T) { + opts := config.NewProxyOptions() + fs := flag.NewFlagSet("test", flag.ContinueOnError) + opts.AddFlags(fs) + require.NoError(t, fs.Parse([]string{"--config", "testdata/config.yaml"})) + require.NoError(t, opts.Complete(fs)) + + assert.Equal(t, ":0", opts.Listen) + // charon_url not set in testdata/config.yaml → auto-derived from charon.listen ":9090" + assert.Equal(t, "http://127.0.0.1:9090", opts.CharonURL) +} + +func TestProxyOptionsCLIOverridesFile(t *testing.T) { + opts := config.NewProxyOptions() + fs := flag.NewFlagSet("test", flag.ContinueOnError) + opts.AddFlags(fs) + require.NoError(t, fs.Parse([]string{"--config", "testdata/config.yaml", "--listen", ":5555"})) + require.NoError(t, opts.Complete(fs)) + + assert.Equal(t, ":5555", opts.Listen, "CLI flag must take precedence over config file") +} + +func TestProxyOptionsValidateOK(t *testing.T) { + opts := config.NewProxyOptions() + require.NoError(t, opts.Validate()) +} + +func TestProxyOptionsValidateEmptyBackendFails(t *testing.T) { + opts := config.NewProxyOptions() + opts.Backend = "" + err := opts.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "backend") +} From 0173eab6a952b0c472e34643dce129e3d220000c Mon Sep 17 00:00:00 2001 From: Etai Lev Ran Date: Wed, 8 Jul 2026 16:03:11 +0300 Subject: [PATCH 2/2] config: extract TelemetryOptions.AddFlags, visitedFlags helper, real Validate, test coverage - Extract (*TelemetryOptions).AddFlags(fs) so --telemetry-exporter-url is registered once and called from both CharonOptions and ProxyOptions - Extract visitedFlags(fs) helper to remove duplicated map+Visit pattern from both Complete methods - Give CharonOptions.Validate() a real check: returns error when DataDir is empty - Add TestProxyOptionsBackendCLIOverridesFile and TestProxyOptionsCharonURLCLIOverridesFile to cover the merge semantics for --backend and --charon-url - Drop dead fileProxyConfig.Enabled field (its consumer, ServerOptions, is gone) Signed-off-by: Etai Lev Ran --- internal/config/options.go | 26 +++++++++++++++++++------- internal/config/options_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/internal/config/options.go b/internal/config/options.go index 3b5c1ad..591147f 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -18,7 +18,6 @@ type fileConfig struct { } type fileProxyConfig struct { - Enabled bool `json:"enabled"` Listen string `json:"listen"` CharonURL string `json:"charon_url"` Inference fileInferenceConfig `json:"inference"` @@ -117,6 +116,18 @@ type TelemetryOptions struct { ServiceName string // identifies this binary in traces } +// AddFlags registers the --telemetry-exporter-url flag on fs. +func (o *TelemetryOptions) AddFlags(fs *flag.FlagSet) { + fs.StringVar(&o.ExporterURL, "telemetry-exporter-url", o.ExporterURL, "OTLP HTTP exporter URL (e.g. http://localhost:4318); empty disables tracing") +} + +// visitedFlags returns a set of flag names that were explicitly set on fs. +func visitedFlags(fs *flag.FlagSet) map[string]bool { + m := make(map[string]bool) + fs.Visit(func(f *flag.Flag) { m[f.Name] = true }) + return m +} + // --------------------------------------------------------------------------- // CharonOptions // --------------------------------------------------------------------------- @@ -160,7 +171,7 @@ func (o *CharonOptions) AddFlags(fs *flag.FlagSet) { fs.StringVar(&o.ConfigFile, "config", "", "path to config file") fs.StringVar(&o.Listen, "listen", o.Listen, "charon internal API listen address") fs.StringVar(&o.DataDir, "storage-data-dir", o.DataDir, "data directory for Pebble storage") - fs.StringVar(&o.Telemetry.ExporterURL, "telemetry-exporter-url", o.Telemetry.ExporterURL, "OTLP HTTP exporter URL (e.g. http://localhost:4318); empty disables tracing") + o.Telemetry.AddFlags(fs) } // Complete loads the config file (if --config was set) and merges file values @@ -175,8 +186,7 @@ func (o *CharonOptions) Complete(fs *flag.FlagSet) error { return err } - setFlags := make(map[string]bool) - fs.Visit(func(f *flag.Flag) { setFlags[f.Name] = true }) + setFlags := visitedFlags(fs) if !setFlags["listen"] { o.Listen = fc.Charon.Listen @@ -205,6 +215,9 @@ func (o *CharonOptions) Complete(fs *flag.FlagSet) error { // Validate checks CharonOptions for invalid combinations. It performs no I/O. func (o *CharonOptions) Validate() error { + if o.DataDir == "" { + return fmt.Errorf("charon storage data-dir is empty") + } return nil } @@ -249,7 +262,7 @@ func (o *ProxyOptions) AddFlags(fs *flag.FlagSet) { fs.StringVar(&o.Listen, "listen", o.Listen, "proxy server listen address") fs.StringVar(&o.Backend, "backend", o.Backend, "inference backend base URL") fs.StringVar(&o.CharonURL, "charon-url", o.CharonURL, "charon internal API base URL") - fs.StringVar(&o.Telemetry.ExporterURL, "telemetry-exporter-url", o.Telemetry.ExporterURL, "OTLP HTTP exporter URL (e.g. http://localhost:4318); empty disables tracing") + o.Telemetry.AddFlags(fs) } // Complete loads the config file (if --config was set) and merges file values @@ -264,8 +277,7 @@ func (o *ProxyOptions) Complete(fs *flag.FlagSet) error { return err } - setFlags := make(map[string]bool) - fs.Visit(func(f *flag.Flag) { setFlags[f.Name] = true }) + setFlags := visitedFlags(fs) if !setFlags["listen"] { o.Listen = fc.Proxy.Listen diff --git a/internal/config/options_test.go b/internal/config/options_test.go index 5fb162b..53a4775 100644 --- a/internal/config/options_test.go +++ b/internal/config/options_test.go @@ -112,6 +112,36 @@ func TestProxyOptionsCLIOverridesFile(t *testing.T) { assert.Equal(t, ":5555", opts.Listen, "CLI flag must take precedence over config file") } +func TestProxyOptionsBackendCLIOverridesFile(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "cfg.yaml") + err := os.WriteFile(cfgPath, []byte("proxy:\n inference:\n base_url: http://file-backend:11434\n"), 0600) + require.NoError(t, err) + + opts := config.NewProxyOptions() + fs := flag.NewFlagSet("test", flag.ContinueOnError) + opts.AddFlags(fs) + require.NoError(t, fs.Parse([]string{"--config", cfgPath, "--backend", "http://cli-backend:11434"})) + require.NoError(t, opts.Complete(fs)) + + assert.Equal(t, "http://cli-backend:11434", opts.Backend, "CLI --backend must take precedence over config file") +} + +func TestProxyOptionsCharonURLCLIOverridesFile(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "cfg.yaml") + err := os.WriteFile(cfgPath, []byte("proxy:\n charon_url: http://file-charon:8081\n"), 0600) + require.NoError(t, err) + + opts := config.NewProxyOptions() + fs := flag.NewFlagSet("test", flag.ContinueOnError) + opts.AddFlags(fs) + require.NoError(t, fs.Parse([]string{"--config", cfgPath, "--charon-url", "http://cli-charon:9999"})) + require.NoError(t, opts.Complete(fs)) + + assert.Equal(t, "http://cli-charon:9999", opts.CharonURL, "CLI --charon-url must take precedence over config file") +} + func TestProxyOptionsValidateOK(t *testing.T) { opts := config.NewProxyOptions() require.NoError(t, opts.Validate())