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

Proposal: separate REPL and server into two different commands #2173

Open
mark-rushakoff opened this issue Mar 5, 2020 · 2 comments
Open

Comments

@mark-rushakoff
Copy link
Contributor

mark-rushakoff commented Mar 5, 2020

(Split out of discussion in #2161).

Patrick pointed out that wording in the help output of opa run needs to be careful to not assume REPL mode, as presumably most people use opa run -s for the server mode.

I would propose that the REPL is a very different use case from the opa server, and therefore the two should be in separate subcommands. Some options include:

  • Deprecate opa run altogether and introduce two new commands, perhaps opa serve and opa repl.
  • Keep using opa run -s for the server so people don't have to change scripts, but deprecate plain opa run so that -s will be implied in a future version of opa. Then for the REPL:
    • Move REPL to opa repl...
    • Or merge opa repl with opa eval, perhaps with a new flag like --interactive.

Separating the two commands will also allow the command-line flags to be simplified. The supported flags for opa run fall into three categories, which I've quickly grouped as:

Apply to both REPL and Server

  -b, --bundle                               load paths as bundle files or root directories
  -c, --config-file string                   set path of configuration file
  -h, --help                                 help for run
      --ignore strings                       set file and directory names to ignore during loading (e.g., '.*' excludes hidden files)
      --log-format {text,json,json-pretty}   set log format (default json)
  -l, --log-level {debug,info,error}         set log level (default info)
  -m, --max-errors int                       set the number of errors to allow before compilation fails early (default 10)
      --set stringArray                      override config values on the command line (use commas to specify multiple values)
      --set-file stringArray                 override config values with files on the command line (use commas to specify multiple values)
  -w, --watch                                watch command line files for changes

Apply to REPL only:

  -f, --format string                        set shell output format, i.e, pretty, json (default "pretty")
  -H, --history string                       set path of history file (default "/Users/mr/.opa_history")

Apply to Server only:

  -a, --addr strings                         set listening address of the server (e.g., [ip]:<port> for TCP, unix://<path> for UNIX domain socket) (default [:8181])
      --authentication {token,tls,off}       set authentication scheme (default off)
      --authorization {basic,off}            set authorization scheme (default off)
      --pprof                                enables pprof endpoints
  -s, --server                               start the runtime in server mode
      --shutdown-grace-period int            set the time (in seconds) that the server will wait to gracefully shut down (default 10)
      --tls-ca-cert-file string              set path of TLS CA cert file
      --tls-cert-file string                 set path of TLS certificate file
      --tls-private-key-file string          set path of TLS private key file

I may have miscategorized some of them, but that should be roughly correct.

I see that cmd/flags.go was created around March 2019 but cmd/run.go was created in 2016. Today, cmd/flags provides a place to group common flags if the first group makes sense to keep present in both the REPL and server commands.

Splitting the commands will also allow a more expanded help text specific to using the REPL, which can be more helpful to newcomers to Rego.


Edit history:

  • moved --watch flag to "both" category
@tsandall
Copy link
Member

tsandall commented Mar 5, 2020

👍 this is definitely an area that could be improved. I don't have too many strong opinions but I would like to at least see some other options. E.g., one way we could improve things would be to create a separate serve (or repl) subcommand and then leave run as is, deprecating the relocated functionality over time. I'd lean toward moving the server into a separate command because the default is currently the REPL. Also, from a UX point of view, I think opa run ... is nice for people wanting to tinker with things.

In either case, we ought to think about improvements to server defaults that could be made after the switch. Top of my list are:

  1. Making the server more secure by default. For example, binding to localhost by default instead of all interfaces.
  2. Enabling the console decision logger and removing the Apache-style access logs.
  3. Detecting the terminal and using either a "pretty" log format or a compact "json" log format.

The categories look good to me, aside from --watch. That works for both REPL and server mode.

@tsandall tsandall added the design label Mar 5, 2020
@mark-rushakoff mark-rushakoff changed the title Proposal: split REPL out of opa run Proposal: separate REPL and server into two different commands Mar 5, 2020
@stale
Copy link

stale bot commented Nov 22, 2021

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants