Organize Gunicorn and Uvicorn server settings

#2
#3
ff9155a

The core logic for running the Uvicorn and Gunicorn+Uvicorn servers is
located in start.py. The `start.start_server()` method is what actually
starts the servers. Uvicorn and Gunicorn are configured differently, so
and the `start.start_server()` method ended up getting quite complex in
order to handle these differences.

This commit will refactor the configuration options passed to Gunicorn
and Uvicorn into separate functions. The result is a start script with
the same programming API and almost exactly the same line length, but
improved readability and separation of concerns.

- Refactor Gunicorn and Uvicorn options into separate functions,
  `start.set_gunicorn_options()` and `start.set_uvicorn_options()`.
- Remove `start.set_conf_path()`: this function was written in a general
  way to find either Gunicorn or logging configuration files. Starting
  with ff9155a, it became used only for Gunicorn configuration files.
  Now that the configuration options for Gunicorn are in a separate
  function, the logic from `set_conf_path()` can be moved there.
- Simplify logger type annotations: simply `import logging` and annotate
  functions with `logging.Logger` instead of having a separate import
  for `from logging import Logger`.
- Reorganize test_start.py to more clearly reflect order of start.py

inboard/start.py

@@ -4,27 +4,14 @@
 import logging.config
 import os
 import subprocess
-from logging import Logger
 from pathlib import Path
 from typing import Optional
 
 import uvicorn  # type: ignore
 
 
-def set_conf_path(module_stem: str) -> str:
-    """Set the path to a configuration file."""
-    conf_var = str(
-        os.getenv(f"{module_stem.upper()}_CONF", f"/app/inboard/{module_stem}_conf.py")
-    )
-    if conf_var and Path(conf_var).is_file():
-        conf_path = conf_var
-    else:
-        raise FileNotFoundError(f"Unable to find {conf_var}")
-    return conf_path
-
-
 def configure_logging(
-    logger: Logger = logging.getLogger(),
+    logger: logging.Logger = logging.getLogger(),
     logging_conf: str = os.getenv("LOGGING_CONF", "inboard.logging_conf"),
 ) -> dict:
     """Configure Python logging based on a path to a logging module or file."""
