From 71a3bce065bbef39be5e7928d238fa0650d9257e Mon Sep 17 00:00:00 2001 From: Matthew Karsten Date: Mon, 30 Mar 2026 09:21:40 -0500 Subject: [PATCH] fix: bind CLI config file values to cobra flags (#973) Config file values (~/.kagent/config.yaml) were ignored because cobra flag defaults always took precedence. This wires viper-backed config into the flag system so the precedence is: CLI flag > env > config file > viper default. Adds BindFlags() to explicitly map dash-separated flag names to underscore-separated config keys, and Apply() to populate the shared Config struct from the merged viper state before any subcommand runs. Includes tests verifying config file values are used and that explicit CLI flags still override them. --- go/core/cli/cmd/kagent/main.go | 19 ++- go/core/cli/internal/config/config.go | 24 ++++ go/core/cli/internal/config/config_test.go | 133 +++++++++++++++++++++ 3 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 go/core/cli/internal/config/config_test.go diff --git a/go/core/cli/cmd/kagent/main.go b/go/core/cli/cmd/kagent/main.go index 8792835b0..7ea369a70 100644 --- a/go/core/cli/cmd/kagent/main.go +++ b/go/core/cli/cmd/kagent/main.go @@ -429,12 +429,29 @@ Examples: rootCmd.AddCommand(installCmd, uninstallCmd, invokeCmd, bugReportCmd, versionCmd, dashboardCmd, getCmd, initCmd, buildCmd, deployCmd, addMcpCmd, runCmd, mcp.NewMCPCmd(), envdoc.NewEnvCmd()) - // Initialize config + // Initialize config file and viper defaults if err := config.Init(); err != nil { fmt.Fprintf(os.Stderr, "Error initializing config: %v\n", err) os.Exit(1) } + // Bind cobra flags to viper so config file values are used when flags + // are not explicitly set on the command line. + config.BindFlags(rootCmd.PersistentFlags()) + + // Populate cfg from the merged viper state (config file + env + flags) + // before any subcommand runs, ensuring all commands see config file values. + originalPreRunE := rootCmd.PersistentPreRunE + rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + if err := config.Apply(cfg); err != nil { + return fmt.Errorf("error applying config: %w", err) + } + if originalPreRunE != nil { + return originalPreRunE(cmd, args) + } + return nil + } + if err := rootCmd.ExecuteContext(ctx); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) diff --git a/go/core/cli/internal/config/config.go b/go/core/cli/internal/config/config.go index 04cd31f8c..8a549ada6 100644 --- a/go/core/cli/internal/config/config.go +++ b/go/core/cli/internal/config/config.go @@ -62,6 +62,30 @@ func Init() error { return nil } +// BindFlags binds cobra persistent flags to viper keys so that config file +// values are used as defaults when the corresponding CLI flag is not explicitly +// set. Flag names use dashes (e.g. "kagent-url") while config file keys use +// underscores (e.g. "kagent_url"), so each binding is explicit. +func BindFlags(flags *pflag.FlagSet) { + viper.BindPFlag("kagent_url", flags.Lookup("kagent-url")) //nolint:errcheck + viper.BindPFlag("namespace", flags.Lookup("namespace")) //nolint:errcheck + viper.BindPFlag("output_format", flags.Lookup("output-format")) //nolint:errcheck + viper.BindPFlag("verbose", flags.Lookup("verbose")) //nolint:errcheck + viper.BindPFlag("timeout", flags.Lookup("timeout")) //nolint:errcheck +} + +// Apply populates the given Config from the merged viper state (config file + +// env + CLI flags). Call this after flag parsing to ensure cfg reflects all +// sources with the correct precedence: CLI flag > env > config file > default. +func Apply(cfg *Config) error { + merged, err := Get() + if err != nil { + return err + } + *cfg = *merged + return nil +} + func Get() (*Config, error) { var config Config if err := viper.Unmarshal(&config); err != nil { diff --git a/go/core/cli/internal/config/config_test.go b/go/core/cli/internal/config/config_test.go new file mode 100644 index 000000000..14cc966f1 --- /dev/null +++ b/go/core/cli/internal/config/config_test.go @@ -0,0 +1,133 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/spf13/pflag" + "github.com/spf13/viper" +) + +func TestConfigFileValuesAreUsed(t *testing.T) { + // Reset viper for a clean test + viper.Reset() + + // Create a temp config directory + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "config.yaml") + + // Write a config file with custom values + configContent := `kagent_url: http://custom-server:9090 +namespace: my-namespace +output_format: json +verbose: true +timeout: 60s +` + if err := os.WriteFile(configFile, []byte(configContent), 0644); err != nil { + t.Fatalf("Failed to write config file: %v", err) + } + + // Set up viper to read the config + viper.SetConfigFile(configFile) + viper.SetConfigType("yaml") + viper.SetDefault("kagent_url", "http://localhost:8083") + viper.SetDefault("output_format", "table") + viper.SetDefault("namespace", "kagent") + viper.SetDefault("timeout", 300*time.Second) + + if err := viper.ReadInConfig(); err != nil { + t.Fatalf("Failed to read config: %v", err) + } + + // Create flags (simulating what main.go does) + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.String("kagent-url", "http://localhost:8083", "KAgent URL") + flags.StringP("namespace", "n", "kagent", "Namespace") + flags.StringP("output-format", "o", "table", "Output format") + flags.BoolP("verbose", "v", false, "Verbose output") + flags.Duration("timeout", 300*time.Second, "Timeout") + + // Bind flags to viper + BindFlags(flags) + + // Get merged config (should use config file values since no flags were explicitly set) + cfg := &Config{} + if err := Apply(cfg); err != nil { + t.Fatalf("Apply failed: %v", err) + } + + if cfg.KAgentURL != "http://custom-server:9090" { + t.Errorf("Expected KAgentURL=http://custom-server:9090, got %s", cfg.KAgentURL) + } + if cfg.Namespace != "my-namespace" { + t.Errorf("Expected Namespace=my-namespace, got %s", cfg.Namespace) + } + if cfg.OutputFormat != "json" { + t.Errorf("Expected OutputFormat=json, got %s", cfg.OutputFormat) + } + if !cfg.Verbose { + t.Error("Expected Verbose=true, got false") + } + if cfg.Timeout != 60*time.Second { + t.Errorf("Expected Timeout=60s, got %v", cfg.Timeout) + } +} + +func TestCLIFlagsOverrideConfigFile(t *testing.T) { + // Reset viper for a clean test + viper.Reset() + + // Create a temp config directory + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "config.yaml") + + // Write a config file with custom values + configContent := `kagent_url: http://config-server:9090 +namespace: config-ns +` + if err := os.WriteFile(configFile, []byte(configContent), 0644); err != nil { + t.Fatalf("Failed to write config file: %v", err) + } + + viper.SetConfigFile(configFile) + viper.SetConfigType("yaml") + viper.SetDefault("kagent_url", "http://localhost:8083") + viper.SetDefault("namespace", "kagent") + viper.SetDefault("output_format", "table") + viper.SetDefault("timeout", 300*time.Second) + + if err := viper.ReadInConfig(); err != nil { + t.Fatalf("Failed to read config: %v", err) + } + + // Create and parse flags with explicit values + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.String("kagent-url", "http://localhost:8083", "KAgent URL") + flags.StringP("namespace", "n", "kagent", "Namespace") + flags.StringP("output-format", "o", "table", "Output format") + flags.BoolP("verbose", "v", false, "Verbose output") + flags.Duration("timeout", 300*time.Second, "Timeout") + + // Simulate passing --kagent-url on the CLI + if err := flags.Parse([]string{"--kagent-url", "http://cli-override:7777"}); err != nil { + t.Fatalf("Failed to parse flags: %v", err) + } + + BindFlags(flags) + + cfg := &Config{} + if err := Apply(cfg); err != nil { + t.Fatalf("Apply failed: %v", err) + } + + // CLI flag should win over config file + if cfg.KAgentURL != "http://cli-override:7777" { + t.Errorf("Expected KAgentURL=http://cli-override:7777, got %s", cfg.KAgentURL) + } + // Config file value should still be used for namespace (not overridden by CLI) + if cfg.Namespace != "config-ns" { + t.Errorf("Expected Namespace=config-ns, got %s", cfg.Namespace) + } +}