chore: allow to use custom environment #214

Eun · 2023-04-19T12:40:43Z

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:

Chaining the commands:

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

Changing the signature:

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

would break existing API

Adding a new init function:

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

not sure how to specific callback function then.

jxsl13 · 2023-04-19T17:40:58Z

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 · 2023-04-19T17:44:57Z

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 · 2023-04-19T17:51:51Z

do you have an example of such a test?

my first thoughts would be:

Koanf.All()
maps.Flatten

Eun · 2023-04-19T18:22:32Z

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 · 2023-04-19T19:08:17Z

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 · 2023-04-19T19:33:21Z

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 · 2023-04-19T19:45:35Z

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 · 2023-04-19T19:49:37Z

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 · 2023-04-19T21:27:11Z

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 · 2023-04-20T05:07:09Z

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 · 2023-04-20T06:32:15Z

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 · 2023-04-20T08:06:00Z

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

func ProviderWithOptions(...Option) *Env

jxsl13 · 2023-05-02T10:32:13Z

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

Eun · 2023-05-02T13:13:06Z

I made the changes, please have a look now.

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

jxsl13 · 2023-05-02T13:38:19Z

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.

providers/env/env.go

jxsl13 · 2023-06-19T17:15:07Z

(Y)

jxsl13 · 2023-06-19T17:15:37Z

@knadh

providers/env/env.go

knadh · 2023-06-23T05:22:35Z

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 · 2023-06-23T05:26:43Z

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 · 2023-09-27T18:17:02Z

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 · 2023-09-27T18:36:56Z

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

knadh · 2023-09-27T18:42:40Z

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 · 2023-09-27T18:47:24Z

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

knadh · 2023-11-26T08:12:29Z

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

Eun · 2023-12-11T08:53:58Z

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

allow custom environment

959dbf8

add options

8f08c02

add WithEnvironMap

35c9ffd

StandoffVenus reviewed Jun 16, 2023

View reviewed changes

providers/env/env.go Outdated Show resolved Hide resolved

use func for getting the environment

4631660

jxsl13 reviewed Jun 22, 2023

View reviewed changes

providers/env/env.go Outdated Show resolved Hide resolved

jxsl13 approved these changes Jun 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: allow to use custom environment #214

chore: allow to use custom environment #214

Eun commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 19, 2023

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 19, 2023

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 20, 2023

knadh commented Apr 20, 2023

Eun commented Apr 20, 2023

jxsl13 commented May 2, 2023

Eun commented May 2, 2023

jxsl13 commented May 2, 2023 •

edited

jxsl13 commented Jun 19, 2023

jxsl13 commented Jun 19, 2023

knadh commented Jun 23, 2023

Eun commented Jun 23, 2023

knadh commented Sep 27, 2023

Eun commented Sep 27, 2023

knadh commented Sep 27, 2023

Eun commented Sep 27, 2023 •

edited

knadh commented Nov 26, 2023

Eun commented Dec 11, 2023

chore: allow to use custom environment #214

Are you sure you want to change the base?

chore: allow to use custom environment #214

Conversation

Eun commented Apr 19, 2023 • edited

jxsl13 commented Apr 19, 2023 • edited

Eun commented Apr 19, 2023 • edited

jxsl13 commented Apr 19, 2023 • edited

Eun commented Apr 19, 2023 • edited

jxsl13 commented Apr 19, 2023 • edited

Eun commented Apr 19, 2023

jxsl13 commented Apr 19, 2023 • edited

Eun commented Apr 19, 2023

jxsl13 commented Apr 19, 2023 • edited

Eun commented Apr 20, 2023

knadh commented Apr 20, 2023

Eun commented Apr 20, 2023

jxsl13 commented May 2, 2023

Eun commented May 2, 2023

jxsl13 commented May 2, 2023 • edited

jxsl13 commented Jun 19, 2023

jxsl13 commented Jun 19, 2023

knadh commented Jun 23, 2023

Eun commented Jun 23, 2023

knadh commented Sep 27, 2023

Eun commented Sep 27, 2023

knadh commented Sep 27, 2023

Eun commented Sep 27, 2023 • edited

knadh commented Nov 26, 2023

Eun commented Dec 11, 2023

Eun commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

Eun commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

jxsl13 commented Apr 19, 2023 •

edited

jxsl13 commented May 2, 2023 •

edited

Eun commented Sep 27, 2023 •

edited