Parse config from environmental variables #51
Conversation
code/config/lib/Loot/Config/Env.hs
Outdated
| -- | A complete description of parsing error. | ||
| data EnvParseError = EnvParseError | ||
| { errKey :: Text | ||
| , errValue :: Maybe String |
There was a problem hiding this comment.
I guess, this should not really be a Maybe (similar to parseEnvValue, see below).
code/config/lib/Loot/Config/Env.hs
Outdated
| -- | Describes a way to parse an item appearing in config. | ||
| class EnvValue a where | ||
| -- | Parse a variable value. | ||
| parseEnvValue :: Maybe String -> Parser a |
There was a problem hiding this comment.
I don’t quite understand why it takes a Maybe String. I think the case of Nothing needs to be handled uniformly somewhere upstream.
There was a problem hiding this comment.
I also don't like it much, but I was thinking like follows: currently, if a user wants to define a Maybe a option (so that e.g. Nothing from CLI could override Just a from Yaml config), he can parse to it in CLI and in Yaml, and absense of value will be interpreted as Nothing.
Probably it is worth being able to parse to Nothing in Env as well to preserve the mentioned feature. And generally user may want to have an ability to see in a parser whether env value was provided or not. But I can't say I know justified use cases for it.
What do you think?
There was a problem hiding this comment.
So, the way loot-config currently works (and I know it is not entirely clear from the docs and from the code structure – that’s why I want to rewrite it from scratch with a different design that emphasises monoids more) is it “automatically wraps” all config options into a Maybe, which indicates whether the option was or was not present in the (part of the) config. In this way all options are essentially considered required (so finalising the config will fail if some of them are missing). Since all options are required, it really doesn’t make a lot of sense to allow to “unset” them in parts of configuration.
However, users can “manually” make non-mandatory options, but wrapping them into a Maybe themselves. So, instead of a you’ll put Maybe a in your config declaration, and then loot-conf will wrap it in yet another Maybe and will work with Maybe (Maybe a) – the external maybe is its “internal” thing that shows whether the option was present in at least one config part, and it doesn’t care about the internal Maybe – it’s fully transparent for loot-config and, let’s say, managed by the user, so they can have a config (part) where they provide the option (they still have to provide the key!) with no value, and it will automatically get parsed as Nothing (or, at least, I hope it works this way 😓).
I’m afraid I already lost the line of my thought...
Anyway, the point is that, there is no code in loot-config that handles this latter case, it just works naturally (or, at least, is supposed to work naturally) because the FromJSON instance for Maybe a parses empty string as Nothing (again, just to be clear, this Maybe is not the one that loot-config adds, it comes from the user’s config spec and loot-config doesn’t know about it and doesn’t care). So, if you make sure that your FromEnv instance for Maybe a parses the empty string as Nothing, everything will just work™. There is no special case for this in loot-config now, and there is not need to have it in your new code.
There was a problem hiding this comment.
Holy shit, that’s so long, I hope it is possible to understand what I wrote. (That’s my punishment for not writing a design doc that would explain how loot-config actually works.)
There was a problem hiding this comment.
Oh yeah, sounds fair. I now see that I wrote some nonsense there, and that scenario I described was supported from the very beginning.
A substantial part of my motivation (which I somehow haven't expressed) was that user may want to declare a special type where the absence of env value is automatically interpreted as some special value (e.g. MyInt which is initiated as 0 unless the respective env var is set). But my recent practice shows that in most cases it is better to decouple parsing and default values, so the already supported approaches should be enough for all cases: when there is a static default value, we provide it as configFromFile <> defaultConfig, and if the business logic certainly needs to know whether the value was absent to perform some nontrivial computation, having Maybe a is enough.
In either case, I find this logic hacky and indeed everything gets simpler if we handle no value = Nothing case inside.
| withPresent $ either fail pure . Aeson.eitherDecode . encodeUtf8 | ||
|
|
||
| -- | Options which define the expected format of environmental variables. | ||
| data ParseOptions = ParseOptions |
There was a problem hiding this comment.
Let’s provide a helper for easily making keyBuilders that prefix the name. Realistically, in the absolute majority of cases you want all your env variables prefixed with <APP_NAME>_.
There was a problem hiding this comment.
Yeah, was also thinking about this scenario. I wanted people to use keyBuilder defaultOptions if they want to get the default behaviour of keyBuilder, but looks like no library usually designs it this way.
code/config/lib/Loot/Config/Env.hs
Outdated
| -- | Describes a way to parse an item appearing in config. | ||
| class EnvValue a where | ||
| -- | Parse a variable value. | ||
| parseEnvValue :: Maybe String -> Parser a |
There was a problem hiding this comment.
Holy shit, that’s so long, I hope it is possible to understand what I wrote. (That’s my punishment for not writing a design doc that would explain how loot-config actually works.)
|
Also, please, use meaningful titles in fixup commits – I usually review fix commits one by one, and it is easier when I know what each commit does :). |
|
Yeah, it also seems sane to me. It's just no one did ever complain about a stack of fixup commits before so I thought no one cares, but apparently with properly named commits it should be more convenient. |
You can write "!fixup bla-bla-bla" in the commit message title and then describe the essence of the fixup in the commit message below the first line. Also there is |
Problem: we already support parsing from yaml config and from CLI options, but in the place where I'm currently working it is common to use mostly environmental variables. Meanwhile, I failed to find any decent solution for this which would do exactly environment parsing and nothing more. If we implement env parsing here, this can become the superior library in the Haskell ecosystem solving this task. Solution: implement env parsing. Variable name is evaluted from the full path to the variable in config, and values are parsed using the new dedicated typeclass.
Problem: unlike with parsing config or CLI parameters, with env variables it is not always obvious which exact variables the config expects. In case if name unexpected for a user is generated, this can be difficult to debug. Solution: provide a method which enlists all env keys expected by configuration.
Fix indentation, apply new stylish-haskell.
Martoon-00
force-pushed
the
martoon/config-from-env
branch
from
734210c to
2c04755
Compare
This PR provides a set of flexible methods to parse configuration from environmental variables. The user has to have an instance of the dedicated typeclass for each type appearing in the configuration. By default we fetch all the env variables, even ones not required by config, but the user can supply his own version of environment if he wishes, e.g. app's actual env + context of dotenv file.
TODOs:
timepackage, base64 bytestring wrapper, maybe something else)?Textat places, butRecord.hsusesStringeverywhere, should I do the same?