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

feat: add more config examples to CLI #3481

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

taimoorzaeem
Copy link
Collaborator

@taimoorzaeem taimoorzaeem commented May 3, 2024

Closes #3248.

docs/references/cli.rst Outdated Show resolved Hide resolved
src/PostgREST/CLI.hs Show resolved Hide resolved
src/PostgREST/CLI.hs Outdated Show resolved Hide resolved
src/PostgREST/CLI.hs Outdated Show resolved Hide resolved
|
|## The standard connection URI format, documented at
|## https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING
|ALTER ROLE authenticator SET pgrst.db_uri = 'postgresql://';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a setting that is possible to set via database configuration. I suggest you go check the code which of those settings are actually possible to set via database config - because there's more than just this that don't make sense here at all.

src/PostgREST/CLI.hs Outdated Show resolved Hide resolved
Comment on lines +24 to +27
- #3248, Add more config examples to CLI - @taimoorzaeem
+ Now accepts ``--example-file``, ``--example-db``, and ``--example-env``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also have two changes here, one to rename the dump schema cache command and the other to remove --example and -e. Those should probably go into the changed section as well.

Comment on lines 247 to 248
[str|-- Enables the aggregate functions in postgresql
|-- ALTER ROLE authenticator SET pgrst.db_aggregates_enabled = 'false';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you added this - but only in the db settings. I guess we have quite a few settings which are not in those example config files by now, because that's always forgotten when adding a new config.

It would probably be good to go through all of them and add them to all three (two for those with are not db-configurable) examples.

Comment on lines 305 to 308
|
|-- Unix socket location
|-- if specified it takes precedence over server-port
|-- ALTER ROLE authenticator SET pgrst.server_unix_socket = '/tmp/pgrst.sock';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|
|-- Unix socket location
|-- if specified it takes precedence over server-port
|-- ALTER ROLE authenticator SET pgrst.server_unix_socket = '/tmp/pgrst.sock';

This is not in the list of dbSettingsNames.

@taimoorzaeem
Copy link
Collaborator Author

@wolfgangwalther Hmm, I have noticed an inconsistency between docs and dbSettingsNames. The dbSettingsNames include db_tx_end but the docs configuration page says that db_tx_end is n/a for in-db config. More the dbSettingsNames include raw_media_types but there is no detail available around this config in the documentation.

@wolfgangwalther
Copy link
Member

The dbSettingsNames include db_tx_end but the docs configuration page says that db_tx_end is n/a for in-db config.

A good catch. I added it in aef29d4.

More the dbSettingsNames include raw_media_types but there is no detail available around this config in the documentation.

We removed raw-media-types in v12.0.0 and replaced it with aggregation / media handlers. Removed the left-over in c6f152f.

@wolfgangwalther
Copy link
Member

Please rebase, solving conflicts with CHANGELOG.md.

CHANGELOG.md Outdated
Comment on lines 28 to 29
+ Replace ``-e`` and ``--example`` with ``--example-file``
+ Rename option ``--dump-schema`` to ``--dump-schema-cache``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they're kinda breaking changes, these should go under a separate subtitle (after the ### Fixed entries like in the 10.1.0 changelog), something like:

### Changed
 - #3248, Modified some CLI options - @taimoorzaeem
   + Replace ``-e`` and ``--example`` with ``--example-file``
   + Rename option ``--dump-schema`` to ``--dump-schema-cache``

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make these options non-breaking changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially proposed to not break the options in #3321 (review), but we agreed to only use the new ones at the end of that review... if we'd like to go back to not breaking the options, then we could use that implementation.

Copy link
Member

@laurenceisla laurenceisla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be commented since they do not have a default value, and should also show a sample value (similar to how db-anon-role works). Only checked for the changes in the --example-file, maybe others are missing (verify in the docs: if they have "Default: n/a" then they should be commented).

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

steve-chavez commented May 21, 2024

Hm, the newly added env vars and db options look hard to maintain.

I'm thinking we should somehow auto-generate them.

Also the in-db config format is not really future-proof as we're transitioning to a pre_config function: https://postgrest.org/en/v12/references/configuration.html#in-database-configuration

@laurenceisla
Copy link
Member

Hm, the newly added env vars and db options look hard to maintain.

I'm thinking we should somehow auto-generate them.

That's a good idea, we could use a sample configuration and the function used to detect valid in-db configs. Although I believe that it would be a bit tricky to implement (perhaps in another PR?).

Also the in-db config format is not really future-proof as we're transitioning to a pre_config function: https://postgrest.org/en/v12/references/configuration.html#in-database-configuration

Hmm... then maybe printing a sample function instead of modifying a role would be the best here? or maybe both?

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.

Add CLI page
4 participants