Merge branch 'main' into 742-environment-reload

.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

dev-requirements.txt

@@ -46,7 +46,7 @@ deepdiff==8.0.1
 deepmerge==2.0
 Deprecated==1.2.15
 distlib==0.3.9
-dls-dodal==1.36.2
+dls-dodal==1.37.0
 dnspython==2.7.0
 docopt==0.6.2
 doct==1.1.0

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
@@ -75,13 +76,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/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

src/blueapi/worker/task.py

@@ -22,7 +22,8 @@ class Task(BlueapiBaseModel):
 
     def prepare_params(self, ctx: BlueskyContext) -> Mapping[str, Any]:
         model = _lookup_params(ctx, self)
-        return _model_to_kwargs(model)
+        # Re-create dict manually to avoid nesting in model_dump output
+        return {field: getattr(model, field) for field in model.__pydantic_fields__}
 
     def do_task(self, ctx: BlueskyContext) -> None:
         LOGGER.info(f"Asked to run plan {self.name} with {self.params}")
@@ -49,22 +50,3 @@ def _lookup_params(ctx: BlueskyContext, task: Task) -> BaseModel:
     model = plan.model
     adapter = TypeAdapter(model)
     return adapter.validate_python(task.params)
-
-
-def _model_to_kwargs(model: BaseModel) -> Mapping[str, Any]:
-    """
-    Converts an instance of BaseModel back to a dictionary that
-    can be passed as **kwargs.
-    Used instead of BaseModel.model_dump() because we don't want
-    the dumping to be nested and because it fires UserWarnings
-    about data types it is unfamiliar with
-    (such as ophyd devices).
-
-    Args:
-        model: Pydantic model to convert to kwargs
-
-    Returns:
-        Mapping[str, Any]: Dictionary that can be passed as **kwargs
-    """
-
-    return {name: getattr(model, name) for name in model.model_fields_set}