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

added: cli option --dump-config prints loaded config and exits #1678

Merged
merged 2 commits into from
Dec 6, 2020

Conversation

wolfgangwalther
Copy link
Member

As discussed in #1659 (comment), this adds the CLI option --dump-config, which prints the config after loading and then exits. The printed config is itself a valid postgrest config file.

This is mostly useful as a base for future improvements. When adding aliases for some config options (#1668 (comment)) or reading options directly from environment variables (#1624, #1572), this can be used as the base to write proper tests.

Also added colors to our io-tests. They are all nice and green now. Much easier to spot the occasional red one...

test/io-tests.sh Outdated Show resolved Hide resolved
@wolfgangwalther
Copy link
Member Author

Lot's of haskell again, @steve-chavez. Looking forward to your feedback! I already learned a bit from hlint :D.

@wolfgangwalther
Copy link
Member Author

Cirrus seems to be hitting a build timeout of 15 minutes now, saw that for a couple of commits in different PRs now. @steve-chavez you added a timeout of 30 minutes a little time ago. Did they lower the timeout or what's happening?

src/PostgREST/Config.hs Outdated Show resolved Hide resolved
@steve-chavez
Copy link
Member

steve-chavez commented Dec 4, 2020

Cirrus seems to be hitting a build timeout of 15 minutes now, saw that for a couple of commits in different PRs now

@wolfgangwalther Hmm.. are you sure it's a timeout? It looks like it's failing because of low memory: haskell/cabal#5505.

I saw https://cirrus-ci.com/task/6377627252424704, that shows:

cabal: Failed to build hasql-implicits-0.1.0.2 (which is required by
exe:postgrest from postgrest-7.0.1). The build process was killed (i.e.
SIGKILL). The typical reason for this is that there is not enough memory
available (e.g. the OS killed a process using lots of memory).

Maybe we should try adding -j1 to cirrus(I remember -j1 didn't work before, but might work now).

(I'll try fixing this on another PR)

Edit: The-j1 seems to have worked. I added the change on #1600 (was also getting the cirrus error here).

@wolfgangwalther
Copy link
Member Author

@wolfgangwalther Hmm.. are you sure it's a timeout? It looks like it's failing because of low memory: haskell/cabal#5505.

Absolutely, the low memory makes sense. I did not look carefully - I just figured it was not a real build error. I had 3 cirrus builds failing at 15 minutes, so I was a bit too quick to assume it was a timeout thing.

Edit: The-j1 seems to have worked. I added the change on #1600 (was also getting the cirrus error here).

Sweet, so I won't look at this here and just see green where red is :)

@steve-chavez
Copy link
Member

The printed config is itself a valid postgrest config file.

I thought this would mean I could do postgrest --dump-config > pgrst.conf and generate a default config. But now I get it's mostly for tests. I'll give it a review now :)

@wolfgangwalther
Copy link
Member Author

wolfgangwalther commented Dec 5, 2020

I thought this would mean I could do postgrest --dump-config > pgrst.conf and generate a default config. But now I get it's mostly for tests.

I think we're missing 3 things to be able to do that:

  • the FILENAME argument must be optional. This is something that I have to change anyway to support "all-environment-only" configurations.
  • with --dump-config enabled, postgrest should not throw when missing one of the required settings. This is something that could happen with the environment variables approach - not 100% yet. Depends on implementation of that.
  • A good example config file needs comments, just like we provide them with the help output.

Maybe we can do that a bit different - what if we split the output of --help / the error output on stdout and stderr, so that only the example config file is on stdout and everything else on stderr? Then you should be able to do postgrest --help > example.config and have our example ready to go.

@steve-chavez
Copy link
Member

what if we split the output of --help / the error output on stdout and stderr, so that only the example config file is on stdout and everything else on stderr? Then you should be able to do postgrest --help > example.config and have our example ready to go.

That sounds good! Seems like a simpler implementation.

This is most useful for automated tests for upcoming configuration
features. Can also be used for debugging.
@wolfgangwalther
Copy link
Member Author

what if we split the output of --help / the error output on stdout and stderr, so that only the example config file is on stdout and everything else on stderr? Then you should be able to do postgrest --help > example.config and have our example ready to go.

That sounds good! Seems like a simpler implementation.

Had a look at that.. and the implementation is not as easy as thought. The whole thing is dumped by Options.Applicative.

Implementation-wise we could just do the following:

  • remove the example config from the options help output
  • add another CLI flag --dump-example-config to print just that

I think that's also better to use. WDYT?


Anyway, this would be something for a different PR. I addressed the other comments, CI is passing -> merge time!

@wolfgangwalther wolfgangwalther merged commit 8979442 into PostgREST:master Dec 6, 2020
@wolfgangwalther wolfgangwalther deleted the dump-config branch December 6, 2020 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants