chore: allow to use custom environment

[Eun]

When using the env provider in tests it is always necessary to set environment values beforehand:

func TestLoadConfig(t *testing.T) {
	k := koanf.New(".")
	os.Setenv("APP_USER", "Joe")
	os.Setenv("APP_PASSWORD", "secret")
	defer os.Unsetenv("APP_USER")
	defer os.Unsetenv("APP_PASSWORD")
	err := k.Load(env.Provider("APP_", ".", nil), nil)
	if err != nil {
		return nil, errors.Wrapf(err, "unable to load config from environment")
	}
	k.Print()
}

With this change it would be possible to have a pattern like this:

func TestLoadConfig(t *testing.T) {
	k := koanf.New(".")
	provider := env.Provider("APP_", ".", nil)
	provider.Environ = []string{
		"APP_USER=Joe",
		"APP_PASSWORD=secret",
	}
	err := k.Load(provider, nil)
	if err != nil {
		return nil, errors.Wrapf(err, "unable to load config from environment")
	}
	k.Print()
}

The reason for having this is to stop pollutiong the global environment state. Especially on test failures the current design becomes a source of problems for future tests.

---

Following alternatives are also possible:

1. Chaining the commands:

   env.Provider("APP_", ".", nil).WithEnviron([]string{"APP_USER=Joe"})
   

2. Changing the signature:

   env.Provider("APP_", ".", nil, []string{"APP_USER=Joe"})
   

   would break existing API

3. Adding a new init function:

   func ProviderWithEnviron(prefix, delim string, environ []string) *Env
   

   not sure how to specific callback function then.

[jxsl13]

imo, this testing aspect does not really belong into the provider.

I think that this is a pretty common use case where you can use the flexibility of koanf and multiple providers to achieve your goal:

package main

import (
	"encoding/json"
	"fmt"
	"os"
	"strings"

	"github.com/knadh/koanf/providers/confmap"
	"github.com/knadh/koanf/providers/env"
	"github.com/knadh/koanf/v2"
)

type Config struct {
	Name     string `koanf:"svc.name"`
	Port     int    `koanf:"svc.port"`
	Hostname string `koanf:"svc.hostname"`
}

func loadEnv(cfg any, override ...map[string]any) error {
	const (
		koanfDelim = "."
		envDelim   = "_"
	)

	envToKoanf := func(s string) string {
		return strings.ToLower(strings.ReplaceAll(s, envDelim, koanfDelim))
	}

	k := koanf.New(koanfDelim)
	err := k.Load(env.Provider("", envDelim, envToKoanf), nil)
	if err != nil {
		return err
	}

	for _, o := range override {
		mapped := make(map[string]any, len(o))
		for k, v := range o {
			mapped[envToKoanf(k)] = v
		}
		_ = k.Load(confmap.Provider(mapped, envDelim), nil)
	}

	return k.Unmarshal("", cfg)
}

func main() {
	// testing with
	os.Setenv("SVC_PORT", "1234")
	os.Setenv("SVC_NAME", "my-service")

	cfg := Config{}
	err := loadEnv(&cfg, map[string]any{
		"SVC_PORT":     3306,
		"SVC_HOSTNAME": "localhost",
	})
	if err != nil {
		panic(err)
	}

	data, _ := json.MarshalIndent(cfg, "", " ")
	fmt.Println(string(data))
	/*
		expected:
		{
		"Name": "my-service",
		"Port": 3306,
		"Hostname": "localhost"
		}
	*/

}

https://go.dev/play/p/ZsxOgp4YgBh

Maybe confmap might need an additional provider constructor that maps the keys before setting them to its internal map.

Koanf allows you to merge different configurations with more or less any kind of logic in between. Instead of overwriting existing keys you can completely skip the env.Provider part in case you have at least a single key in all passed overwrite maps.

package main

import (
	"encoding/json"
	"fmt"
	"os"
	"strings"

	"github.com/knadh/koanf/providers/confmap"
	"github.com/knadh/koanf/providers/env"
	"github.com/knadh/koanf/v2"
)

type Config struct {
	Name     string `koanf:"svc.name"`
	Port     int    `koanf:"svc.port"`
	Hostname string `koanf:"svc.hostname"`
}

func loadEnv(cfg any, override ...map[string]any) error {
	const (
		koanfDelim = "."
		envDelim   = "_"
	)

	envToKoanf := func(s string) string {
		return strings.ToLower(strings.ReplaceAll(s, envDelim, koanfDelim))
	}

	k := koanf.New(koanfDelim)

	num := 0
	for _, m := range override {
		num += len(m)
	}

	if num == 0 {
		err := k.Load(env.Provider("", envDelim, envToKoanf), nil)
		if err != nil {
			return err
		}
	} else {
		for _, o := range override {
			mapped := make(map[string]any, len(o))
			for k, v := range o {
				mapped[envToKoanf(k)] = v
			}
			_ = k.Load(confmap.Provider(mapped, envDelim), nil)
		}
	}

	return k.Unmarshal("", cfg)
}

