Skip to content

Commit 133d31f

Browse files
mauriciocoderMauricio Bonetti
mauriciocoder
and
Mauricio Bonetti
authored
Pass cmd parameter to getTelemetryFromFlags instead of nil, allowing config file fallbacks to work correctly for OTEL flags.
Co-authored-by: Mauricio Bonetti <[email protected]>
1 parent 4ca5291 commit 133d31f

File tree

2 files changed

+224
-4
lines changed

2 files changed

+224
-4
lines changed

‎cmd/thv/app/run_flags.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ func BuildRunnerConfig(
211211

212212
// Get OTEL flag values with config fallbacks
213213
config := cfg.GetConfig()
214-
finalOtelEndpoint, finalOtelSamplingRate, finalOtelEnvironmentVariables := getTelemetryFromFlags(nil, config,
214+
finalOtelEndpoint, finalOtelSamplingRate, finalOtelEnvironmentVariables := getTelemetryFromFlags(cmd, config,
215215
runConfig.OtelEndpoint, runConfig.OtelSamplingRate, runConfig.OtelEnvironmentVariables)
216216

217217
// Create container runtime
@@ -306,17 +306,17 @@ func getTelemetryFromFlags(cmd *cobra.Command, config *cfg.Config, otelEndpoint
306306
otelEnvironmentVariables []string) (string, float64, []string) {
307307
// Use config values as fallbacks for OTEL flags if not explicitly set
308308
finalOtelEndpoint := otelEndpoint
309-
if cmd != nil && !cmd.Flags().Changed("otel-endpoint") && config.OTEL.Endpoint != "" {
309+
if !cmd.Flags().Changed("otel-endpoint") && config.OTEL.Endpoint != "" {
310310
finalOtelEndpoint = config.OTEL.Endpoint
311311
}
312312

313313
finalOtelSamplingRate := otelSamplingRate
314-
if cmd != nil && !cmd.Flags().Changed("otel-sampling-rate") && config.OTEL.SamplingRate != 0.0 {
314+
if !cmd.Flags().Changed("otel-sampling-rate") && config.OTEL.SamplingRate != 0.0 {
315315
finalOtelSamplingRate = config.OTEL.SamplingRate
316316
}
317317

318318
finalOtelEnvironmentVariables := otelEnvironmentVariables
319-
if cmd != nil && !cmd.Flags().Changed("otel-env-vars") && len(config.OTEL.EnvVars) > 0 {
319+
if !cmd.Flags().Changed("otel-env-vars") && len(config.OTEL.EnvVars) > 0 {
320320
finalOtelEnvironmentVariables = config.OTEL.EnvVars
321321
}
322322

‎cmd/thv/app/run_flags_test.go

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
package app
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
8+
"github.com/adrg/xdg"
9+
"github.com/spf13/cobra"
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
13+
"github.com/stacklok/toolhive/pkg/config"
14+
"github.com/stacklok/toolhive/pkg/logger"
15+
)
16+
17+
// mockConfig creates a temporary config file with the provided configuration.
18+
func mockConfig(t *testing.T, cfg *config.Config) func() {
19+
t.Helper()
20+
tempDir := t.TempDir()
21+
originalXDGConfigHome := os.Getenv("XDG_CONFIG_HOME")
22+
t.Setenv("XDG_CONFIG_HOME", tempDir)
23+
xdg.Reload()
24+
configDir := filepath.Join(tempDir, "toolhive")
25+
err := os.MkdirAll(configDir, 0755)
26+
require.NoError(t, err)
27+
if cfg != nil {
28+
err = config.UpdateConfig(func(c *config.Config) { *c = *cfg })
29+
require.NoError(t, err)
30+
}
31+
return func() {
32+
t.Setenv("XDG_CONFIG_HOME", originalXDGConfigHome)
33+
xdg.Reload()
34+
}
35+
}
36+
37+
//nolint:paralleltest // Cannot use t.Parallel() with t.Setenv() in Go 1.24+
38+
func TestBuildRunnerConfig_TelemetryProcessing(t *testing.T) {
39+
// Initialize logger to prevent nil pointer dereference
40+
logger.Initialize()
41+
42+
tests := []struct {
43+
name string
44+
setupFlags func(*cobra.Command)
45+
configOTEL config.OpenTelemetryConfig
46+
runFlags *RunFlags
47+
expectedEndpoint string
48+
expectedSamplingRate float64
49+
expectedEnvironmentVariables []string
50+
}{
51+
{
52+
name: "CLI flags provided, taking precedence over config file",
53+
setupFlags: func(cmd *cobra.Command) {
54+
// Mark CLI flags as changed to simulate user providing them
55+
cmd.Flags().Set("otel-endpoint", "https://cli-endpoint.example.com")
56+
cmd.Flags().Set("otel-sampling-rate", "0.8")
57+
cmd.Flags().Set("otel-env-vars", "CLI_VAR1=value1")
58+
cmd.Flags().Set("otel-env-vars", "CLI_VAR2=value2")
59+
},
60+
configOTEL: config.OpenTelemetryConfig{
61+
Endpoint: "https://config-endpoint.example.com",
62+
SamplingRate: 0.2,
63+
EnvVars: []string{"CONFIG_VAR1=configvalue1", "CONFIG_VAR2=configvalue2"},
64+
},
65+
runFlags: &RunFlags{
66+
OtelEndpoint: "https://cli-endpoint.example.com",
67+
OtelSamplingRate: 0.8,
68+
OtelEnvironmentVariables: []string{"CLI_VAR1=value1", "CLI_VAR2=value2"},
69+
// Set other required fields to avoid nil pointer errors
70+
Transport: "sse",
71+
ProxyMode: "sse",
72+
Host: "localhost",
73+
PermissionProfile: "none",
74+
},
75+
expectedEndpoint: "https://cli-endpoint.example.com",
76+
expectedSamplingRate: 0.8,
77+
expectedEnvironmentVariables: []string{"CLI_VAR1=value1", "CLI_VAR2=value2"},
78+
},
79+
{
80+
name: "No CLI flags provided, config takes precedence",
81+
setupFlags: func(_ *cobra.Command) {
82+
// Don't set any flags - they should remain unchanged/default
83+
// This simulates the case where user doesn't provide CLI flags
84+
},
85+
configOTEL: config.OpenTelemetryConfig{
86+
Endpoint: "https://config-endpoint.example.com",
87+
SamplingRate: 0.3,
88+
EnvVars: []string{"CONFIG_VAR1=configvalue1", "CONFIG_VAR2=configvalue2"},
89+
},
90+
runFlags: &RunFlags{
91+
OtelEndpoint: "",
92+
OtelSamplingRate: 0.1,
93+
OtelEnvironmentVariables: nil,
94+
Transport: "sse",
95+
ProxyMode: "sse",
96+
Host: "localhost",
97+
PermissionProfile: "none",
98+
},
99+
expectedEndpoint: "https://config-endpoint.example.com",
100+
expectedSamplingRate: 0.3,
101+
expectedEnvironmentVariables: []string{"CONFIG_VAR1=configvalue1", "CONFIG_VAR2=configvalue2"},
102+
},
103+
{
104+
name: "Partial CLI flags provided, mix of CLI and config values",
105+
setupFlags: func(cmd *cobra.Command) {
106+
// Only set endpoint flag, leave others to use config values
107+
cmd.Flags().Set("otel-endpoint", "https://partial-cli-endpoint.example.com")
108+
},
109+
configOTEL: config.OpenTelemetryConfig{
110+
Endpoint: "https://config-endpoint.example.com",
111+
SamplingRate: 0.5,
112+
EnvVars: []string{"CONFIG_VAR1=configvalue1"},
113+
},
114+
runFlags: &RunFlags{
115+
OtelEndpoint: "https://partial-cli-endpoint.example.com",
116+
OtelSamplingRate: 0.1,
117+
OtelEnvironmentVariables: nil,
118+
Transport: "sse",
119+
ProxyMode: "sse",
120+
Host: "localhost",
121+
PermissionProfile: "none",
122+
},
123+
expectedEndpoint: "https://partial-cli-endpoint.example.com",
124+
expectedSamplingRate: 0.5,
125+
expectedEnvironmentVariables: []string{"CONFIG_VAR1=configvalue1"},
126+
},
127+
{
128+
name: "Empty config values, CLI flags should be used",
129+
setupFlags: func(cmd *cobra.Command) {
130+
cmd.Flags().Set("otel-endpoint", "https://cli-only-endpoint.example.com")
131+
cmd.Flags().Set("otel-sampling-rate", "0.9")
132+
},
133+
configOTEL: config.OpenTelemetryConfig{
134+
Endpoint: "",
135+
SamplingRate: 0.0,
136+
EnvVars: nil,
137+
},
138+
runFlags: &RunFlags{
139+
OtelEndpoint: "https://cli-only-endpoint.example.com",
140+
OtelSamplingRate: 0.9,
141+
OtelEnvironmentVariables: nil,
142+
Transport: "sse",
143+
ProxyMode: "sse",
144+
Host: "localhost",
145+
PermissionProfile: "none",
146+
},
147+
expectedEndpoint: "https://cli-only-endpoint.example.com",
148+
expectedSamplingRate: 0.9,
149+
expectedEnvironmentVariables: nil,
150+
},
151+
}
152+
153+
for _, tt := range tests {
154+
t.Run(tt.name, func(t *testing.T) {
155+
cmd := &cobra.Command{}
156+
AddRunFlags(cmd, &RunFlags{})
157+
tt.setupFlags(cmd)
158+
cleanup := mockConfig(t, &config.Config{
159+
OTEL: tt.configOTEL,
160+
})
161+
defer cleanup()
162+
configInstance := config.GetConfig()
163+
finalEndpoint, finalSamplingRate, finalEnvVars := getTelemetryFromFlags(
164+
cmd,
165+
configInstance,
166+
tt.runFlags.OtelEndpoint,
167+
tt.runFlags.OtelSamplingRate,
168+
tt.runFlags.OtelEnvironmentVariables,
169+
)
170+
171+
// Assert the results
172+
assert.Equal(t, tt.expectedEndpoint, finalEndpoint, "OTEL endpoint should match expected value")
173+
assert.Equal(t, tt.expectedSamplingRate, finalSamplingRate, "OTEL sampling rate should match expected value")
174+
assert.Equal(t, tt.expectedEnvironmentVariables, finalEnvVars, "OTEL environment variables should match expected value")
175+
})
176+
}
177+
}
178+
179+
//nolint:paralleltest // Cannot use t.Parallel() with t.Setenv() in Go 1.24+
180+
func TestBuildRunnerConfig_TelemetryProcessing_Integration(t *testing.T) {
181+
// This is a more complete integration test that tests telemetry processing
182+
// within the full BuildRunnerConfig function context
183+
logger.Initialize()
184+
cmd := &cobra.Command{}
185+
runFlags := &RunFlags{
186+
Transport: "sse",
187+
ProxyMode: "sse",
188+
Host: "localhost",
189+
PermissionProfile: "none",
190+
OtelEndpoint: "https://integration-test.example.com",
191+
OtelSamplingRate: 0.7,
192+
}
193+
AddRunFlags(cmd, runFlags)
194+
err := cmd.Flags().Set("otel-endpoint", "https://integration-test.example.com")
195+
require.NoError(t, err)
196+
err = cmd.Flags().Set("otel-sampling-rate", "0.7")
197+
require.NoError(t, err)
198+
cleanup := mockConfig(t, &config.Config{
199+
OTEL: config.OpenTelemetryConfig{
200+
Endpoint: "https://config-fallback.example.com",
201+
SamplingRate: 0.2,
202+
EnvVars: []string{"CONFIG_VAR=value"},
203+
},
204+
})
205+
defer cleanup()
206+
207+
configInstance := config.GetConfig()
208+
finalEndpoint, finalSamplingRate, finalEnvVars := getTelemetryFromFlags(
209+
cmd,
210+
configInstance,
211+
runFlags.OtelEndpoint,
212+
runFlags.OtelSamplingRate,
213+
runFlags.OtelEnvironmentVariables,
214+
)
215+
216+
// Verify that CLI values take precedence
217+
assert.Equal(t, "https://integration-test.example.com", finalEndpoint, "CLI endpoint should take precedence over config")
218+
assert.Equal(t, 0.7, finalSamplingRate, "CLI sampling rate should take precedence over config")
219+
assert.Equal(t, []string{"CONFIG_VAR=value"}, finalEnvVars, "Environment variables should fall back to config when not set via CLI")
220+
}

0 commit comments

Comments
 (0)