add CLI to docs #3321

taimoorzaeem · 2024-03-09T17:27:04Z

Add more config examples to CLI
Add CLI docs page

CHANGELOG.md

laurenceisla · 2024-03-10T00:30:32Z

src/PostgREST/CLI.hs

+    exampleParser :: O.Parser CLI
+    exampleParser = 
+      CLIExample 
+        <$> showExample
+        <*> exampleConfig
+
+    exampleConfig =
+      O.strArgument $
+        O.metavar "ExampleType"
+        <> O.help "Type of config file/db/env"


Doing this does not show ExampleType as a parameter of --example and it shows these options instead:

Usage: postgrest [-v|--version] [[-e|--example] ExampleType | [--dump-config | --dump-schema-cache] [FILENAME]]

When it should look something like:

Usage: postgrest-12.0.2 [-v|--version] [-e|--example EXAMPLETYPE] [--dump-config | --dump-schema] [FILENAME]

You'll need to use directly strOption or strArgument (not sure which is better) and add file as a default value to avoid breaking changes. Here's an example on how to use this: https://www.fpcomplete.com/haskell/library/optparse-applicative/

src/PostgREST/CLI.hs

wolfgangwalther · 2024-03-10T18:10:01Z

I think the commit should be split into two: One adds a feature (feat: ...), the other a new page in the docs (docs: ...).

taimoorzaeem · 2024-03-16T04:17:08Z

@laurenceisla How about the interface now? It is as best as I could make yet. The only problem is that postgrest-run stopped working with this change. Could you help me figure this out? I'll add the comments etc in the following commits.

wolfgangwalther · 2024-03-16T14:23:08Z

src/PostgREST/CLI.hs

+      |
+      |PGRST_ADMIN_SERVER_PORT: 3001
+      |]
+exampleConfig _ = "print error" -- unreachable


It's better to avoid those "unreachable" lines with proper typing. Instead of [char], the argument should have a type like data ExampleType = ExampleFile | ExampleDB | ExampleEnv.

wolfgangwalther · 2024-03-16T14:30:23Z

src/PostgREST/CLI.hs

-data CLI = CLI
+data CLIOpts = CLIOpts
+  { example :: Example
+  , command :: Commands
+  }
+
+type Example = [Char]
+
+data Commands = Commands
  { cliCommand :: Command
  , cliPath    :: Maybe FilePath
  }


This typing is not very good - it leaves many invalid combinations possible. You could pass both a Command and a value for --example at the same time, for example. And it's not even possible to pass no example option?

I think you should leave things here untouched and promote --example to be a regular Command:

data Command = CmdRun | CmdDumpConfig | CmdDumpSchema | CmdExample ExampleType data ExampleType = ExampleFile | ExampleDB | ExampleEnv

Then build the parser accordingly.

The problem with adding CmdExample ExampleType to Command is that this is a value option. The other enums CmdDumpConfig and CmdDumpSchema are handled as flags. Combining them will complicate things in implementation as well. But maybe it is doable. I'll try this and get back to you.

Also using this would make the interface help look like this:

Usage: postgrest-12.0.2 [-v|--version] [--example EXAMPLETYPE | --dump-config | --dump-schema] [FILENAME]

instead of this more nice looking one:

Usage: postgrest-12.0.2 [-v|--version] [-e|--example EXAMPLETYPE] [--dump-config | --dump-schema] [FILENAME]

Also using this would make the interface help look like this:

Usage: postgrest-12.0.2 [-v|--version] [--example EXAMPLETYPE | --dump-config | --dump-schema] [FILENAME]

This means that --example EXAMPLETYPE, --dump-config and --dump-schema are mutually exclusive, which makes sense now. Before, you could do something like:

postgrest --example --dump-config # or postgrest --dump-config --example

And it will ignore the --dump-config and print the example directly. But now that --example has an argument, it complicates things as Wolfgang mentioned. So I think it's OK to have them mutually exclusive like that.

@laurenceisla I think the above is not possible to implement as it is inconsistent. Adding an argument option in Command that was previously just enum does not work.
As I have looked further into this, a pattern like:

