feat: Added precedence of CLI parameters over envs (#3190)

* feat: Added precedence of CLI parameters over envs * Update docs/usage/cli.rst Co-authored-by: Peter Schutt <[email protected]> * Remove redundant LitestarEnv fields and fix tests * Update docs/usage/cli.rst * Update litestar/cli/commands/core.py * Update docs/usage/cli.rst * Update docs/usage/cli.rst * Update litestar/cli/commands/core.py --------- Co-authored-by: kedod <kedod> Co-authored-by: Peter Schutt <[email protected]> Co-authored-by: Jacob Coffee <[email protected]>
litestar-org · Mar 18, 2024 · 615da3e · 615da3e
1 parent 19fd4e4
commit 615da3e
Show file tree

Hide file tree

Showing 5 changed files with 165 additions and 81 deletions.
diff --git a/docs/usage/cli.rst b/docs/usage/cli.rst
@@ -95,6 +95,9 @@ The ``run`` command executes a Litestar application using `uvicorn <https://www.
 
     This feature is intended for development purposes only and should not be used to deploy production applications.
 
+.. versionchanged:: 2.8.0
+    CLI options take precedence over environment variables!
+
 .. _cli-run-options:
 
 Options

diff --git a/litestar/cli/_utils.py b/litestar/cli/_utils.py
@@ -74,22 +74,11 @@ class LitestarEnv:
     """Information about the current Litestar environment variables."""
 
     app_path: str
-    debug: bool
     app: Litestar
     cwd: Path
     host: str | None = None
     port: int | None = None
-    fd: int | None = None
-    uds: str | None = None
-    reload: bool | None = None
-    reload_dirs: tuple[str, ...] | None = None
-    reload_include: tuple[str, ...] | None = None
-    reload_exclude: tuple[str, ...] | None = None
-    web_concurrency: int | None = None
     is_app_factory: bool = False
-    certfile_path: str | None = None
-    keyfile_path: str | None = None
-    create_self_signed_cert: bool = False
 
     @classmethod
     def from_env(cls, app_path: str | None, app_dir: Path | None = None) -> LitestarEnv:
@@ -119,31 +108,14 @@ def from_env(cls, app_path: str | None, app_dir: Path | None = None) -> Litestar
             loaded_app = _autodiscover_app(cwd)
 
         port = getenv("LITESTAR_PORT")
-        web_concurrency = getenv("WEB_CONCURRENCY")
-        uds = getenv("LITESTAR_UNIX_DOMAIN_SOCKET")
-        fd = getenv("LITESTAR_FILE_DESCRIPTOR")
-        reload_dirs = tuple(s.strip() for s in getenv("LITESTAR_RELOAD_DIRS", "").split(",") if s) or None
-        reload_include = tuple(s.strip() for s in getenv("LITESTAR_RELOAD_INCLUDES", "").split(",") if s) or None
-        reload_exclude = tuple(s.strip() for s in getenv("LITESTAR_RELOAD_EXCLUDES", "").split(",") if s) or None
 
         return cls(
             app_path=loaded_app.app_path,
             app=loaded_app.app,
-            debug=_bool_from_env("LITESTAR_DEBUG"),
             host=getenv("LITESTAR_HOST"),
             port=int(port) if port else None,
-            uds=uds,
-            fd=int(fd) if fd else None,
-            reload=_bool_from_env("LITESTAR_RELOAD"),
-            reload_dirs=reload_dirs,
-            reload_include=reload_include,
-            reload_exclude=reload_exclude,
-            web_concurrency=int(web_concurrency) if web_concurrency else None,
             is_app_factory=loaded_app.is_factory,
             cwd=cwd,
-            certfile_path=getenv("LITESTAR_SSL_CERT_PATH"),
-            keyfile_path=getenv("LITESTAR_SSL_KEY_PATH"),
-            create_self_signed_cert=_bool_from_env("LITESTAR_CREATE_SELF_SIGNED_CERT"),
         )
 
 

diff --git a/litestar/cli/commands/core.py b/litestar/cli/commands/core.py
@@ -103,6 +103,15 @@ def _run_uvicorn_in_subprocess(
     )
 
 
+class CommaSplittedPath(click.Path):
+    """A Click Path that splits the input string by commas.
+
+    .. versionadded:: 2.8.0
+    """
+
+    envvar_list_splitter = ","
+
+
 @command(name="version")
 @option("-s", "--short", help="Exclude release level and serial information", is_flag=True, default=False)
 def version_command(short: bool) -> None:
@@ -120,15 +129,32 @@ def info_command(app: Litestar) -> None:
 
 
 @command(name="run")
-@option("-r", "--reload", help="Reload server on changes", default=False, is_flag=True)
-@option("-R", "--reload-dir", help="Directories to watch for file changes", multiple=True)
+@option("-r", "--reload", help="Reload server on changes", default=False, is_flag=True, envvar="LITESTAR_RELOAD")
+@option(
+    "-R",
+    "--reload-dir",
+    help="Directories to watch for file changes",
+    type=CommaSplittedPath(),
+    multiple=True,
+    envvar="LITESTAR_RELOAD_DIRS",
+)
 @option(
-    "-I", "--reload-include", help="Glob patterns for files to include when watching for file changes", multiple=True
+    "-I",
+    "--reload-include",
+    help="Glob patterns for files to include when watching for file changes",
+    type=CommaSplittedPath(),
+    multiple=True,
+    envvar="LITESTAR_RELOAD_INCLUDES",
 )
 @option(
-    "-E", "--reload-exclude", help="Glob patterns for files to exclude when watching for file changes", multiple=True
+    "-E",
+    "--reload-exclude",
+    help="Glob patterns for files to exclude when watching for file changes",
+    type=CommaSplittedPath(),
+    multiple=True,
+    envvar="LITESTAR_RELOAD_EXCLUDES",
 )
-@option("-p", "--port", help="Serve under this port", type=int, default=8000, show_default=True)
+@option("-p", "--port", help="Serve under this port", type=int, default=8000, show_default=True, envvar="LITESTAR_PORT")
 @option(
     "-W",
     "--wc",
@@ -137,8 +163,9 @@ def info_command(app: Litestar) -> None:
     type=click.IntRange(min=1, max=multiprocessing.cpu_count() + 1),
     show_default=True,
     default=1,
+    envvar="WEB_CONCURRENCY",
 )
-@option("-H", "--host", help="Server under this host", default="127.0.0.1", show_default=True)
+@option("-H", "--host", help="Server under this host", default="127.0.0.1", show_default=True, envvar="LITESTAR_HOST")
 @option(
     "-F",
     "--fd",
@@ -147,16 +174,26 @@ def info_command(app: Litestar) -> None:
     type=int,
     default=None,
     show_default=True,
+    envvar="LITESTAR_FILE_DESCRIPTOR",
 )
-@option("-U", "--uds", "--unix-domain-socket", help="Bind to a UNIX domain socket.", default=None, show_default=True)
-@option("-d", "--debug", help="Run app in debug mode", is_flag=True)
-@option("-P", "--pdb", "--use-pdb", help="Drop into PDB on an exception", is_flag=True)
-@option("--ssl-certfile", help="Location of the SSL cert file", default=None)
-@option("--ssl-keyfile", help="Location of the SSL key file", default=None)
+@option(
+    "-U",
+    "--uds",
+    "--unix-domain-socket",
+    help="Bind to a UNIX domain socket.",
+    default=None,
+    show_default=True,
+    envvar="LITESTAR_UNIX_DOMAIN_SOCKET",
+)
+@option("-d", "--debug", help="Run app in debug mode", is_flag=True, envvar="LITESTAR_DEBUG")
+@option("-P", "--pdb", "--use-pdb", help="Drop into PDB on an exception", is_flag=True, envvar="LITESTAR_PDB")
+@option("--ssl-certfile", help="Location of the SSL cert file", default=None, envvar="LITESTAR_SSL_CERT_PATH")
+@option("--ssl-keyfile", help="Location of the SSL key file", default=None, envvar="LITESTAR_SSL_KEY_PATH")
 @option(
     "--create-self-signed-cert",
     help="If certificate and key are not found at specified locations, create a self-signed certificate and a key",
     is_flag=True,
+    envvar="LITESTAR_CREATE_SELF_SIGNED_CERT",
 )
 def run_command(
     reload: bool,
@@ -207,20 +244,8 @@ def run_command(
     env: LitestarEnv = ctx.obj
     app = env.app
 
-    reload_dirs = env.reload_dirs or reload_dir
-    reload_include = env.reload_include or reload_include
-    reload_exclude = env.reload_exclude or reload_exclude
-
-    host = env.host or host
-    port = env.port if env.port is not None else port
-    fd = env.fd if env.fd is not None else fd
-    uds = env.uds or uds
-    reload = env.reload or reload or bool(reload_dirs) or bool(reload_include) or bool(reload_exclude)
-    workers = env.web_concurrency or wc
-
-    ssl_certfile = ssl_certfile or env.certfile_path
-    ssl_keyfile = ssl_keyfile or env.keyfile_path
-    create_self_signed_cert = create_self_signed_cert or env.create_self_signed_cert
+    reload = reload or bool(reload_dir) or bool(reload_include) or bool(reload_exclude)
+    workers = wc
 
     certfile_path, keyfile_path = (
         create_ssl_files(ssl_certfile, ssl_keyfile, host)
@@ -263,7 +288,7 @@ def run_command(
                 port=port,
                 workers=workers,
                 reload=reload,
-                reload_dirs=reload_dirs,
+                reload_dirs=reload_dir,
                 reload_include=reload_include,
                 reload_exclude=reload_exclude,
                 fd=fd,

diff --git a/tests/unit/test_cli/test_core_commands.py b/tests/unit/test_cli/test_core_commands.py
@@ -3,7 +3,7 @@
 import re
 import sys
 from pathlib import Path
-from typing import Callable, Generator, List, Optional, Tuple
+from typing import Callable, Generator, List, Literal, Optional, Tuple, Union
 from unittest.mock import MagicMock
 
 import pytest
@@ -117,7 +117,7 @@ def test_run_command(
     else:
         uds = None
 
-    if fd:
+    if fd is not None:
         if set_in_env:
             monkeypatch.setenv("LITESTAR_FILE_DESCRIPTOR", str(fd))
         else:
@@ -151,7 +151,6 @@ def test_run_command(
             args.extend([f"--reload-exclude={s}" for s in reload_exclude])
 
     path = create_app_file(custom_app_file or "app.py", directory=app_dir)
-
     result = runner.invoke(cli_command, args)
 
     assert result.exception is None
@@ -257,6 +256,115 @@ def test_run_command_with_app_factory(
     )
 
 
+@pytest.mark.parametrize(
+    "cli, env, expected",
+    (
+        (
+            ("--reload", True),
+            ("LITESTAR_RELOAD", False),
+            "--reload",
+        ),
+        (
+            ("--reload-dir", [".", "../somewhere_else"]),
+            ("LITESTAR_RELOAD_DIRS", ["../somewhere_else3", "../somewhere_else2"]),
+            ["--reload-dir=.", "--reload-dir=../somewhere_else"],
+        ),
+        (
+            ("--reload-include", ["*.rst", "*.yml"]),
+            ("LITESTAR_RELOAD_INCLUDES", ["*.rst2", "*.yml2"]),
+            ["--reload-include=*.rst", "--reload-include=*.yml"],
+        ),
+        (
+            ("--reload-exclude", ["*.rst", "*.yml"]),
+            ("LITESTAR_RELOAD_EXCLUDES", ["*.rst2", "*.yml2"]),
+            ["--reload-exclude=*.rst", "--reload-exclude=*.yml"],
+        ),
+        (
+            ("--wc", 2),
+            ("WEB_CONCURRENCY", 4),
+            "--workers=2",
+        ),
+        (
+            ("--fd", 0),
+            ("LITESTAR_FILE_DESCRIPTOR", 1),
+            "--fd=0",
+        ),
+        (
+            ("--uds", "/run/uvicorn/litestar_test.sock"),
+            ("LITESTAR_UNIX_DOMAIN_SOCKET", "/run/uvicorn/litestar_test2.sock"),
+            "--uds=/run/uvicorn/litestar_test.sock",
+        ),
+        (
+            ("-d", True),
+            ("LITESTAR_DEBUG", False),
+            ("LITESTAR_DEBUG", "1"),
+        ),
+        (
+            ("--pdb", True),
+            ("LITESTAR_PDB", False),
+            ("LITESTAR_PDB", "1"),
+        ),
+    ),
+)
+def test_run_command_arguments_precedence(
+    cli: Tuple[str, Union[Literal[True], List[str], str]],
+    env: Tuple[str, Union[Literal[True], List[str], str]],
+    expected: str,
+    runner: CliRunner,
+    monkeypatch: MonkeyPatch,
+    mock_subprocess_run: MagicMock,
+    tmp_project_dir: Path,
+    create_app_file: CreateAppFileFixture,
+    mock_uvicorn_run: MagicMock,
+) -> None:
+    args = []
+    args.extend(["--app", f"{Path('my_app.py').stem}:app"])
+    args.extend(["--app-dir", str(Path(tmp_project_dir / "custom_subfolder"))])
+    args.extend(["run"])
+    create_app_file("my_app.py", directory="custom_subfolder")
+
+    env_name, env_value = env
+    cli_name, cli_value = cli
+
+    if env_name:
+        if isinstance(env_value, list):
+            monkeypatch.setenv(env_name, "".join(env_value))
+        else:
+            monkeypatch.setenv(env_name, env_value)  # type: ignore[arg-type] # pyright: ignore (reportGeneralTypeIssues)
+
+    if cli_name:
+        if cli_value is True:
+            args.append(cli_name)
+        elif isinstance(cli_value, list):
+            for value in cli_value:
+                args.extend([cli_name, value])
+        else:
+            args.extend([cli_name, cli_value])
+
+    result = runner.invoke(cli_command, args)
+
+    assert result.exception is None
+    assert result.exit_code == 0
+
+    if cli_name in ["--fd", "--uds"]:
+        mock_subprocess_run.assert_not_called()
+        if isinstance(expected, list):  # type: ignore[unreachable]
+            assert all(_ in mock_uvicorn_run.call_args_list[0].args[0] for _ in expected)  # type: ignore[unreachable]
+        else:
+            assert mock_uvicorn_run.call_args_list[0].kwargs.get(cli_name.strip("--")) == cli_value
+
+    elif cli_name in ["-d", "--pdb"]:
+        assert os.environ.get(expected[0]) == expected[1]
+
+    else:
+        mock_subprocess_run.assert_called_once()
+
+        if isinstance(expected, list):  # type: ignore[unreachable]
+            assert all(_ in mock_subprocess_run.call_args_list[0].args[0] for _ in expected)  # type: ignore[unreachable]
+        else:
+            assert expected in mock_subprocess_run.call_args_list[0].args[0]
+
+
 @pytest.fixture()
 def unset_env() -> Generator[None, None, None]:
     initial_env = {**os.environ}

diff --git a/tests/unit/test_cli/test_env_resolution.py b/tests/unit/test_cli/test_env_resolution.py
@@ -1,5 +1,4 @@
 from pathlib import Path
-from typing import Optional
 
 import pytest
 from _pytest.monkeypatch import MonkeyPatch
@@ -13,29 +12,6 @@
 pytestmark = pytest.mark.xdist_group("cli_autodiscovery")
 
 
-@pytest.mark.parametrize("env_name,attr_name", [("LITESTAR_DEBUG", "debug"), ("LITESTAR_RELOAD", "reload")])
-@pytest.mark.parametrize(
-    "env_value,expected_value",
-    [("true", True), ("True", True), ("1", True), ("0", False), (None, False)],
-)
-def test_litestar_env_from_env_booleans(
-    monkeypatch: MonkeyPatch,
-    app_file: Path,
-    attr_name: str,
-    env_name: str,
-    env_value: Optional[str],
-    expected_value: bool,
-) -> None:
-    monkeypatch.delenv(env_name, raising=False)
-    if env_value is not None:
-        monkeypatch.setenv(env_name, env_value)
-
-    env = LitestarEnv.from_env(f"{app_file.stem}:app")
-
-    assert getattr(env, attr_name) is expected_value
-    assert isinstance(env.app, Litestar)
-
-
 def test_litestar_env_from_env_port(monkeypatch: MonkeyPatch, app_file: Path) -> None:
     env = LitestarEnv.from_env(f"{app_file.stem}:app")
     assert env.port is None