func main() {
	// testing with
	os.Setenv("SVC_PORT", "1234")
	os.Setenv("SVC_NAME", "my-service")

	cfg := Config{}
	err := loadEnv(&cfg, map[string]any{
		"SVC_PORT":     3306,
		"SVC_HOSTNAME": "localhost",
	})
	if err != nil {
		panic(err)
	}

	data, _ := json.MarshalIndent(cfg, "", " ")
	fmt.Println(string(data))
	/*
		expected:
		{
		"Name": "",
		"Port": 3306,
		"Hostname": "localhost"
		}
	*/

	cfg2 := Config{}
	err = loadEnv(&cfg2)
	if err != nil {
		panic(err)
	}

	data, _ = json.MarshalIndent(cfg2, "", " ")
	fmt.Println(string(data))
	/*
		expected:
		{
		"Name": "my-service",
		"Port": 3306,
		"Hostname": ""
		}
	*/
}

https://go.dev/play/p/oDH9RshVMwt

[Eun]

Well you are correct, but you won’t be able to test the whole env logic, namely the callback functionality and whether the environment variable values land on the right path or not.

[jxsl13]

do you have an example of such a test?

my first thoughts would be:

• Koanf.All()
• maps.Flatten

[Eun]

Simple example: An app that load config only from environment. It needs custom logic to be able to parse a specific format from the environment variables.

package main

import (
	"os"
	"strings"
	"testing"

	"github.com/knadh/koanf/providers/env"
	koanf "github.com/knadh/koanf/v2"
	"github.com/mitchellh/mapstructure"
	"github.com/pkg/errors"
	"github.com/stretchr/testify/require"
)

type Config struct {
	Users  []string `conf_key:"users_credentials"`
	Admins []string `conf_key:"admins_credentials"`
}

func LoadConfig() (*Config, error) {
	k := koanf.New(".")

	err := k.Load(env.ProviderWithValue("APP_", ".", func(s, v string) (string, any) {
		s = strings.ToLower(s)
		s = strings.Replace(s, "app_", "", 1)

		parts := strings.FieldsFunc(v, func(r rune) bool {
			return r == ',' || r == ';' || r == '|'
		})

		switch len(parts) {
		case 0:
			return s, nil
		case 1:
			return s, parts[0]
		default:
			return s, parts
		}
	}), nil)
	if err != nil {
		return nil, errors.Wrapf(err, "unable to load config from environment")
	}

	var out Config

	unmarshalConf := koanf.UnmarshalConf{
		Tag:       "config_key",
		FlatPaths: false,
		DecoderConfig: &mapstructure.DecoderConfig{
			DecodeHook:       nil,
			TagName:          "config_key",
			Result:           &out,
			WeaklyTypedInput: true,
		},
	}
	if err := k.UnmarshalWithConf("", &out, unmarshalConf); err != nil {
		return nil, errors.Wrap(err, "unable to unmarshal config")
	}
	return &out, nil
}

func TestLoadConfig(t *testing.T) {
	os.Setenv("APP_USERS", "Joe,Alice")
	os.Setenv("APP_ADMINS", "root")
	defer os.Unsetenv("APP_USERS")
	defer os.Unsetenv("APP_ADMINS")

	cfg, err := LoadConfig()
	require.NoError(t, err)

	require.Equal(t, cfg, &Config{
		Users:  []string{"Joe", "Alice"},
		Admins: []string{"root"},
	})
}

[jxsl13]

This might work for your tests:

package main

import (
	"strings"
	"testing"

	"github.com/knadh/koanf/providers/confmap"
	"github.com/knadh/koanf/providers/env"
	"github.com/knadh/koanf/v2"
	"github.com/mitchellh/mapstructure"
	"github.com/pkg/errors"
	"github.com/stretchr/testify/require"
)

type Config struct {
	Users  []string `conf_key:"users_credentials"`
	Admins []string `conf_key:"admins_credentials"`
}

// if override is set no env variables are used
func LoadConfig(override ...map[string]string) (*Config, error) {
	k, err := loadEnv(envToKoanfValue, override...)
	if err != nil {
		return nil, errors.Wrap(err, "unable to load env variables")
	}
	var out Config

	unmarshalConf := koanf.UnmarshalConf{
		Tag:       "config_key",
		FlatPaths: false,
		DecoderConfig: &mapstructure.DecoderConfig{
			DecodeHook:       nil,
			TagName:          "config_key",
			Result:           &out,
			WeaklyTypedInput: true,
		},
	}
	if err := k.UnmarshalWithConf("", &out, unmarshalConf); err != nil {
		return nil, errors.Wrap(err, "unable to unmarshal config")
	}
	return &out, nil
}