data Command = ... | CmdExample ExampleType | Something Argument

is used in subparsers. ref. Hence, implementing the above is inconsistent if not impossible. How about using subparsers to lift the interface something like:

$ postgrest dump [--dump-config | --dump-schema-cache] $ postgrest example [--file | --db | --env]

I think it seems much simpler, extendible and easier to implement.

I think the above is not possible to implement as it is inconsistent.

Hm. The inconsistency seems to be that with my proposal it would be possible to add a config file name to the "example" command. Which doesn't make sense either.

How about using subparsers to lift the interface something like:

$ postgrest dump [--dump-config | --dump-schema-cache] $ postgrest example [--file | --db | --env]

This doesn't consider the FILENAME argument for the config file, which should be there in both the dump commands and the run commands (which is the default if nothing is provided), but not in the example case.

@laurenceisla
I have made the interface like you said earlier:

Usage: postgrest-12.0.2 [-v|--version] [--example EXAMPLETYPE | --dump-config | --dump-schema] [FILENAME]

Please confirm and review this design so I can move to testing.

Argh... this gets complicated due to the need of a parameter and default values for the --example. It's in conflict with the CLI type expected by the parser and generates the problems that both of you encountered along the way.

How about using subparsers to lift the interface something like:

I think this may be the way to go in the future. It would need a rework of the CLI, I suppose.

Please confirm and review this design so I can move to testing.

I'm sorry to keep going back and forth but what if, instead of using an option with value, we do the same as with --dump-*. I mean, to use:

--example-file, --example-env, --example-db.

This would work as an extension of the already used --example and wouldn't change nor break the way the CLI is currently used. The drawback is that it's more verbose (and maybe the --example alone may be confusing since it returns the file by default?)

@taimoorzaeem @wolfgangwalther WDYT? I'm adding how the parsing would go in a review below: #3321 (review)

laurenceisla

So this is how my suggestion in #3321 (comment) would be implemented (other minor changes may be needed too). It returns the following:

Usage: postgrest [-v|--version] 
                 [(-e|--example|--example-file) | --example-env | --example-db] 
                 [--dump-config | --dump-schema-cache] [FILENAME]

laurenceisla · 2024-04-09T23:47:57Z

src/PostgREST/CLI.hs

+  | CmdExample ExampleType
+
+data ExampleType = ExampleFile | ExampleDb | ExampleEnv


laurenceisla · 2024-04-09T23:48:54Z

src/PostgREST/CLI.hs

@@ -82,7 +87,8 @@ readCLIShowHelp =
  where
    prefs = O.prefs $ O.showHelpOnError <> O.showHelpOnEmpty
    opts = O.info parser $ O.fullDesc <> progDesc
-    parser = O.helper <*> versionFlag <*> exampleParser <*> cliParser
+    -- TODO: use subparser for mulitple commands
+    parser = O.helper <*> versionFlag <*> cliParser


Suggested change

parser = O.helper <*> versionFlag <*> cliParser

parser = O.helper <*> versionFlag <*> exampleParser <*> cliParser

laurenceisla · 2024-04-09T23:48:58Z

src/PostgREST/CLI.hs

+    parseExampleType :: O.ReadM Command
+    parseExampleType = O.maybeReader $ readString . map toLower
+      where
+        readString "file" = Just $ CmdExample ExampleFile
+        readString "db"   = Just $ CmdExample ExampleDb
+        readString "env"  = Just $ CmdExample ExampleEnv
+        readString _      = Just $ CmdExample ExampleFile -- default "file"
+
+
+    dumpExampleConfigFile =
+      O.option parseExampleType $
+        O.long "example"
+        <> O.short 'e'
+        <> O.metavar "EXAMPLETYPE"
+        <> O.help "Type of config file/db/env"


Suggested change

parseExampleType :: O.ReadM Command

parseExampleType = O.maybeReader $ readString . map toLower

where

readString "file" = Just $ CmdExample ExampleFile

readString "db" = Just $ CmdExample ExampleDb

readString "env" = Just $ CmdExample ExampleEnv

readString _ = Just $ CmdExample ExampleFile -- default "file"

dumpExampleConfigFile =

