search

byronwasti / movine

A migration manager written in Rust that attempts to be smart yet minimal
MIT License
109 stars 9 forks source link

Support configuration from environment variables #2

Closed connec closed 4 years ago

connec commented 4 years ago

I've taken a quick stab at adding environment variable support in the least intrusive way I could think of using envy and only attempting to load when loading the configuration file fails. I updated the README.md with instructions for using file or environment configuration.

I've also added dotenv since this seems like quite a common tool for CLI configuration. It's a shame it's included even when installed as a library, it seems there's an RFC to support per-target dependencies but no conclusion yet.

Any errors encountered by Config::from_env are ignored, including IO errors when accessing the environment or missing environment variables, which I'm not totally happy with but I'm not sure how best to handle those without ending up with a slightly awkward error message ("missing movine.toml or required environment variables"?).

I see this has come up already in https://github.com/byronwasti/movine/issues/1#issuecomment-615497929, so if you are thinking about another approach I'm happy to close this or take your feedback on how you would like to see it implemented!

byronwasti commented 4 years ago

Thanks for the PR! It looks good, and I'm happy to learn about the envy crate. Thanks for updating the README.md as well!

connec commented 4 years ago

Awesome! Thank you. I'd be happy to look at another couple of things that would be useful to me if you think they'd be useful?

byronwasti commented 4 years ago

That would be fantastic if you want to tackle those things!

I was planning on reworking the config loading into two stages: (1) load from the config file or environment variables into config structs full of Option<T>s and then (2) merge into one configuration (with no Option<T>s). I believe this should allow nice mix-and-match between config file & env variables (which might be nice for things like passwords) while allowing better error-handling functionality and defaults.

You should feel no obligation to do anything like that though! I just wanted to share some context in case you have thoughts/other ideas. I happily welcome any incremental improvements to the error handling/defaults handling. I also want to make sure I don't step on your toes, so if you think it would be nice to have that refactor done first I'm happy to crank that out this weekend.

connec commented 4 years ago

I probably won't have much more time this weekend and don't want to block any planned improvements in the meantime, so I'll probably check in in a few days and see what's what. Happy to try the approach you've mentioned once I have time, though, if you have other priorities.

byronwasti commented 4 years ago

Of course, just wanted to let you know my plans. I took a stab at reworking the config stuff, and it has turned out trickier than I thought. Getting nice error messages is a pain! I've got a branch at bw/merging-configs which is me taking a stab at things. It should wrap up your first two issues: slightly better error messages and default parameters. I'm still working through things, but I should also be able to add the DATABASE_URL support in it as well. Depending on what comes up I should hopefully be able to get this merged in the next few days!

connec commented 4 years ago

I ended up taking a shot as well (I also thought it was trickier than expected!): https://github.com/byronwasti/movine/compare/master...connec:config-refactor

I prefer your error handling, rather than the approach I took of building a Vec<(&str, &str)>. I see you also collapsed config into adaptor – I don't have a strong opinion, but I kept them separate. I did remove the into_* methods and kept it to the loading and merging, but added from_config methods to the adaptors to keep a similar API (as in the README).

Let me know if you think any of this stuff is useful, and I'd be happy to work them together somehow if so!

byronwasti commented 4 years ago

Awesome! Thanks for giving it a go as well. I collapsed config into adaptor mainly because I wanted to see how things would look if all database-specific code was in adaptor. l have mixed feelings about it, and will likely switch it back.

Your branch is super useful for getting a different perspective. I like your addition of from_config instead of into_*. I had a very similar merge setup initially. It would be nice if Rust had a similar spread operator to Python, then we could just { **config_a, **config_b } and get all the nice merging and overriding features by default!

I'll hopefully be able to spend some time in the next few days and take ideas from both of our branches. I think you took the right approach not trying to change everything at once, I was definitely overly optimistic with my refactoring.

Now having some perspective on the problem, I'm thinking a 3-stage solution might be the way to go. (1) Load the data from environment variables and config files (2) determine which adaptor to move forward with (and how, since the database_url should override everything but maybe with a warning in the logs?) and (3) build the structs for the adaptors and throw errors if missing params.

My reasoning for the 3-stage solution is mainly for error propagation reasons. (1) will throw on filesystem errors (specifically, if the config file fails on some bizarre IO error), (2) will throw on not having any environment variables or config files specified at all, and (3) can give nice adaptor-specific error messages.

In any case, if I get something I like I'll throw up a PR and tag you in it. And if you end up doing the same I'm happy to take a look and merge. It is arguable how much effort into refactoring should be done rather than just getting the feature support in and cleaning up incrementally afterwards.