func loadEnv(cb func(string, string) (string, any), override ...map[string]string) (*koanf.Koanf, error) {
	k := koanf.New(".")

	num := 0
	for _, m := range override {
		num += len(m)
	}

	if num == 0 {
		err := k.Load(env.ProviderWithValue("APP_", ".", cb), nil)
		if err != nil {
			return nil, errors.Wrapf(err, "unable to load config from environment")
		}

	} else {
		for _, o := range override {
			mapped := make(map[string]any, len(o))
			for k, v := range o {
				mk, mv := cb(k, v)
				mapped[mk] = mv
			}
			err := k.Load(confmap.Provider(mapped, "."), nil)
			if err != nil {
				return nil, err
			}
		}
	}

	return k, nil
}

func envToKoanfValue(k, v string) (string, any) {
	k = strings.ToLower(k)
	k = strings.Replace(k, "app_", "", 1)

	parts := strings.FieldsFunc(v, func(r rune) bool {
		return r == ',' || r == ';' || r == '|'
	})

	switch len(parts) {
	case 0:
		return k, nil
	case 1:
		return k, parts[0]
	default:
		return k, parts
	}
}

func TestLoadConfig(t *testing.T) {
	cfg, err := LoadConfig(map[string]string{
		"APP_USERS":  "Joe,Alice",
		"APP_ADMINS": "root",
	})
	require.NoError(t, err)

	require.Equal(t, cfg, &Config{
		Users:  []string{"Joe", "Alice"},
		Admins: []string{"root"},
	})
}

[Eun]

I am well aware that I can restructure my code in a way that the logic can be tested, I am not looking for a solution like this because it breaks the simplicity of the code.

[jxsl13]

in regard to the pr code:

• environ field might be prefereably private (smaller public api and easier to change later on)
• an additional constructor should be used that takes a map[string]string as (additional) parameter and constructs the string slice inside of the constructor making the provider easier to use

my two cents :).

[Eun]

I agree that having an addition constructor would be the cleanest, but what about the callback methods (that are used in the other two constructors)?

Which one should we choose?

[jxsl13]

I think you would need to extend ProviderWithValue to have an additional parameter map[string]string My problem is a little bit the proper name and another one would be that the parameter list starts to get bloated. Might make sense to implement a constructor with the option pattern and make all of the other constructors use that one. This is what I mean with the pattern: option implementation (private) https://github.com/jxsl13/amqpx/blob/main/pool/connection_pool_options.go#L14-L27

constructor implementation: https://github.com/jxsl13/amqpx/blob/1c7e895ed1bd7ee17fd460e78788bdf3272ddd0f/pool/connection_pool.go#L39

[Eun]

I am fine with the option implementation. Do you think its feasible to break api? Or should we keep the old constructors for compatibility reasons?

[knadh]

We can't break the constructor as env is a widely used provider.

There's an issue of ambiguity with this approach. With an additional variable, be it a constructor or a variable in the config, the provider switches to using os.Environ automatically if the variable is empty. What if it has to be legitimately empty? Here, the empty/len vs. nil check isn't explicit.

[Eun]

Good catch @knadh! So are you fine with adding a third constructor/init function that follows the option pattern?

func ProviderWithOptions(...Option) *Env

[jxsl13]

I'd say yes. Update your PR code and you will get feedback as fas as I can say.

[Eun]

I made the changes, please have a look now.

I removed some tests because the main logic now sits in ProviderWithOptions

[jxsl13]

Could you add an option that's called WithEnvironmentMap. I think the normal user would prefer using a key-value map instead of that list.

[jxsl13]

(Y)

[jxsl13]

@knadh

[knadh]

This works well, but thinking about the option function pattern. env would be the only bundled provider to use this pattern, which is not great for consistency.

[Eun]

This works well, but thinking about the option function pattern. env would be the only bundled provider to use this pattern, which is not great for consistency.

Yes that is correct, but what are the alternatives?

[knadh]

Sorry, picking this up again. You can maintain this implementation separately on your repo. We can list it on the README in case others want to use your implementation.

[Eun]

So this means you don't want to merge because of?

[knadh]

Don't want to introduce the functional option pattern into this provider which would be inconsistent with other providers. If there's more demand for this particular usecase, we can consider it.

[Eun]

Ok, would you accept a pr with a function like ProviderWithEnviron and we pass in the environment slice?

[knadh]

Sorry, lost track of this PR. Yes, ProviderWithEnviron() (derived from ProviderWithValue() to support key/value callbacks) makes sense.

[Eun]

@knadh Ok thanks for confirmation. Ill work an that as soon as I have some spare time.