diff --git a/helm/blueapi/config_schema.json b/helm/blueapi/config_schema.json index 1986d6f8df..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 of repo to check out - defaults to remote's default when cloning and the existing branch when the repo already exists", - "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 3159f6713e..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 of repo to check out - defaults to remote's default when cloning and the existing branch when the repo already exists", - "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 85ad9310ce..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,29 +69,29 @@ def ensure_repo( 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=target_revision) LOGGER.info(f"Cloned {remote_url} -> {local_directory}") elif local_directory.is_dir(): repo = Repo(local_directory) - LOGGER.info(f"Found {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 a file" + f"Unable to open {local_directory} as a git repository because it is not a " + "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 '%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..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 of repo to check out - defaults to remote's default when " - "cloning and the existing branch when the repo already exists" + "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/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/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index 1c78401430..42c00a4ca3 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,43 @@ 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() + + +@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( @@ -292,14 +324,11 @@ 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", - branch="demo", + target_revision="demo", ), ], ) 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 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", ), ], ),