Fix parameters camelCase support in loadEnvVars for environment variable parsing #588
Conversation
…names - Introduced transformEnvVariable function to format env var names based on JSON tags - Modified loadEnvVars to use the new transformEnvVariable function - Added generateTagMapping to create mappings from struct fields' JSON tags
- Implemented `ServerLoadBalancerStrategyOverwrite` test to verify
that server load balancer strategy is correctly set and loaded from
environment variables.
- Implemented `pluginDefaultPolicyOverwrite` test to verify
that plugin default policy is correctly set and loaded from
environment variables.
- Added `initializeConfig` helper function to facilitate configuration
setup for tests.
- Updated `TestLoadEnvVariables` to include the new scenario for
server load balancer strategy overwrite.
| PluginConfigFile: parentDir + PluginsConfigFilename, | ||
| }) | ||
| err := config.InitConfig(ctx) | ||
| require.Nil(t, err) |
There was a problem hiding this comment.
| require.Nil(t, err) | |
| require.NoError(t, err) |
There was a problem hiding this comment.
config.InitConfig returns a errors.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.
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.
Useres or whatever
There was a problem hiding this comment.
GatewayDError is actually an implementation of an error, and we have Error() string. The issue is that our functions return *gerr.GatewayDError instead of the error 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.
| ctx := context.Background() | ||
|
|
||
| // Set the environment variable | ||
| t.Setenv("GATEWAYD_DEFAULTPOLICY", "test") |
There was a problem hiding this comment.
I would have expected
| t.Setenv("GATEWAYD_DEFAULTPOLICY", "test") | |
| t.Setenv("GATEWAYD_DEFAULT_POLICY", "test") |
Is there a problem or something to code differently?
There was a problem hiding this comment.
This is a test case for the plugin. In the plugin configuration, there isn't a configuration group, so DefaultPolicy is a parameter directly within gatewayd_plugins.yaml.
CompatibilityPolicy: "strict"
DefaultPolicy: "passthrough"
Ticket(s)
#583
Description
This PR addresses a bug in the loadEnvVars function that prevented correct loading of environment variables when their names are in camelCase. The issue was caused by the function’s inability to transform camelCase environment variable names into the expected format for configuration.
Changes Made:
Updated loadEnvVars Function:
Introduced transformEnvVariable Function:
Added generateTagMapping Function:
Expanded Test Coverage:
Development Checklist
make gen-docscommand.Legal Checklist