From d5c7a0359fa9a1fca14193481b53ef43e32a321b Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 9 Apr 2026 13:29:14 +0100 Subject: [PATCH 1/6] Add tag handling to scratch checkout --- src/blueapi/cli/scratch.py | 44 ++++++++++++++++++++++++-------------- src/blueapi/config.py | 23 +++++++++++++++----- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index 85ad9310ce..88230c32c1 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -48,12 +48,15 @@ def setup_scratch( ) for repo in config.repositories: local_directory = config.root / repo.name - ensure_repo(repo.remote_url, local_directory, repo.branch) + ensure_repo(repo.remote_url, local_directory, repo.branch, repo.tag) scratch_install(local_directory, timeout=install_timeout) def ensure_repo( - remote_url: str, local_directory: Path, branch: str | None = None + remote_url: str, + local_directory: Path, + branch: str | None = None, + tag: str | None = None, ) -> None: """ Ensure that a repository is checked out for use in the scratch area. @@ -64,6 +67,9 @@ def ensure_repo( local_directory: Output path for cloning """ + if tag and branch: + raise ValueError(f"Can't use both branch ({branch!r}) and tag ({tag!r})") + # Set umask to DLS standard os.umask(stat.S_IWOTH) @@ -71,27 +77,33 @@ def ensure_repo( LOGGER.info(f"Cloning {remote_url}") repo = Repo.clone_from(remote_url, local_directory) LOGGER.info(f"Cloned {remote_url} -> {local_directory}") + if branch: + if not (local := getattr(repo.heads, branch, None)): + origin = repo.remotes[0] + origin.fetch() + LOGGER.info( + "Creating branch '%s' to track remote '%s'", + branch, + origin.refs[branch], + ) + local = repo.create_head(branch, origin.refs[branch]) + local.set_tracking_branch(origin.refs[branch]) + + LOGGER.info("Checking out branch %r", branch) + local.checkout() + elif tag: + if tag_obj := getattr(repo.tags, tag, None): + LOGGER.info("Checking out tag %r", tag) + tag_obj.checkout() + else: + raise ValueError("Could not find tag: " + repr(tag)) elif local_directory.is_dir(): - repo = Repo(local_directory) LOGGER.info(f"Found {local_directory}") else: raise KeyError( f"Unable to open {local_directory} as a git repository because it is a file" ) - if branch: - if not (local := getattr(repo.heads, branch, None)): - origin = repo.remotes[0] - origin.fetch() - LOGGER.info( - "Creating branch '%s' to track remote '%s'", branch, origin.refs[branch] - ) - local = repo.create_head(branch, origin.refs[branch]) - local.set_tracking_branch(origin.refs[branch]) - - LOGGER.info("Checking out branch '%s'", branch) - local.checkout() - def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> None: """ diff --git a/src/blueapi/config.py b/src/blueapi/config.py index b89b94fd61..ea58e804a8 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -6,7 +6,7 @@ from functools import cached_property from pathlib import Path from string import Template -from typing import Annotated, Any, ClassVar, Generic, Literal, TypeVar, cast +from typing import Annotated, Any, ClassVar, Generic, Literal, Self, TypeVar, cast import requests import yaml @@ -21,6 +21,7 @@ UrlConstraints, ValidationError, field_validator, + model_validator, ) from pydantic.json_schema import SkipJsonSchema @@ -192,10 +193,14 @@ class ScratchRepository(BlueapiBaseModel): default="https://github.com/example/example.git", ) branch: str | SkipJsonSchema[None] = Field( - description=( - "Branch of repo to check out - defaults to remote's default when " - "cloning and the existing branch when the repo already exists" - ), + description="Branch of repo to check out - defaults to remote's default", + exclude_if=lambda f: f is None, + # using default_factory instead of default means the schema doesn't + # include an invalid value + default_factory=lambda: None, + ) + tag: str | SkipJsonSchema[None] = Field( + description="Tag of repo to check out.", exclude_if=lambda f: f is None, # using default_factory instead of default means the schema doesn't # include an invalid value @@ -209,6 +214,14 @@ def check_remote_url(cls, value: str) -> str: raise ValueError(f"remote_url '{value}' is not allowed.") return value + @model_validator(mode="after") + def check_clone_options(self) -> Self: + if self.branch is not None and self.tag is not None: + raise ValueError( + f"Cannot set both branch ({self.branch}) and tag ({self.tag})" + ) + return self + class ScratchConfig(BlueapiBaseModel): root: Path = Field( From 593273e5580af31f4311775ae43dfcdd4ecdce25 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 10 Apr 2026 14:34:55 +0100 Subject: [PATCH 2/6] Use gitpython branch handling during clone --- helm/blueapi/config_schema.json | 2 +- helm/blueapi/values.schema.json | 2 +- src/blueapi/cli/scratch.py | 36 +++++----------------------- src/blueapi/config.py | 23 ++++-------------- tests/unit_tests/cli/test_scratch.py | 27 ++++++++------------- 5 files changed, 23 insertions(+), 67 deletions(-) diff --git a/helm/blueapi/config_schema.json b/helm/blueapi/config_schema.json index 1986d6f8df..f36d75a075 100644 --- a/helm/blueapi/config_schema.json +++ b/helm/blueapi/config_schema.json @@ -431,7 +431,7 @@ "type": "string" }, "branch": { - "description": "Branch of repo to check out - defaults to remote's default when cloning and the existing branch when the repo already exists", + "description": "Branch (or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.", "title": "Branch", "type": "string" } diff --git a/helm/blueapi/values.schema.json b/helm/blueapi/values.schema.json index 3159f6713e..c97db904bd 100644 --- a/helm/blueapi/values.schema.json +++ b/helm/blueapi/values.schema.json @@ -854,7 +854,7 @@ "properties": { "branch": { "title": "Branch", - "description": "Branch of repo to check out - defaults to remote's default when cloning and the existing branch when the repo already exists", + "description": "Branch (or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.", "type": "string" }, "name": { diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index 88230c32c1..9a05de01bc 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -48,15 +48,12 @@ def setup_scratch( ) for repo in config.repositories: local_directory = config.root / repo.name - ensure_repo(repo.remote_url, local_directory, repo.branch, repo.tag) + ensure_repo(repo.remote_url, local_directory, repo.branch) scratch_install(local_directory, timeout=install_timeout) def ensure_repo( - remote_url: str, - local_directory: Path, - branch: str | None = None, - tag: str | None = None, + remote_url: str, local_directory: Path, branch: str | None = None ) -> None: """ Ensure that a repository is checked out for use in the scratch area. @@ -67,41 +64,20 @@ def ensure_repo( local_directory: Output path for cloning """ - if tag and branch: - raise ValueError(f"Can't use both branch ({branch!r}) and tag ({tag!r})") - # Set umask to DLS standard os.umask(stat.S_IWOTH) if not local_directory.exists(): LOGGER.info(f"Cloning {remote_url}") - repo = Repo.clone_from(remote_url, local_directory) + Repo.clone_from(remote_url, local_directory, branch=branch) LOGGER.info(f"Cloned {remote_url} -> {local_directory}") - if branch: - if not (local := getattr(repo.heads, branch, None)): - origin = repo.remotes[0] - origin.fetch() - LOGGER.info( - "Creating branch '%s' to track remote '%s'", - branch, - origin.refs[branch], - ) - local = repo.create_head(branch, origin.refs[branch]) - local.set_tracking_branch(origin.refs[branch]) - - LOGGER.info("Checking out branch %r", branch) - local.checkout() - elif tag: - if tag_obj := getattr(repo.tags, tag, None): - LOGGER.info("Checking out tag %r", tag) - tag_obj.checkout() - else: - raise ValueError("Could not find tag: " + repr(tag)) elif local_directory.is_dir(): + Repo(local_directory) LOGGER.info(f"Found {local_directory}") else: raise KeyError( - f"Unable to open {local_directory} as a git repository because it is a file" + f"Unable to open {local_directory} as a git repository because it is not a " + "directory" ) diff --git a/src/blueapi/config.py b/src/blueapi/config.py index ea58e804a8..ad1852ec95 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -6,7 +6,7 @@ from functools import cached_property from pathlib import Path from string import Template -from typing import Annotated, Any, ClassVar, Generic, Literal, Self, TypeVar, cast +from typing import Annotated, Any, ClassVar, Generic, Literal, TypeVar, cast import requests import yaml @@ -21,7 +21,6 @@ UrlConstraints, ValidationError, field_validator, - model_validator, ) from pydantic.json_schema import SkipJsonSchema @@ -193,14 +192,10 @@ class ScratchRepository(BlueapiBaseModel): default="https://github.com/example/example.git", ) branch: str | SkipJsonSchema[None] = Field( - description="Branch of repo to check out - defaults to remote's default", - exclude_if=lambda f: f is None, - # using default_factory instead of default means the schema doesn't - # include an invalid value - default_factory=lambda: None, - ) - tag: str | SkipJsonSchema[None] = Field( - description="Tag of repo to check out.", + description=( + "Branch (or tag) to check out when cloning - defaults to remote's HEAD. " + "If a tag is used, the repo will be left in a 'detached head' state." + ), exclude_if=lambda f: f is None, # using default_factory instead of default means the schema doesn't # include an invalid value @@ -214,14 +209,6 @@ def check_remote_url(cls, value: str) -> str: raise ValueError(f"remote_url '{value}' is not allowed.") return value - @model_validator(mode="after") - def check_clone_options(self) -> Self: - if self.branch is not None and self.tag is not None: - raise ValueError( - f"Cannot set both branch ({self.branch}) and tag ({self.tag})" - ) - return self - class ScratchConfig(BlueapiBaseModel): root: Path = Field( diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index 1c78401430..47b9061864 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -123,7 +123,7 @@ def test_repo_cloned_if_not_found_locally( ensure_repo("http://example.com/foo.git", nonexistant_path) mock_repo.assert_not_called() mock_repo.clone_from.assert_called_once_with( - "http://example.com/foo.git", nonexistant_path + "http://example.com/foo.git", nonexistant_path, branch=None ) @@ -140,7 +140,7 @@ def write_repo_files(): with file_path.open("w") as stream: stream.write("foo") - mock_repo.clone_from.side_effect = lambda url, path: write_repo_files() + mock_repo.clone_from.side_effect = lambda url, path, branch=None: write_repo_files() ensure_repo("http://example.com/foo.git", repo_root) assert file_path.exists() @@ -162,26 +162,23 @@ def test_cloned_repo_changes_to_new_branch(mock_repo, directory_path: Path): ensure_repo("http://example.com/foo.git", directory_path / "demo_branch", "demo") - mock_repo.clone_from.assert_called_once_with("http://example.com/foo.git", ANY) - repo.create_head.assert_called_once_with("demo", ANY) - repo.create_head().checkout.assert_called_once() + mock_repo.clone_from.assert_called_once_with( + "http://example.com/foo.git", ANY, branch="demo" + ) @patch("blueapi.cli.scratch.Repo") -def test_existing_repo_changes_to_existing_branch(mock_repo, directory_path: Path): +def test_existing_repo_not_changed_to_existing_branch(mock_repo, directory_path: Path): (directory_path / "demo_branch").mkdir() - repo = Mock(spec=Repo) - mock_repo.return_value = repo ensure_repo("http://example.com/foo.git", directory_path / "demo_branch", "demo") + mock_repo.assert_called_once_with(directory_path / "demo_branch") mock_repo.clone_from.assert_not_called() - repo.create_head.assert_not_called() - repo.heads.demo.checkout.assert_called_once() @patch("blueapi.cli.scratch.Repo") -def test_existing_repo_changes_to_new_branch(mock_repo, directory_path: Path): +def test_existing_repo_not_changed_to_new_branch(mock_repo, directory_path: Path): (directory_path / "demo_branch").mkdir() repo = MagicMock(name="ExistingRepo", spec=Repo) repo.heads.demo = None @@ -190,8 +187,7 @@ def test_existing_repo_changes_to_new_branch(mock_repo, directory_path: Path): ensure_repo("http://example.com/foo.git", directory_path / "demo_branch", "demo") mock_repo.clone_from.assert_not_called() - repo.create_head.assert_called_once_with("demo", repo.remotes[0].refs["demo"]) - repo.create_head().checkout.assert_called_once() + repo.create_head.assert_not_called() def test_setup_scratch_fails_on_nonexistant_root( @@ -292,10 +288,7 @@ def test_setup_scratch_iterates_repos( config = ScratchConfig( root=directory_path_with_sgid, repositories=[ - ScratchRepository( - name="foo", - remote_url="http://example.com/foo.git", - ), + ScratchRepository(name="foo", remote_url="http://example.com/foo.git"), ScratchRepository( name="bar", remote_url="http://example.com/bar.git", From f0aeacbd7f795edb337395f7347a84fe18423101 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 15 Apr 2026 13:53:12 +0100 Subject: [PATCH 3/6] branch -> target_revision + warnings --- helm/blueapi/config_schema.json | 6 +++--- helm/blueapi/values.schema.json | 10 +++++----- src/blueapi/cli/scratch.py | 22 +++++++++++++++++----- src/blueapi/config.py | 11 +++++++---- tests/unit_tests/test_helm_chart.py | 2 +- 5 files changed, 33 insertions(+), 18 deletions(-) diff --git a/helm/blueapi/config_schema.json b/helm/blueapi/config_schema.json index f36d75a075..b5d0c9bf3d 100644 --- a/helm/blueapi/config_schema.json +++ b/helm/blueapi/config_schema.json @@ -430,9 +430,9 @@ "title": "Remote Url", "type": "string" }, - "branch": { - "description": "Branch (or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.", - "title": "Branch", + "target_revision": { + "description": "Revision (branch or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.", + "title": "Target Revision", "type": "string" } }, diff --git a/helm/blueapi/values.schema.json b/helm/blueapi/values.schema.json index c97db904bd..639209dd90 100644 --- a/helm/blueapi/values.schema.json +++ b/helm/blueapi/values.schema.json @@ -852,11 +852,6 @@ "title": "ScratchRepository", "type": "object", "properties": { - "branch": { - "title": "Branch", - "description": "Branch (or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.", - "type": "string" - }, "name": { "title": "Name", "description": "Unique name for this repository in the scratch directory", @@ -868,6 +863,11 @@ "description": "URL to clone from", "default": "https://github.com/example/example.git", "type": "string" + }, + "target_revision": { + "title": "Target Revision", + "description": "Revision (branch or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.", + "type": "string" } }, "additionalProperties": false diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index 9a05de01bc..9ed3ef0297 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -48,12 +48,12 @@ def setup_scratch( ) for repo in config.repositories: local_directory = config.root / repo.name - ensure_repo(repo.remote_url, local_directory, repo.branch) + ensure_repo(repo.remote_url, local_directory, repo.target_revision) scratch_install(local_directory, timeout=install_timeout) def ensure_repo( - remote_url: str, local_directory: Path, branch: str | None = None + remote_url: str, local_directory: Path, target_revision: str | None = None ) -> None: """ Ensure that a repository is checked out for use in the scratch area. @@ -69,11 +69,23 @@ def ensure_repo( if not local_directory.exists(): LOGGER.info(f"Cloning {remote_url}") - Repo.clone_from(remote_url, local_directory, branch=branch) + Repo.clone_from(remote_url, local_directory, branch=target_revision) LOGGER.info(f"Cloned {remote_url} -> {local_directory}") elif local_directory.is_dir(): - Repo(local_directory) - LOGGER.info(f"Found {local_directory}") + repo = Repo(local_directory) + head = repo.head.commit + LOGGER.info("Found %s @ %s", local_directory, head.name_rev) + if target_revision: + if target_revision in repo.refs: + if repo.refs[target_revision].commit != head: + LOGGER.warning( + "Repository %s not at target revision: %r instead of %r", + local_directory.name, + head.name_rev, + target_revision, + ) + else: + LOGGER.warning("Target revision %r not found", target_revision) else: raise KeyError( f"Unable to open {local_directory} as a git repository because it is not a " diff --git a/src/blueapi/config.py b/src/blueapi/config.py index ad1852ec95..2adbbfd047 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -12,6 +12,7 @@ import yaml from bluesky_stomp.models import BasicAuthentication from pydantic import ( + AliasChoices, AnyUrl, BaseModel, Field, @@ -191,11 +192,13 @@ class ScratchRepository(BlueapiBaseModel): description="URL to clone from", default="https://github.com/example/example.git", ) - branch: str | SkipJsonSchema[None] = Field( + target_revision: str | SkipJsonSchema[None] = Field( description=( - "Branch (or tag) to check out when cloning - defaults to remote's HEAD. " - "If a tag is used, the repo will be left in a 'detached head' state." + "Revision (branch or tag) to check out when cloning - defaults to " + "remote's HEAD. If a tag is used, the repo will be left in a " + "'detached head' state." ), + validation_alias=AliasChoices("branch", "tag", "target_revision"), exclude_if=lambda f: f is None, # using default_factory instead of default means the schema doesn't # include an invalid value @@ -455,5 +458,5 @@ def generate(self, schema, mode="validation"): return json_schema return ApplicationConfig.model_json_schema( - schema_generator=_GenerateJsonSchema, ref_template="{model}" + by_alias=False, schema_generator=_GenerateJsonSchema, ref_template="{model}" ) diff --git a/tests/unit_tests/test_helm_chart.py b/tests/unit_tests/test_helm_chart.py index 52394c5618..43b8b5769a 100644 --- a/tests/unit_tests/test_helm_chart.py +++ b/tests/unit_tests/test_helm_chart.py @@ -84,7 +84,7 @@ ScratchRepository( name="bar", remote_url="https://example.git", - branch="bar_branch", + target_revision="bar_branch", ), ], ), From a6f76db468b023522a1a068d366bd0918e18a9b7 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 15 Apr 2026 14:47:05 +0100 Subject: [PATCH 4/6] Update tests to check branch/tag aliases --- tests/unit_tests/cli/test_cli.py | 17 ++++++++++++++++- tests/unit_tests/example_yaml/scratch.yaml | 9 +++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index cbde517414..687b3a03fc 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1055,7 +1055,22 @@ def test_init_scratch_calls_setup_scratch(mock_setup_scratch: Mock, runner: CliR ScratchRepository( name="dodal", remote_url="https://github.com/DiamondLightSource/dodal.git", - ) + ), + ScratchRepository( + name="with_target", + remote_url="https://github.com/DiamondLightSource/dodal.git", + target_revision="demo", + ), + ScratchRepository( + name="with_branch", + remote_url="https://github.com/DiamondLightSource/dodal.git", + target_revision="demo_branch", + ), + ScratchRepository( + name="with_tag", + remote_url="https://github.com/DiamondLightSource/dodal.git", + target_revision="demo_tag", + ), ], ) diff --git a/tests/unit_tests/example_yaml/scratch.yaml b/tests/unit_tests/example_yaml/scratch.yaml index b64dc715f5..c1acc17be2 100644 --- a/tests/unit_tests/example_yaml/scratch.yaml +++ b/tests/unit_tests/example_yaml/scratch.yaml @@ -3,3 +3,12 @@ scratch: repositories: - name: dodal remote_url: https://github.com/DiamondLightSource/dodal.git + - name: with_target + remote_url: https://github.com/DiamondLightSource/dodal.git + target_revision: demo + - name: with_branch + remote_url: https://github.com/DiamondLightSource/dodal.git + branch: demo_branch + - name: with_tag + remote_url: https://github.com/DiamondLightSource/dodal.git + tag: demo_tag From de2bd59777cbc9f0a9db32c4805e8841c6d8ca28 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 15 Apr 2026 15:28:17 +0100 Subject: [PATCH 5/6] Fix missed branch rename --- tests/unit_tests/cli/test_scratch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index 47b9061864..33a5282164 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -292,7 +292,7 @@ def test_setup_scratch_iterates_repos( ScratchRepository( name="bar", remote_url="http://example.com/bar.git", - branch="demo", + target_revision="demo", ), ], ) From 841930b80324709b46e31f9494dfcdd73baf838d Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 15 Apr 2026 16:29:32 +0100 Subject: [PATCH 6/6] Add tests for revision warning logging --- tests/unit_tests/cli/test_scratch.py | 36 ++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index 33a5282164..42c00a4ca3 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -190,6 +190,42 @@ def test_existing_repo_not_changed_to_new_branch(mock_repo, directory_path: Path repo.create_head.assert_not_called() +@patch("blueapi.cli.scratch.Repo") +@patch("blueapi.cli.scratch.LOGGER") +def test_existing_repo_state_checked( + mock_logger: MagicMock, mock_repo: MagicMock, directory_path: Path +): + repo = mock_repo.return_value + repo.head.commit.name_rev = "current" + repo.refs = {"demo": Mock()} + + ensure_repo("http://example.com/foo.git", directory_path, "demo") + + mock_logger.warning.assert_called_once_with( + "Repository %s not at target revision: %r instead of %r", + directory_path.name, + repo.head.commit.name_rev, + "demo", + ) + + +@patch("blueapi.cli.scratch.Repo") +@patch("blueapi.cli.scratch.LOGGER") +def test_existing_repo_unknown_revision( + mock_logger: MagicMock, mock_repo: MagicMock, directory_path: Path +): + repo = mock_repo.return_value + repo.head.commit.name_rev = "current" + repo.refs = {} + + ensure_repo("http://example.com/foo.git", directory_path, "demo") + + mock_logger.warning.assert_called_once_with( + "Target revision %r not found", + "demo", + ) + + def test_setup_scratch_fails_on_nonexistant_root( nonexistant_path: Path, ):