Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch postgrest configurator-ng to dhall configure for latest ghc build #1292

Open
clojurians-org opened this issue May 14, 2019 · 8 comments
Labels
config related to the configuration options

Comments

@clojurians-org
Copy link

clojurians-org commented May 14, 2019

the depends on configurator-ng(and it's further depend on critbit) is too old to adapt to new ghc environment.
it's only used for simple config parser function.

i currently try to use nix to build static build for postgrest, there's a little problem to solve.
but dynamic build is relative easy on postgrest for latest GHC after adopting dhall instead of configurator-ng. (the Ranged-sets can be upgrade to RELEASE-0.4.0 version)

a simple build scratch version:
https://github.com/clojurians-org/nix-haskell-build/blob/master/postgrest/src/PostgREST/Config.hs

@steve-chavez
Copy link
Member

Thank you for the suggestion. Do you know if there's a way for dhall to accept our current configuration syntax(without curly brackets and commas)?

@robx
Copy link
Contributor

robx commented May 15, 2019

While I don't object at all to moving away from configurator-ng (it seems quite abandoned), as a less invasive short-term fix I tried updating critbit and configurator-ng for current GHC: lpsmith/configurator-ng#9 haskell/critbit#93

It's unclear to me whether those will make it to hackage anytime soon though.

EDIT: As a way to move forward here: #1305

@Qu4tro
Copy link
Contributor

Qu4tro commented May 22, 2019

I fully support the move to dhall. Not only is well supported, it also brings both flexibility and ease with it.

@steve-chavez I think the conversion from currents configurator-ng config to dhall ones, can be automated. Would this settle worries about backwards incompatibility?
We would then provide said tool, so that people could use to effortlessly update their config to the new version.

This would also simplify configuration code-wise. The example from @clojurians-org already simplifies the Config.hs module so much. dhall also supports reading from ref files, so we should be able to remove loadSecretFile and loadDbUriFile from Main.hs as well.

@ruslantalpa
Copy link
Contributor

Unless dhall can support the exact config format @robx has the right approach, fix upstream. Maintainers should be contacted over email and asked respectfully to merge (when they like the code) the pr and if they are willing, until the change makes it into hackage, postgrest should use directly the branches with the fixes.

There is no need for complex configuration capabilities, having a simple configuration file is a goal for postgrest to have and it does not get any simpler the key=value and its a format everyone knows and understands as opposed to dhall

@Qu4tro
Copy link
Contributor

Qu4tro commented May 22, 2019

I agree that fixing upstream should be the approach to take to fix this issue. Contacting the maintainers and use the fixed branch in the meantime.

I disagree with you, because the configuration file PostgREST uses, obeys a few more rules than simply key=value . For certain keys, if a value starts with @, it is interpreted as a path and that file is read instead. That feature is builtin with dhall.
I also don't agree that having complex configuration capabilities goes against having a simple configuration file. The configuration file can stay simple, i.e. stay clear of nesting and not forcing the user to use advanced features, but we can still enjoy the advanced configuration capabilities that dhall provides, i.e. pool size depending on db-uri, due to different staging/production capabilities.
And it doesn't get that much harder. If advanced features are avoided a valid dhall config is basically wrapping the current config in brackets and adding a comma for each non-empty line.
This is my opinion as user.

My opinion as a developer is that it is a harder change, because it would involve updating a number of dependencies and adding several more tests. It would be nice, because we could get rid of quite a bit of code, especially one so error prone, that's arguably mostly unrelated to PostgREST itself. Mostly that.

@robx
Copy link
Contributor

robx commented Jun 5, 2019

With #1312, PostgREST can now be built with GHC 8.6.3, compare #1319.

Of course switching to dhall might still be a good idea?

@steve-chavez
Copy link
Member

Really interesting podcast about dhall: https://www.se-radio.net/2019/08/episode-375-gabriel-gonzalez-on-configuration

@steve-chavez
Copy link
Member

We still have #937.
Also, as Qu4tro mentioned, with dhall we'd solve #1130 for free.

I think our format is similar to toml, if a toml to dhall conversion is possible we could maintain backwards compatibility and migrate to dhall.

@steve-chavez steve-chavez added the config related to the configuration options label Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config related to the configuration options
Development

No branches or pull requests

5 participants