@@ -52,20 +39,7 @@ def configure_logging(
         raise
 
 
-def set_app_module(logger: Logger = logging.getLogger()) -> str:
-    """Set the name of the Python module with the app instance to run."""
-    try:
-        app_module = str(os.getenv("APP_MODULE"))
-        if not importlib.util.find_spec((module := app_module.split(sep=":")[0])):
-            raise ImportError(f"Unable to find or import {module}")
-        logger.debug(f"App module set to {app_module}.")
-        return app_module
-    except Exception as e:
-        logger.error(f"Error when setting app module: {e.__class__.__name__} {e}.")
-        raise
-
-
-def run_pre_start_script(logger: Logger = logging.getLogger()) -> str:
+def run_pre_start_script(logger: logging.Logger = logging.getLogger()) -> str:
     """Run a pre-start script at the provided path."""
     logger.debug("Checking for pre-start script.")
     pre_start_path = os.getenv("PRE_START_PATH", "/app/inboard/app/prestart.py")
@@ -81,42 +55,67 @@ def run_pre_start_script(logger: Logger = logging.getLogger()) -> str:
     return message
 
 
+def set_app_module(logger: logging.Logger = logging.getLogger()) -> str:
+    """Set the name of the Python module with the app instance to run."""
+    try:
+        app_module = str(os.getenv("APP_MODULE"))
+        if not importlib.util.find_spec((module := app_module.split(sep=":")[0])):
+            raise ImportError(f"Unable to find or import {module}")
+        logger.debug(f"App module set to {app_module}.")
+        return app_module
+    except Exception as e:
+        logger.error(f"Error when setting app module: {e.__class__.__name__} {e}.")
+        raise
+
+
+def set_gunicorn_options() -> list:
+    """Set options for running the Gunicorn server."""
+    gunicorn_conf_path = os.getenv("GUNICORN_CONF", "/app/inboard/gunicorn_conf.py")
+    worker_class = os.getenv("WORKER_CLASS", "uvicorn.workers.UvicornWorker")
+    if not Path(gunicorn_conf_path).is_file():
+        raise FileNotFoundError(f"Unable to find {gunicorn_conf_path}")
+    return ["gunicorn", "-k", worker_class, "-c", gunicorn_conf_path]
+
+
+def set_uvicorn_options(log_config: Optional[dict] = None) -> dict:
+    """Set options for running the Uvicorn server."""
+    with_reload = (
+        True
+        if (value := os.getenv("WITH_RELOAD")) and value.lower() == "true"
+        else False
+    )
+    reload_dirs = (
+        [d.lstrip() for d in str(os.getenv("RELOAD_DIRS")).split(sep=",")]
+        if os.getenv("RELOAD_DIRS")
+        else None
+    )
+    return dict(
+        host=os.getenv("HOST", "0.0.0.0"),
+        port=int(os.getenv("PORT", "80")),
+        log_config=log_config,
+        log_level=os.getenv("LOG_LEVEL", "info"),
+        reload=with_reload,
+        reload_dirs=reload_dirs,
+    )
+
+
 def start_server(
     process_manager: str,
-    app_module: str = str(os.getenv("APP_MODULE", "inboard.app.main_base:app")),
-    logger: Logger = logging.getLogger(),
+    app_module: str,
+    logger: logging.Logger = logging.getLogger(),
     logging_conf_dict: Optional[dict] = None,
-    worker_class: str = str(os.getenv("WORKER_CLASS", "uvicorn.workers.UvicornWorker")),
 ) -> None:
     """Start the Uvicorn or Gunicorn server."""
     try:
         if process_manager == "gunicorn":
             logger.debug("Running Uvicorn with Gunicorn.")
-            gunicorn_conf_path = set_conf_path("gunicorn")
-            subprocess.run(
-                ["gunicorn", "-k", worker_class, "-c", gunicorn_conf_path, app_module]
-            )
+            gunicorn_options: list = set_gunicorn_options()
+            gunicorn_options.append(app_module)
+            subprocess.run(gunicorn_options)
         elif process_manager == "uvicorn":
-            with_reload = (
-                True
-                if (value := os.getenv("WITH_RELOAD")) and value.lower() == "true"
-                else False
-            )
-            reload_dirs = (
-                [d.lstrip() for d in str(os.getenv("RELOAD_DIRS")).split(sep=",")]
-                if os.getenv("RELOAD_DIRS")
-                else None
-            )
             logger.debug("Running Uvicorn without Gunicorn.")
-            uvicorn.run(
-                app_module,
-                host=os.getenv("HOST", "0.0.0.0"),
-                port=int(os.getenv("PORT", "80")),
-                log_config=logging_conf_dict,
-                log_level=os.getenv("LOG_LEVEL", "info"),
-                reload=with_reload,
-                reload_dirs=reload_dirs,
-            )
+            uvicorn_options: dict = set_uvicorn_options(log_config=logging_conf_dict)
+            uvicorn.run(app_module, **uvicorn_options)
         else:
             raise NameError("Process manager needs to be either uvicorn or gunicorn")
     except Exception as e:

tests/test_start.py

@@ -9,37 +9,6 @@
 from inboard import gunicorn_conf, start
 
 
-class TestConfPaths:
-    """Test paths to configuration files.
-    ---
-    """
-
-    def test_set_default_conf_path_gunicorn(self, gunicorn_conf_path: Path) -> None:
-        """Test default Gunicorn configuration file path (different without Docker)."""
-        assert "inboard/gunicorn_conf.py" in str(gunicorn_conf_path)
-        assert "logging" not in str(gunicorn_conf_path)
-        assert start.set_conf_path("gunicorn") == str(gunicorn_conf_path)
-
-    def test_set_custom_conf_path_gunicorn(
-        self,
-        gunicorn_conf_tmp_file_path: Path,
-        monkeypatch: pytest.MonkeyPatch,
-        tmp_path: Path,
-    ) -> None:
-        """Set path to custom temporary Gunicorn configuration file."""
-        monkeypatch.setenv("GUNICORN_CONF", str(gunicorn_conf_tmp_file_path))
-        assert os.getenv("GUNICORN_CONF") == str(gunicorn_conf_tmp_file_path)
-        assert "/gunicorn_conf.py" in str(gunicorn_conf_tmp_file_path)
-        assert "logging" not in str(gunicorn_conf_tmp_file_path)
-        assert start.set_conf_path("gunicorn") == str(gunicorn_conf_tmp_file_path)
-
-    def test_set_incorrect_conf_path(self, monkeypatch: pytest.MonkeyPatch) -> None:
-        """Set path to non-existent file and raise an error."""
-        with pytest.raises(FileNotFoundError):
-            monkeypatch.setenv("GUNICORN_CONF", "/no/file/here")
-            start.set_conf_path("gunicorn")
-
-
 class TestConfigureGunicorn:
     """Test Gunicorn configuration independently of Gunicorn server.
     ---
@@ -255,6 +224,66 @@ def test_configure_logging_tmp_module_no_dict(
         )
 
 
+class TestRunPreStartScript:
+    """Run pre-start scripts using the method in `start.py`.
+    ---
+    """
+
+    def test_run_pre_start_script_py(
+        self,
+        mocker: MockerFixture,
+        monkeypatch: pytest.MonkeyPatch,
+        pre_start_script_tmp_py: Path,
+    ) -> None:
+        """Test `start.run_pre_start_script` using temporary Python pre-start script."""
+        mock_logger = mocker.patch.object(start.logging, "root", autospec=True)
+        monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_py))
+        pre_start_path = os.getenv("PRE_START_PATH")
+        start.run_pre_start_script(logger=mock_logger)
+        mock_logger.debug.assert_has_calls(
+            calls=[
+                mocker.call("Checking for pre-start script."),
+                mocker.call(f"Running pre-start script with python {pre_start_path}."),
+                mocker.call(f"Ran pre-start script with python {pre_start_path}."),
+            ]
+        )
+
+    def test_run_pre_start_script_sh(
+        self,
+        mocker: MockerFixture,
+        monkeypatch: pytest.MonkeyPatch,
+        pre_start_script_tmp_sh: Path,
+    ) -> None:
+        """Test `start.run_pre_start_script` using temporary pre-start shell script."""
+        mock_logger = mocker.patch.object(start.logging, "root", autospec=True)
+        monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_sh))
+        pre_start_path = os.getenv("PRE_START_PATH")
+        start.run_pre_start_script(logger=mock_logger)
+        mock_logger.debug.assert_has_calls(
+            calls=[
+                mocker.call("Checking for pre-start script."),
+                mocker.call(f"Running pre-start script with sh {pre_start_path}."),
+                mocker.call(f"Ran pre-start script with sh {pre_start_path}."),
+            ]
+        )
+
+    def test_run_pre_start_script_no_file(
+        self,
+        mocker: MockerFixture,
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> None:
+        """Test `start.run_pre_start_script` with an incorrect file path."""
+        mock_logger = mocker.patch.object(start.logging, "root", autospec=True)
+        monkeypatch.setenv("PRE_START_PATH", "/no/file/here")
+        start.run_pre_start_script(logger=mock_logger)
+        mock_logger.debug.assert_has_calls(
+            calls=[
+                mocker.call("Checking for pre-start script."),
+                mocker.call("No pre-start script found."),
+            ]
+        )
+
+
 class TestSetAppModule:
     """Set app module string using the method in `start.py`.
     ---
@@ -355,63 +384,92 @@ def test_set_app_module_incorrect(
         )
 
 
-class TestRunPreStartScript:
-    """Run pre-start scripts using the method in `start.py`.
+class TestSetGunicornOptions:
+    """Test Gunicorn configuration options method.
     ---
     """
 
-    def test_run_pre_start_script_py(
-        self,
-        mocker: MockerFixture,
-        monkeypatch: pytest.MonkeyPatch,
-        pre_start_script_tmp_py: Path,
+    def test_set_gunicorn_options_default(
+        self, gunicorn_conf_path: Path, monkeypatch: pytest.MonkeyPatch
     ) -> None:
-        """Test `start.run_pre_start_script` using temporary Python pre-start script."""
-        mock_logger = mocker.patch.object(start.logging, "root", autospec=True)
-        monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_py))
-        pre_start_path = os.getenv("PRE_START_PATH")
-        start.run_pre_start_script(logger=mock_logger)
-        mock_logger.debug.assert_has_calls(
-            calls=[
-                mocker.call("Checking for pre-start script."),
-                mocker.call(f"Running pre-start script with python {pre_start_path}."),
-                mocker.call(f"Ran pre-start script with python {pre_start_path}."),
-            ]
-        )
-
-    def test_run_pre_start_script_sh(
+        """Test default Gunicorn server options."""
+        monkeypatch.setenv("GUNICORN_CONF", str(gunicorn_conf_path))
+        result = start.set_gunicorn_options()
+        assert os.getenv("GUNICORN_CONF") == str(gunicorn_conf_path)
+        assert "/gunicorn_conf.py" in str(gunicorn_conf_path)
+        assert "logging" not in str(gunicorn_conf_path)
+        assert isinstance(result, list)
+        assert result == [
+            "gunicorn",
+            "-k",
+            "uvicorn.workers.UvicornWorker",
+            "-c",
+            str(gunicorn_conf_path),
+        ]
+
+    def test_set_gunicorn_options_custom(
         self,
-        mocker: MockerFixture,
+        gunicorn_conf_tmp_file_path: Path,
         monkeypatch: pytest.MonkeyPatch,
-        pre_start_script_tmp_sh: Path,
+        tmp_path: Path,
     ) -> None:
-        """Test `start.run_pre_start_script` using temporary pre-start shell script."""
-        mock_logger = mocker.patch.object(start.logging, "root", autospec=True)
-        monkeypatch.setenv("PRE_START_PATH", str(pre_start_script_tmp_sh))
-        pre_start_path = os.getenv("PRE_START_PATH")
-        start.run_pre_start_script(logger=mock_logger)
-        mock_logger.debug.assert_has_calls(
-            calls=[
-                mocker.call("Checking for pre-start script."),
-                mocker.call(f"Running pre-start script with sh {pre_start_path}."),
-                mocker.call(f"Ran pre-start script with sh {pre_start_path}."),
-            ]
+        """Test custom Gunicorn server options with temporary configuration file."""
+        monkeypatch.setenv("GUNICORN_CONF", str(gunicorn_conf_tmp_file_path))
+        result = start.set_gunicorn_options()
+        assert os.getenv("GUNICORN_CONF") == str(gunicorn_conf_tmp_file_path)
+        assert "/gunicorn_conf.py" in str(gunicorn_conf_tmp_file_path)
+        assert "logging" not in str(gunicorn_conf_tmp_file_path)
+        assert isinstance(result, list)
+        assert result == [
+            "gunicorn",
+            "-k",
+            "uvicorn.workers.UvicornWorker",
+            "-c",
+            str(gunicorn_conf_tmp_file_path),
+        ]
+
+    def test_set_incorrect_conf_path(self, monkeypatch: pytest.MonkeyPatch) -> None:
+        """Set path to non-existent file and raise an error."""
+        monkeypatch.setenv("GUNICORN_CONF", "/no/file/here")
+        with pytest.raises(FileNotFoundError):
+            start.set_gunicorn_options()
+
+
+class TestSetUvicornOptions:
+    """Test Uvicorn configuration options method.
+    ---
+    """
+
+    def test_set_uvicorn_options_default(self, monkeypatch: pytest.MonkeyPatch) -> None:
+        """Test default Uvicorn server options."""
+        monkeypatch.setenv("WITH_RELOAD", "false")
+        result = start.set_uvicorn_options()
+        assert isinstance(result, dict)
+        assert result == dict(
+            host="0.0.0.0",
+            port=80,
+            log_config=None,
+            log_level="info",
+            reload=False,
+            reload_dirs=None,
         )
 
-    def test_run_pre_start_script_no_file(
-        self,
-        mocker: MockerFixture,
-        monkeypatch: pytest.MonkeyPatch,
+    def test_set_uvicorn_options_custom(
+        self, logging_conf_dict: dict, monkeypatch: pytest.MonkeyPatch
     ) -> None:
-        """Test `start.run_pre_start_script` with an incorrect file path."""
-        mock_logger = mocker.patch.object(start.logging, "root", autospec=True)
-        monkeypatch.setenv("PRE_START_PATH", "/no/file/here")
-        start.run_pre_start_script(logger=mock_logger)
-        mock_logger.debug.assert_has_calls(
-            calls=[
-                mocker.call("Checking for pre-start script."),
-                mocker.call("No pre-start script found."),
-            ]
+        """Test custom Uvicorn server options."""
+        monkeypatch.setenv("LOG_LEVEL", "debug")
+        monkeypatch.setenv("WITH_RELOAD", "true")
+        monkeypatch.setenv("RELOAD_DIRS", "inboard, tests")
+        result = start.set_uvicorn_options(log_config=logging_conf_dict)
+        assert isinstance(result, dict)
+        assert result == dict(
+            host="0.0.0.0",
+            port=80,
+            log_config=logging_conf_dict,
+            log_level="debug",
+            reload=True,
+            reload_dirs=["inboard", "tests"],
         )