added WithGetenvFunc option

[tempcke]

added WithGetenvFunc option so that from tests we do not have to rely on global state, defaults to os.Getenv still

[tempcke]

just to add some color as to why this is useful. I had the idea after reading Mat Ryer's post https://grafana.com/blog/2024/02/09/how-i-write-http-services-in-go-after-13-years/

where he calls run from main() with

func run(
    ctx    context.Context,
    args   []string,
    getenv func(string) string,
    stdin  io.Reader,
    stdout, stderr io.Writer,
) error

which allows him to test everything without worrying about conflicting global state and I really liked the idea.

[peterbourgon]

I don't think I grok the use case, here. In any case that you would pass a "getenv" func that pulled from something other than the true os.Environ, why wouldn't you pass flags as args directly?

edit: Is it just about tests?

[tempcke]

yes, the only time you would ever not pass in os.Getenv is when trying to write a test against run itself. This change hurts nothing and changes no existing behavior but it does add a slight advantage to testing.

Even in your own tests you are setting the env vars and then defer them back to the original values and can not run tests in parallel because of it.

[peterbourgon]

Got it.

This change hurts nothing and changes no existing behavior but it does add a slight advantage to testing.

I don't agree. Every addition to the API of a module adds cognitive overhead and complexity -- costs that need to be justified with commensurate benefits. This is a non-trivial change with benefits that aren't immediately clear to me. Just need to think it through.

[peterbourgon]

Doesn't seem like this juice is worth the squeeze.