Merge branch 'main' into tiled-insertion

.github/workflows/_container.yml

@@ -106,7 +106,6 @@ jobs:
 
       - name: package chart and push it
         run: |
-          sed -i "$ a appVersion: ${GITHUB_REF##*/}" helm/blueapi/Chart.yaml
           helm dependencies update helm/blueapi
-          helm package helm/blueapi --version ${GITHUB_REF##*/} -d /tmp/
+          helm package helm/blueapi --version ${GITHUB_REF##*/} --app-version ${GITHUB_REF##*/} -d /tmp/
           helm push /tmp/blueapi-${GITHUB_REF##*/}.tgz oci://ghcr.io/diamondlightsource/charts

.github/workflows/_system_test.yml

@@ -0,0 +1,32 @@
+on:
+  workflow_call:
+
+env:
+  # https://github.com/pytest-dev/pytest/issues/2042
+  PY_IGNORE_IMPORTMISMATCH: "1"
+
+jobs:
+  run:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          # Need this to get version number from last tag
+          fetch-depth: 0
+
+      - name: Install python packages
+        uses: ./.github/actions/install_requirements
+
+      - name: Start RabbitMQ
+        uses: namoshek/rabbitmq-github-action@v1
+        with:
+          ports: "61613:61613"
+          plugins: rabbitmq_stomp
+
+      - name: Start Blueapi Server
+        run: blueapi -c ${{ github.workspace }}/tests/unit_tests/example_yaml/valid_stomp_config.yaml serve &
+
+      - name: Run tests
+        run: tox -e system-test

.github/workflows/_test.yml

@@ -16,9 +16,6 @@ on:
 env:
   # https://github.com/pytest-dev/pytest/issues/2042
   PY_IGNORE_IMPORTMISMATCH: "1"
-  BLUEAPI_TEST_STOMP_PORTS: "[61613,61614]"
-
-
 
 jobs:
   run:

.github/workflows/ci.yml

@@ -34,6 +34,11 @@ jobs:
     secrets:
       CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
 
+  system-test:
+    needs: check
+    if: needs.check.outputs.branch-pr == ''
+    uses: ./.github/workflows/_system_test.yml
+
   container:
     needs: check
     if: needs.check.outputs.branch-pr == ''

docs/how-to/edit-live.md

@@ -11,6 +11,8 @@ Blueapi can be configured to install editable Python packages from a chosen dire
 
 scratch:
     root: /path/to/my/scratch/directory
+    # Required GID for the scratch area
+    required_gid: 12345
     repositories:
         # Repository for DLS devices
         - name: dodal
@@ -21,6 +23,9 @@ scratch:
           remote_url: https://github.com/DiamondLightSource/mx-bluesky.git
 ```
 
+Note the `required_gid` field, which is useful for stopping blueapi from locking the files it clones
+to a particular owner.
+
 ## Synchronization
 
 Blueapi will synchronize reality with the configuration if you run

src/blueapi/cli/cli.py

@@ -1,5 +1,7 @@
 import json
 import logging
+import os
+import stat
 import sys
 from functools import wraps
 from pathlib import Path
@@ -39,6 +41,9 @@
 def main(ctx: click.Context, config: Path | None | tuple[Path, ...]) -> None:
     # if no command is supplied, run with the options passed
 
+    # Set umask to DLS standard
+    os.umask(stat.S_IWOTH)
+
     config_loader = ConfigLoader(ApplicationConfig)
     if config is not None:
         configs = (config,) if isinstance(config, Path) else config

src/blueapi/cli/scratch.py

@@ -1,12 +1,14 @@
 import logging
 import os
 import stat
+import textwrap
 from pathlib import Path
 from subprocess import Popen
 
 from git import Repo
 
 from blueapi.config import ScratchConfig
+from blueapi.utils import get_owner_gid, is_sgid_set
 
 _DEFAULT_INSTALL_TIMEOUT: float = 300.0
 
@@ -23,7 +25,7 @@ def setup_scratch(
         install_timeout: Timeout for installing packages
     """
 
-    _validate_directory(config.root)
+    _validate_root_directory(config.root, config.required_gid)
 
     logging.info(f"Setting up scratch area: {config.root}")
 
@@ -74,9 +76,6 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No
 
     _validate_directory(path)
 
-    # Set umask to DLS standard
-    os.umask(stat.S_IWOTH)
-
     logging.info(f"Installing {path}")
     process = Popen(
         [
@@ -94,6 +93,37 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No
         raise RuntimeError(f"Failed to install {path}: Exit Code: {process.returncode}")
 
 
+def _validate_root_directory(root_path: Path, required_gid: int | None) -> None:
+    _validate_directory(root_path)
+
+    if not is_sgid_set(root_path):
+        raise PermissionError(
+            textwrap.dedent(f"""
+        The scratch area root directory ({root_path}) needs to have the
+        SGID permission bit set. This allows blueapi to clone
+        repositories into it while retaining the ability for
+        other users in an approved group to edit/delete them.
+
+        See https://www.redhat.com/en/blog/suid-sgid-sticky-bit for how to
+        set the SGID bit.
+        """)
+        )
+    elif required_gid is not None and get_owner_gid(root_path) != required_gid:
+        raise PermissionError(
+            textwrap.dedent(f"""
+        The configuration requires that {root_path} be owned by the group with
+        ID {required_gid}.
+        You may be able to find this group's name by running the following
+        in the terminal.
+
+        getent group 1000 | cut -d: -f1
+
+        You can transfer ownership, if you have sufficient permissions, with the chgrp
+        command.
+        """)
+        )
+
+
 def _validate_directory(path: Path) -> None:
     if not path.exists():
         raise KeyError(f"{path}: No such file or directory")

src/blueapi/config.py

@@ -1,3 +1,4 @@
+import textwrap
 from collections.abc import Mapping
 from enum import Enum
 from functools import cached_property
@@ -84,13 +85,34 @@ class RestConfig(BlueapiBaseModel):
 
 
 class ScratchRepository(BlueapiBaseModel):
-    name: str = "example"
-    remote_url: str = "https://github.com/example/example.git"
+    name: str = Field(
+        description="Unique name for this repository in the scratch directory",
+        default="example",
+    )
+    remote_url: str = Field(
+        description="URL to clone from",
+        default="https://github.com/example/example.git",
+    )
 
 
 class ScratchConfig(BlueapiBaseModel):
-    root: Path = Path("/tmp/scratch/blueapi")
-    repositories: list[ScratchRepository] = Field(default_factory=list)
+    root: Path = Field(
+        description="The root directory of the scratch area, all repositories will "
+        "be cloned under this directory.",
+        default=Path("/tmp/scratch/blueapi"),
+    )
+    required_gid: int | None = Field(
+        description=textwrap.dedent("""
+    Required owner GID for the scratch directory. If supplied the setup-scratch
+    command will check the scratch area ownership and raise an error if it is
+    not owned by <GID>.
+    """),
+        default=None,
+    )
+    repositories: list[ScratchRepository] = Field(
+        description="Details of repositories to be cloned and imported into blueapi",
+        default_factory=list,
+    )
 
 
 class OIDCConfig(BlueapiBaseModel):

src/blueapi/service/main.py

@@ -59,9 +59,12 @@ def _runner() -> WorkerDispatcher:
     return RUNNER
 
 
-def setup_runner(config: ApplicationConfig | None = None, use_subprocess: bool = True):
+def setup_runner(
+    config: ApplicationConfig | None = None,
+    runner: WorkerDispatcher | None = None,
+):
     global RUNNER
-    runner = WorkerDispatcher(config, use_subprocess)
+    runner = runner or WorkerDispatcher(config)
     runner.start()
 
     RUNNER = runner

src/blueapi/service/runner.py

@@ -8,7 +8,6 @@
 from typing import Any, ParamSpec, TypeVar
 
 from observability_utils.tracing import (
-    add_span_attributes,
     get_context_propagator,
     get_tracer,
     start_as_current_span,
@@ -44,17 +43,19 @@ class WorkerDispatcher:
 
     _config: ApplicationConfig
     _subprocess: PoolClass | None
-    _use_subprocess: bool
     _state: EnvironmentResponse
 
     def __init__(
         self,
         config: ApplicationConfig | None = None,
-        use_subprocess: bool = True,
+        subprocess_factory: Callable[[], PoolClass] | None = None,
     ) -> None:
+        def default_subprocess_factory():
+            return Pool(initializer=_init_worker, processes=1)
+
         self._config = config or ApplicationConfig()
         self._subprocess = None
-        self._use_subprocess = use_subprocess
+        self._subprocess_factory = subprocess_factory or default_subprocess_factory
         self._state = EnvironmentResponse(
             initialized=False,
         )
@@ -68,12 +69,8 @@ def reload(self):
 
     @start_as_current_span(TRACER)
     def start(self):
-        add_span_attributes(
-            {"_use_subprocess": self._use_subprocess, "_config": str(self._config)}
-        )
         try:
-            if self._use_subprocess:
-                self._subprocess = Pool(initializer=_init_worker, processes=1)
+            self._subprocess = self._subprocess_factory()
             self.run(setup, self._config)
             self._state = EnvironmentResponse(initialized=True)
         except Exception as e:
@@ -107,40 +104,25 @@ def run(
         function: Callable[P, T],
         *args: P.args,
         **kwargs: P.kwargs,
-    ) -> T:
-        """Calls the supplied function, which is modified to accept a dict as it's new
-        first param, before being passed to the subprocess runner, or just run in place.
-        """
-        add_span_attributes({"use_subprocess": self._use_subprocess})
-        if self._use_subprocess:
-            return self._run_in_subprocess(function, *args, **kwargs)
-        else:
-            return function(*args, **kwargs)
-
-    @start_as_current_span(TRACER, "function", "args", "kwargs")
-    def _run_in_subprocess(
-        self,
-        function: Callable[P, T],
-        *args: P.args,
-        **kwargs: P.kwargs,
     ) -> T:
         """Call the supplied function, passing the current Span ID, if one
-        exists,from the observability context inro the _rpc caller function.
+        exists,from the observability context into the import_and_run_function
+        caller function.
+
         When this is deserialized in and run by the subprocess, this will allow
         its functions to use the corresponding span as their parent span."""
+
         if self._subprocess is None:
             raise InvalidRunnerStateError("Subprocess runner has not been started")
         if not (hasattr(function, "__name__") and hasattr(function, "__module__")):
             raise RpcError(f"{function} is anonymous, cannot be run in subprocess")
-        if not callable(function):
-            raise RpcError(f"{function} is not Callable, cannot be run in subprocess")
         try:
             return_type = inspect.signature(function).return_annotation
         except TypeError:
             return_type = None
 
         return self._subprocess.apply(
-            _rpc,
+            import_and_run_function,
             (
                 function.__module__,
                 function.__name__,
@@ -164,7 +146,7 @@ def __init__(self, message):
 class RpcError(Exception): ...
 
 
-def _rpc(
+def import_and_run_function(
     module_name: str,
     function_name: str,
     expected_type: type[T] | None,

src/blueapi/utils/__init__.py

@@ -1,5 +1,6 @@
 from .base_model import BlueapiBaseModel, BlueapiModelConfig, BlueapiPlanModelConfig
 from .connect_devices import connect_devices
+from .file_permissions import get_owner_gid, is_sgid_set
 from .invalid_config_error import InvalidConfigError
 from .modules import load_module_all
 from .serialization import serialize
@@ -14,4 +15,6 @@
     "BlueapiPlanModelConfig",
     "InvalidConfigError",
     "connect_devices",
+    "is_sgid_set",
+    "get_owner_gid",
 ]

src/blueapi/utils/file_permissions.py

@@ -0,0 +1,32 @@
+import stat
+from pathlib import Path
+
+
+def is_sgid_set(path: Path) -> bool:
+    """Check if the SGID bit is set so that new files created
+    under a directory owned by a group are owned by that same group.
+
+    See https://www.redhat.com/en/blog/suid-sgid-sticky-bit
+
+    Args:
+        path: Path to the file to check
+
+    Returns:
+        bool: True if the SGID bit is set
+    """
+
+    mask = path.stat().st_mode
+    return bool(mask & stat.S_ISGID)
+
+
+def get_owner_gid(path: Path) -> int:
+    """Get the GID of the owner of a file
+
+    Args:
+        path: Path to the file to check
+
+    Returns:
+        bool: The GID of the file owner
+    """
+
+    return path.stat().st_gid