search

kelseyhightower / envconfig

Golang library for managing configuration data from environment variables
MIT License
5.06k stars 382 forks source link

Adding build tags to use `os.LookupEnv` in >=go1.5 #65

Closed jprobinson closed 8 years ago

jprobinson commented 8 years ago

This PR adds build tags and a small level of indirection to swap out syscall.Getenv for os.LookupEnv in environments with >=go1.5.

I'm currently doing work with App Engine and would love to use github.com/NYTimes/gizmo/pubsub, but this is the one dependency left in there that makes a reference to the syscall package which is not available in the AE standard environment. This PR will enable the use of envconfig in App Engine.

teepark commented 8 years ago

Really we want to differentiate between appengine and non-appengine right? It looks like there's a specific build tag for that, why don't we use that one instead?

I'm thinking, for instance, that google could offer a newer golang on appengine, but still probably wouldn't have the syscall package available.

jprobinson commented 8 years ago

I guess I went the route of also opening up os.LookupEnv to all users who it is available for as the comments in this package make it sound like we should be using it going forwards anyways:

https://github.com/kelseyhightower/envconfig/blob/master/envconfig.go#L108-L110

        // `os.Getenv` cannot differentiate between an explicitly set empty value
        // and an unset value. `os.LookupEnv` is preferred to `syscall.Getenv`,
        // but it is only available in go1.5 or newer.

I'm fine for just locking it down to appengine if you things that's the better route, though.

jprobinson commented 8 years ago

I've updated to only use the appengine tag. If folks want to add other tags for newer versions at a later date, at least we have a hook now.

teepark commented 8 years ago

Thanks for the contribution @jprobinson! Sorry I was slow with follow-up response there.

I'm not really sure why os.LookupEnv would be preferable to syscall.Getenv besides maybe appengine compatibility -- in fact the source really suggests it's not an important difference :)

Thanks again!