O.option parseExampleType $

O.long "example"

<> O.short 'e'

<> O.metavar "EXAMPLETYPE"

<> O.help "Type of config file/db/env"

exampleParser =

exampleParserFile <|> exampleParserEnv <|> exampleParserDb

exampleParserFile =

O.infoOption (exampleConfig ExampleFile) $

O.long "example"

<> O.long "example-file"

<> O.short 'e'

<> O.help "Show an example of a configuration file"

exampleParserEnv =

O.infoOption (exampleConfig ExampleEnv) $

O.long "example-env"

<> O.help "Show an example of environment variables configuration"

exampleParserDb =

O.infoOption (exampleConfig ExampleDb) $

O.long "example-db"

<> O.help "Show an example of in-database configuration"

Personally, I'd remove -e and --example. It's not like those are heavily used options, that need shortcuts - we can be explicit here.

steve-chavez · 2024-04-25T19:02:49Z

src/PostgREST/CLI.hs

+      |## Number of open connections in the pool
+      |ALTER ROLE authenticator SET pgrst.db_pool = '10';
+      |
+      |## Time in seconds to wait to acquire a slot from the connection pool
+      |# ALTER ROLE authenticator SET pgrst.db_pool_acquisition_timeout = '10';
+      |
+      |## Time in seconds after which to recycle pool connections
+      |# ALTER ROLE authenticator SET pgrst.db_pool_max_lifetime = '1800';
+      |
+      |## Time in seconds after which to recycle unused pool connections
+      |# ALTER ROLE authenticator SET pgrst.db_pool_max_idletime = '30';
+      |
+      |## Allow automatic database connection retrying
+      |# ALTER ROLE authenticator SET pgrst.db_pool_automatic_recovery = 'true';


Oh, these don't work. The available in-db settings are the ones here

postgrest/src/PostgREST/Config/Database.hs

Lines 45 to 71 in 85a4e35

dbSettingsNames :: [Text]

dbSettingsNames =

(prefix <>) <$>

["db_aggregates_enabled"

,"db_anon_role"

,"db_pre_config"

,"db_extra_search_path"

,"db_max_rows"

,"db_plan_enabled"

,"db_pre_request"

,"db_prepared_statements"

,"db_root_spec"

,"db_schemas"

,"db_tx_end"

,"jwt_aud"

,"jwt_role_claim_key"

,"jwt_secret"

,"jwt_secret_is_base64"

,"jwt_cache_max_lifetime"

,"openapi_mode"

,"openapi_security_active"

,"openapi_server_proxy_uri"

,"raw_media_types"

,"server_cors_allowed_origins"

,"server_trace_header"

,"server_timing_enabled"

]

steve-chavez · 2024-04-25T19:04:53Z

docs/references/cli.rst

+These commands show the example configuration file of the selected type.
+
+
+Config or Schema Cache


How about separating these? One section for config and another for the scache.

steve-chavez · 2024-04-25T19:07:38Z

docs/references/cli.rst

+  $ postgrest [--example-db]
+  $ postgrest [--example-env]


@taimoorzaeem Btw, if you'd like you could split this and do another PR with just the docs commit. That one is already mergeable.

Of course you'd need to remove the --example-db/--example/env in that commit since they belong to this PR.

Hmm, you're right. That would be much more manageable. Closing this PR. I would create 2 new PRs one after another for this change.

laurenceisla reviewed Mar 9, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

laurenceisla reviewed Mar 10, 2024

View reviewed changes

src/PostgREST/CLI.hs Outdated Show resolved Hide resolved

laurenceisla reviewed Mar 10, 2024

View reviewed changes

src/PostgREST/CLI.hs Outdated Show resolved Hide resolved

laurenceisla reviewed Mar 10, 2024

View reviewed changes

src/PostgREST/CLI.hs Outdated Show resolved Hide resolved

taimoorzaeem force-pushed the docs/cli branch from 3f92625 to 4edf771 Compare March 16, 2024 04:14

wolfgangwalther requested changes Mar 16, 2024

View reviewed changes

taimoorzaeem force-pushed the docs/cli branch from 4edf771 to 7838d66 Compare April 8, 2024 07:09

