Env-vars expect lower case letters

[mostafa]

I found a tiny bug (more of an assumption on how things work) in processing environment variables. The strings.ToLower in the loadEnvVars() function assumes that all keys are in lower case, yet the config blocks are camel case, like activeWrites.

As a result, the GATEWAYD_POOLS_DEFAULT_ACTIVEWRITES_SIZE=1 gets converted to map[pools:map[default:map[activewrites:map[size:1]]]], which is incorrect because it creates a new config block: activewrites (notice the lower case).

I removed the strings.ToLower function and tested it with normal cased env-vars. Now, using GATEWAYD_pools_default_activeWrites_size=1 gets converted to map[pools:map[default:map[activeWrites:map[size:1]]]] and it works as expected.

However, environment variables are usually written in upper case letters, so we need to find a fix for this that accommodates the convention of upper case env-vars without breaking the camel case config blocks.

This is related to recent changes in #577.

A viable solution is to disregard the casing and only accept lower case config blocks (and maybe config groups). And then ensure that we are converting all the keys to lower case before loading the configuration from the file.

[sinadarbouy]

@mostafa, what about the parameters? I guess it’s not only related to the recent change; we have a problem with those that do not have a configuration block, like GATEWAYD_SERVERS_DEFAULT_ENABLETICKER. So, I guess we need to lowercase all configuration groups, configuration blocks, and parameters

[sinadarbouy]

@mostafa, what about the parameters? I guess it’s not only related to the recent change; we have a problem with those that do not have a configuration block, like GATEWAYD_SERVERS_DEFAULT_ENABLETICKER. So, I guess we need to lowercase all configuration groups, configuration blocks, and parameters

But parameters are struct, not keys, so it’s not a good idea to lowercase them. For parameters, we need to find another solution or leave them as they are. The PR #587 will only lowercase configuration groups and configuration blocks.

[mostafa]

@sinadarbouy how about changing struct tags? Possibly in a separate PR.

[sinadarbouy]

@sinadarbouy how about changing struct tags? Possibly in a separate PR.

If we change the struct tag, we need to change the YAML configuration as well. but, do we want to lowercase the parameters or change them to another format like Kebab Case? Neither of these options is a good idea.

@mostafa What do you think about implementing some parameter mapping? Parameters not defined by the user so could have a mapping, such as:

paramerters_mapping = {
    "NETWORK": "network",
    "ADDRESS": "address",
    "LOADBALANCER_STRATEGY": "loadBalancer.strategy",
    "ENABLETICKER": "enableTicker",
    "TICKINTERVAL": "tickInterval",
    "ENABLETLS": "enableTLS",
    "CERTFILE": "certFile",
    "KEYFILE": "keyFile",
    "HANDSHAKETIMEOUT": "handshakeTimeout"
}

or generate it from env tag in struct, something like

type Server struct {
	EnableTicker bool          `json:"enableTicker" env:"NETWORK"`
	TickInterval time.Duration `json:"tickInterval" jsonschema:"oneof_type=string;integer" env:"TICKINTERVAL"`
}

func generateParametersMapping(s interface{}) map[string]string {
	mapping := make(map[string]string)
	val := reflect.ValueOf(s).Elem()
	typ := val.Type()

	for i := 0; i < val.NumField(); i++ {
		field := typ.Field(i)
		envTag := field.Tag.Get("env")
		if envTag != "" {
			mapping[envTag] = field.Name
		}
	}
	return mapping
}

[mostafa]

Env tag seems like a nice idea.

[ccoVeille]

Definitely, maybe the env might be optional for the obvious ones (the ones that have no problems)

I'm a bit afraid a forgotten env tag on a new fields won't be available via the env variables

Or, the code,or the tests, could expect each struct fields/groups to have an env tag

[sinadarbouy]

Using the env tag can complicate things and introduce additional edge cases, such as needing to parse the ENV variable to determine if it represents parameters, a configuration group, or a configuration block. the PR #588, handle this with the json tag. In this case,LOADENV doesn’t need to consider which nested map it(parse part of env) relates to; instead, it’ll replace it with the corresponding json tag.