-
Notifications
You must be signed in to change notification settings - Fork 21
Fix parameters camelCase support in loadEnvVars for environment variable parsing #588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,6 +2,8 @@ package config | |||||
|
||||||
import ( | ||||||
"context" | ||||||
"fmt" | ||||||
"strings" | ||||||
"testing" | ||||||
|
||||||
"github.com/knadh/koanf" | ||||||
|
@@ -142,3 +144,62 @@ func TestMergeGlobalConfig(t *testing.T) { | |||||
// The log level should now be debug. | ||||||
assert.Equal(t, "debug", config.Global.Loggers[Default].Level) | ||||||
} | ||||||
|
||||||
// initializeConfig initializes the configuration with the given context. | ||||||
// It returns a pointer to the Config struct. If configuration initialization fails, | ||||||
// the test will fail with an error message. | ||||||
func initializeConfig(ctx context.Context, t *testing.T) *Config { | ||||||
t.Helper() | ||||||
config := NewConfig(ctx, Config{ | ||||||
GlobalConfigFile: parentDir + GlobalConfigFilename, | ||||||
PluginConfigFile: parentDir + PluginsConfigFilename, | ||||||
}) | ||||||
err := config.InitConfig(ctx) | ||||||
require.Nil(t, err) | ||||||
return config | ||||||
} | ||||||
|
||||||
// serverLoadBalancerStrategyOverwrite sets the environment variable for server nested configuration | ||||||
// and verifies that the configuration is correctly loaded with the expected value. | ||||||
func ServerLoadBalancerStrategyOverwrite(t *testing.T) { | ||||||
t.Helper() | ||||||
ctx := context.Background() | ||||||
// Convert to uppercase | ||||||
upperDefaultGroup := strings.ToUpper(Default) | ||||||
|
||||||
// Format environment variable name | ||||||
envVarName := fmt.Sprintf("GATEWAYD_SERVERS_%s_LOADBALANCER_STRATEGY", upperDefaultGroup) | ||||||
|
||||||
// Set the environment variable | ||||||
t.Setenv(envVarName, "test") | ||||||
config := initializeConfig(ctx, t) | ||||||
assert.Equal(t, "test", config.Global.Servers[Default].LoadBalancer.Strategy) | ||||||
} | ||||||
|
||||||
// pluginDefaultPolicyOverwrite sets the environment variable for plugin configuration | ||||||
// and verifies that the configuration is correctly loaded with the expected value. | ||||||
func pluginDefaultPolicyOverwrite(t *testing.T) { | ||||||
t.Helper() | ||||||
ctx := context.Background() | ||||||
|
||||||
// Set the environment variable | ||||||
t.Setenv("GATEWAYD_DEFAULTPOLICY", "test") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have expected
Suggested change
Is there a problem or something to code differently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a test case for the plugin. In the plugin configuration, there isn't a
|
||||||
config := initializeConfig(ctx, t) | ||||||
assert.Equal(t, "test", config.Plugin.DefaultPolicy) | ||||||
} | ||||||
|
||||||
// TestLoadEnvVariables runs a suite of tests to verify that environment variables are correctly | ||||||
// loaded into the configuration. Each test scenario sets a specific environment variable and | ||||||
// checks if the configuration reflects the expected value. | ||||||
func TestLoadEnvVariables(t *testing.T) { | ||||||
scenarios := map[string]func(t *testing.T){ | ||||||
"serverLoadBalancerStrategyOverwrite": ServerLoadBalancerStrategyOverwrite, | ||||||
"pluginLocalPathOverwrite": pluginDefaultPolicyOverwrite, | ||||||
} | ||||||
|
||||||
for scenario, fn := range scenarios { | ||||||
t.Run(scenario, func(t *testing.T) { | ||||||
fn(t) | ||||||
}) | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.InitConfig
returns aerrors.GatewayDError
, not a standard error. so, it’s an object, and we can assert it using require.NoError.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So strange, I would have added the Error() string interface to this struct to solve this.
If you don't/cannot want to do this, you can simply rename the variable
err
, err is type connoted.Use
res
or whateverThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GatewayDError is actually an implementation of an error, and we have Error() string. The issue is that our functions return
*gerr.GatewayDError
instead of theerror
type. When we return nil as *gerr.GatewayDError, it’s basically like returning (gerr.GatewayDError, nil), and require.NoError can’t handle that. This link explains it better: Go FAQ - nil error.The solution is to change all return types to the error type. I assume this is outside the scope of this PR.
Renaming it to something else isn’t a great idea because it’s really just an error. Later on, if we switch to using error, it’ll be easier to handle.