taimoorzaeem changed the title ~~docs: Add CLI page to docs~~ feat: add config type to --example in CLI Apr 8, 2024

laurenceisla reviewed Apr 9, 2024

View reviewed changes

taimoorzaeem changed the title ~~feat: add config type to --example in CLI~~ add CLI to docs Apr 11, 2024

taimoorzaeem force-pushed the docs/cli branch 2 times, most recently from 945e55d to a2c003f Compare April 13, 2024 07:38

taimoorzaeem added 2 commits April 24, 2024 22:18

feat: add more config examples to CLI

633f4e4

docs: add docs page for CLI

ad6f7b0

taimoorzaeem force-pushed the docs/cli branch from b262dac to ad6f7b0 Compare April 24, 2024 17:27

steve-chavez reviewed Apr 25, 2024

View reviewed changes

taimoorzaeem closed this Apr 27, 2024

taimoorzaeem deleted the docs/cli branch April 27, 2024 05:44

laurenceisla mentioned this pull request May 21, 2024

feat: add more config examples to CLI #3481

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add CLI to docs #3321

add CLI to docs #3321

taimoorzaeem commented Mar 9, 2024 •

edited

Loading

laurenceisla Mar 10, 2024 •

edited

Loading

wolfgangwalther commented Mar 10, 2024

taimoorzaeem commented Mar 16, 2024

wolfgangwalther Mar 16, 2024

wolfgangwalther Mar 16, 2024

taimoorzaeem Mar 16, 2024

taimoorzaeem Mar 16, 2024

laurenceisla Mar 26, 2024

taimoorzaeem Mar 29, 2024

wolfgangwalther Mar 30, 2024

wolfgangwalther Mar 30, 2024

taimoorzaeem Apr 8, 2024

laurenceisla Apr 9, 2024 •

edited

Loading

laurenceisla left a comment •

edited

Loading

laurenceisla Apr 9, 2024

laurenceisla Apr 9, 2024

laurenceisla Apr 9, 2024

wolfgangwalther Apr 20, 2024

steve-chavez Apr 25, 2024

steve-chavez Apr 25, 2024

steve-chavez Apr 25, 2024

taimoorzaeem Apr 27, 2024 •

edited

Loading

		\| CmdExample ExampleType

		data ExampleType = ExampleFile \| ExampleDb \| ExampleEnv

	parser = O.helper <> versionFlag <> cliParser
	parser = O.helper <> versionFlag <> exampleParser <*> cliParser

	dbSettingsNames :: [Text]
	dbSettingsNames =
	(prefix <>) <$>
	["db_aggregates_enabled"
	,"db_anon_role"
	,"db_pre_config"
	,"db_extra_search_path"
	,"db_max_rows"
	,"db_plan_enabled"
	,"db_pre_request"
	,"db_prepared_statements"
	,"db_root_spec"
	,"db_schemas"
	,"db_tx_end"
	,"jwt_aud"
	,"jwt_role_claim_key"
	,"jwt_secret"
	,"jwt_secret_is_base64"
	,"jwt_cache_max_lifetime"
	,"openapi_mode"
	,"openapi_security_active"
	,"openapi_server_proxy_uri"
	,"raw_media_types"
	,"server_cors_allowed_origins"
	,"server_trace_header"
	,"server_timing_enabled"
	]

		These commands show the example configuration file of the selected type.


		Config or Schema Cache

add CLI to docs #3321

add CLI to docs #3321

Conversation

taimoorzaeem commented Mar 9, 2024 • edited Loading

laurenceisla Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

wolfgangwalther commented Mar 10, 2024

taimoorzaeem commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurenceisla Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

laurenceisla left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taimoorzaeem Apr 27, 2024 • edited Loading

Choose a reason for hiding this comment

taimoorzaeem commented Mar 9, 2024 •

edited

Loading

laurenceisla Mar 10, 2024 •

edited

Loading

laurenceisla Apr 9, 2024 •

edited

Loading

laurenceisla left a comment •

edited

Loading

taimoorzaeem Apr 27, 2024 •

edited

Loading