diff --git a/otdf-sdk-mgr/AGENTS.md b/otdf-sdk-mgr/AGENTS.md index 9964dcc0b..449e0c435 100644 --- a/otdf-sdk-mgr/AGENTS.md +++ b/otdf-sdk-mgr/AGENTS.md @@ -47,21 +47,30 @@ before editing either module: worktree of the expected bare repo. The refresh runs `git worktree prune`, so a worktree directory that was removed can be added again. - **Platform tags are namespaced** as `service/vX.Y.Z`. `_resolve_platform_ref` - prefixes the `service/` infix on plain versions; raw SHAs, refs with a - `/`, `pr:N` shorthand (expanded to `refs/pull/N/head`), and `main`/`HEAD` - pass through unchanged. + prefixes the `service/` infix on plain versions (`v0.9.0`, `0.9.0`) and + nothing else. Branches, `HEAD`, SHAs, refs with a `/`, and `pr:N` + shorthand (expanded to `refs/pull/N/head`) pass through unchanged. + Prefixing every name that isn't obviously a ref would look up + `install tip --ref my-branch platform` as the tag `service/vmy-branch`. - **PR refs aren't under `refs/heads/`,** so `BARE_REFRESH` never pulls `refs/pull/N/head`. `_ensure_worktree` (and the SDK equivalents in `checkout.py`) explicitly `fetch origin +refs/...:refs/...` for any `refs/...` ref before adding or resetting the worktree. Otherwise `git worktree add refs/pull/N/head` dies with `invalid reference`, and a reset rebuilds the PR head as it was at the last fetch. -- **Mutable refs auto-refresh; immutable refs cache.** `refs.is_mutable_ref` - treats full-length hex SHAs and `/vX.Y.Z` tag forms as immutable - (reuse existing dist), and everything else as mutable (re-fetch the bare - repo, `git reset --hard` the worktree, drop the stale binary, rebuild). - Don't reintroduce an unconditional dist-exists skip — it silently serves - stale binaries when a user installs the same branch a second time. +- **Mutable refs auto-refresh; immutable refs cache.** Ask origin, not the + name: a branch can look like a version (web-sdk cuts `release/1.3.1`). + After the bare-repo refresh, `refs.is_mutable_target` takes what + `upstream_ref` resolved to a branch on origin (`refs/remotes/origin/*`), + or any other `refs/...` ref but a tag, as mutable; only names it passed + through fall back on the name heuristic `refs.is_mutable_ref`. Mutable + means `git reset --hard` the worktree, and for the platform drop the stale + binary and rebuild. `install_platform_source` looks for a cached dist + before the refresh, so it reuses one only for a full SHA or a + `service/vX.Y.Z` release (`_is_pinned`) and sends anything else through + `_ensure_worktree` and a rebuild. Don't reintroduce an unconditional + dist-exists skip — it silently serves stale binaries when a user installs + the same branch a second time. - Subprocess output is **not captured** — long-running `go build` / `git clone` streams to the terminal so users can see progress. On failure the error message just reports the command and exit code. diff --git a/otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py b/otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py index 6cf637f10..94fe7290b 100644 --- a/otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py +++ b/otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py @@ -9,7 +9,7 @@ from typing import Any from otdf_sdk_mgr.config import SDK_BARE_REPOS, SDK_GIT_URLS, get_sdk_dirs -from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_ref +from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_target # `git clone --bare` records no fetch refspec: `git fetch` alone updates no refs # and `--tags` adds only tags, so the bare repo's branches stay where the clone @@ -173,7 +173,7 @@ def checkout_sdk_branch(language: str, branch: str) -> str: target = upstream_ref(bare_repo_path, branch) if worktree_path.exists(): - if is_mutable_ref(branch): + if is_mutable_target(branch, target): print(f"Worktree exists at {worktree_path}; resetting to '{branch}'.") require_worktree(worktree_path, bare_repo_path) _run(["git", "-C", str(worktree_path), "reset", "--hard", target]) @@ -248,7 +248,7 @@ def checkout_go_from_platform(ref: str) -> Path: target = upstream_ref(bare_repo_path, ref) if worktree_path.exists(): - if is_mutable_ref(ref): + if is_mutable_target(ref, target): print(f"Worktree for ref '{ref}' exists at {worktree_path}; resetting.") require_worktree(worktree_path, bare_repo_path) _run(["git", "-C", str(worktree_path), "reset", "--hard", target]) diff --git a/otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py b/otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py index c1fb73f88..cb0fdb570 100644 --- a/otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py +++ b/otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py @@ -18,8 +18,8 @@ from otdf_sdk_mgr.checkout import BARE_REFRESH, CheckoutError, require_worktree, upstream_ref from otdf_sdk_mgr.config import SDK_GIT_URLS, SDK_TAG_INFIXES -from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_ref, ref_slug -from otdf_sdk_mgr.semver import normalize_version +from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_target, ref_slug +from otdf_sdk_mgr.semver import normalize_version, parse_semver PLATFORM_BARE_REPO = "platform.git" HELPER_SCRIPTS_BRANCH = "main" @@ -115,32 +115,49 @@ def _is_hex(s: str) -> bool: return bool(s) and all(c in "0123456789abcdef" for c in s.lower()) +def _is_version(s: str) -> bool: + """A plain version, `v0.9.0` or `0.9.0`. SEMVER_RE would let a `/` into a + pre-release suffix, but a name with one is a ref.""" + return "/" not in s and parse_semver(s) is not None + + def _resolve_platform_ref(version_or_ref: str) -> str: - """Turn a user-supplied version into the actual git ref to checkout. - - `v0.9.0` → `service/v0.9.0` (matches SDK_TAG_INFIXES["platform"]). - `pr:42` → `refs/pull/42/head` (expanded first, then passes through the - `/` check). - A ref that already contains `/`, a full-length hex SHA, or `main` is - returned as-is. 7-39 char hex inputs are returned unchanged — they - might be abbreviated SHAs or branch names; `install_platform_source` + """Turn a user-supplied version or ref into the git ref to check out. + + Only a plain version names a release, so only it gets the tag infix: + `v0.9.0` or `0.9.0` → `service/v0.9.0` (SDK_TAG_INFIXES["platform"]). + `pr:42` → `refs/pull/42/head`. Anything else is returned as-is for git + to resolve: a branch (`main`, `my-branch`, `release/v1.2.3`), `HEAD`, a + namespaced tag (`service/v0.9.0`), a `refs/...` ref, or a SHA. 7-39 char + hex might be an abbreviated SHA or a branch name; `install_platform_source` resolves the ambiguity via `_expand_short_sha` once the bare repo is available. """ version_or_ref = expand_pr_shorthand(version_or_ref) - infix = SDK_TAG_INFIXES.get("platform", "service") if "/" in version_or_ref and (":" in version_or_ref or "@" in version_or_ref): raise PlatformInstallError( f"container-image refs are not supported: {version_or_ref!r}; " "use a git ref like v0.9.0, service/v0.9.0, main, or a SHA" ) - if "/" in version_or_ref or version_or_ref in ("main", "HEAD"): - return version_or_ref - if len(version_or_ref) in (40, 64) and _is_hex(version_or_ref): - return version_or_ref - if 7 <= len(version_or_ref) <= 39 and _is_hex(version_or_ref): - return version_or_ref - return f"{infix}/{normalize_version(version_or_ref)}" + if _is_version(version_or_ref): + infix = SDK_TAG_INFIXES.get("platform", "service") + return f"{infix}/{normalize_version(version_or_ref)}" + return version_or_ref + + +def _is_pinned(full_ref: str) -> bool: + """True if `full_ref` names one commit for good: a full SHA, or a release + tag `service/vX.Y.Z`. + + Only these may reuse a dist without refreshing the bare repo. Anything else + might be a branch, whatever its name looks like (`release/v1.2.3`), and + only origin can say. + """ + if len(full_ref) in (40, 64) and _is_hex(full_ref): + return True + infix = SDK_TAG_INFIXES.get("platform", "service") + version = full_ref.removeprefix(f"{infix}/") + return version != full_ref and _is_version(version) def _expand_short_sha(short: str) -> str: @@ -177,10 +194,10 @@ def _ensure_worktree(ref: str) -> Path: Branch names resolve to origin's current tip via `upstream_ref`; a branch origin has deleted raises PlatformInstallError. For mutable refs - (branches, PR heads) an existing worktree is reset to that tip so a - subsequent install picks up new commits, unless it is not a worktree of - the bare repo (PlatformInstallError). For immutable refs (tags, SHAs) we - just reuse. + (`is_mutable_target`: branches, whatever their names look like, and PR + heads) an existing worktree is reset to that tip so a subsequent install + picks up new commits, unless it is not a worktree of the bare repo + (PlatformInstallError). For immutable refs (tags, SHAs) we just reuse. """ bare = _ensure_bare_repo() # `_ensure_bare_repo` refreshes branches and tags only (BARE_REFRESH), so @@ -193,7 +210,7 @@ def _ensure_worktree(ref: str) -> Path: target = _upstream_ref(bare, ref) worktree = _worktree_path_for(ref) if worktree.exists(): - if is_mutable_ref(ref): + if is_mutable_target(ref, target): print(f"Worktree exists at {worktree}; resetting to {ref}.") _require_worktree(worktree, bare) _run(["git", "-C", str(worktree), "reset", "--hard", target]) @@ -253,9 +270,12 @@ def install_platform_source(ref: str, dist_name: str | None = None) -> Path: (`service/v0.9.0`), a branch (`main`), a PR shorthand (`pr:42`), a raw ref (`refs/pull/42/head`), or a SHA. Returns the dist dir. - For immutable refs (tags, SHAs) an existing dist is reused. For mutable - refs (branches, PR heads) the bare repo is re-fetched, the worktree is - reset, the old dist is removed, and the service binary is rebuilt. + A release or a full SHA reuses an existing dist without touching the bare + repo, so a cached release installs offline. Anything else might be a + branch, whatever its name looks like, so it goes through + `_ensure_worktree`, which re-fetches the bare repo and resets the worktree + if the ref can move; then the old dist is removed and the service binary + is rebuilt. """ full_ref = _resolve_platform_ref(ref) if 7 <= len(full_ref) <= 39 and _is_hex(full_ref): @@ -263,17 +283,18 @@ def install_platform_source(ref: str, dist_name: str | None = None) -> Path: # full-SHA case. Falls through to branch-name handling if git can't # resolve the prefix; raises on ambiguity. full_ref = _expand_short_sha(full_ref) + pinned = _is_pinned(full_ref) if dist_name is None: - if is_mutable_ref(full_ref): - dist_name = ref_slug(full_ref) - else: + if pinned: # Normalize only the semver tail so namespaced tags like # `service/v0.9.0` produce the same dist_name as `v0.9.0`. dist_name = normalize_version(full_ref.rsplit("/", 1)[-1]) + else: + dist_name = ref_slug(full_ref) dist_dir = _platform_dist_root() / dist_name binary = dist_dir / "service" marker = dist_dir / ".complete" - if binary.exists() and marker.exists() and not is_mutable_ref(full_ref): + if binary.exists() and marker.exists() and pinned: print(f" Dist already present at {dist_dir}; skipping build.") return dist_dir worktree = _ensure_worktree(full_ref) diff --git a/otdf-sdk-mgr/src/otdf_sdk_mgr/refs.py b/otdf-sdk-mgr/src/otdf_sdk_mgr/refs.py index 16f118599..b2fb60259 100644 --- a/otdf-sdk-mgr/src/otdf_sdk_mgr/refs.py +++ b/otdf-sdk-mgr/src/otdf_sdk_mgr/refs.py @@ -28,7 +28,9 @@ def expand_pr_shorthand(ref: str) -> str: def is_mutable_ref(ref: str) -> bool: """True if `ref` can change over time (branches, PR heads); False for tags/SHAs. - Used to decide whether to re-fetch and rebuild on subsequent installs. + A guess from the name alone, which takes a branch named like a version + (`release/v1.2.3`) for a tag. Once the bare repo has been refreshed, decide + with `is_mutable_target` instead. The caller is expected to have already expanded `pr:N` shorthand and any tag-namespace prefixes (e.g. `service/v0.9.0`, `sdk/v0.4.0`). @@ -42,6 +44,23 @@ def is_mutable_ref(ref: str) -> bool: return not SEMVER_RE.match(tail) +def is_mutable_target(ref: str, target: str) -> bool: + """`is_mutable_ref`, once `checkout.upstream_ref` has resolved `ref` to `target`. + + upstream_ref maps every branch origin has onto `refs/remotes/origin/*`, so + such a target is a branch however its name looks: web-sdk cuts + `release/1.3.1`. Any other `refs/...` ref but a tag, such as a PR head, can + move too. Other names upstream_ref passes through: tags, SHAs, and names + git can't resolve, such as a branch origin deleted since the clone. Those + are judged by name. Answering False for all of them would quietly reuse a + deleted branch's worktree, where a reset fails loudly unless the name looks + like a version. + """ + if target.startswith("refs/") and not target.startswith("refs/tags/"): + return True + return is_mutable_ref(ref) + + def ref_slug(ref: str) -> str: """Slugify a git ref for use as a directory name. diff --git a/otdf-sdk-mgr/tests/test_checkout.py b/otdf-sdk-mgr/tests/test_checkout.py index c7f63833e..89464f055 100644 --- a/otdf-sdk-mgr/tests/test_checkout.py +++ b/otdf-sdk-mgr/tests/test_checkout.py @@ -186,6 +186,18 @@ def test_immutable_ref_is_checked_out_then_reused(self, entry: str, kind: str, o _assert_untouched(worktree, pinned) + @pytest.mark.parametrize("branch", ["release/v1.2.3", "release/1.3.1"]) + def test_version_named_branch_follows_origin(self, entry: str, branch: str, origin: Path): + """A branch whatever its name looks like; web-sdk cuts `release/1.3.1`. + Reused like a tag's, its worktree would keep the first commit.""" + _git("branch", branch, cwd=origin) + worktree = _check_out(entry, origin, branch) + _git("update-ref", f"refs/heads/{branch}", _advance(origin), cwd=origin) + + _check_out(entry, origin, branch) + + assert _head(worktree) == _head(origin) + def test_pr_ref_follows_a_force_push(self, entry: str, origin: Path): """BARE_REFRESH skips `refs/pull/*`, so each checkout fetches the PR head itself, and has to overwrite the old one.""" @@ -294,6 +306,51 @@ def test_scripts_directory_that_lost_its_git_file_is_refused( _assert_enclosing_untouched(enclosing) +@pytest.fixture +def builds(monkeypatch: pytest.MonkeyPatch) -> list[str]: + """Stub out the platform's `go build`; record the commit each build compiles.""" + built: list[str] = [] + + def build(worktree: Path, dist_dir: Path) -> Path: + built.append(_head(worktree)) + dist_dir.mkdir(parents=True, exist_ok=True) + (dist_dir / ".complete").write_text("") + binary = dist_dir / "service" + binary.write_text("") + return binary + + monkeypatch.setattr(platform_installer, "_build_service", build) + return built + + +class TestPlatformInstall: + def test_version_named_branch_is_rebuilt(self, origin: Path, builds: list[str]): + """Not served from the first build's dist as if it were a release.""" + _git("branch", "release/v1.2.3", cwd=origin) + platform_installer.install_platform_source("release/v1.2.3") + tip = _advance(origin) + _git("update-ref", "refs/heads/release/v1.2.3", tip, cwd=origin) + + dist = platform_installer.install_platform_source("release/v1.2.3") + + assert builds[1:] == [tip] + assert dist.name == "release--v1.2.3" + + @pytest.mark.parametrize("kind", ["release", "sha"]) + def test_pinned_build_is_reused_without_origin( + self, kind: str, origin: Path, builds: list[str] + ): + """Neither can move, so a finished dist needs no fetch: `install lts` + and `install release` work offline once built.""" + _git("tag", "service/v1.2.3", cwd=origin) + ref = "v1.2.3" if kind == "release" else _head(origin) + dist = platform_installer.install_platform_source(ref) + shutil.rmtree(origin) + + assert platform_installer.install_platform_source(ref) == dist + assert len(builds) == 1 + + class TestInstallTip: def test_branch_gone_from_origin_is_an_error_not_a_traceback(self, origin: Path): _check_out("sdk", origin, "main") @@ -304,3 +361,14 @@ def test_branch_gone_from_origin_is_an_error_not_a_traceback(self, origin: Path) assert result.exit_code == 1 assert isinstance(result.exception, SystemExit), result.exception assert "Error: branch 'other' is not on origin" in result.output + + def test_platform_branch_without_a_slash(self, origin: Path, builds: list[str]): + """The `tip` docstring's own example. Not a version, so not looked up + as the tag `service/vmy-branch`.""" + _git("branch", "my-branch", cwd=origin) + + result = CliRunner().invoke(install_app, ["tip", "--ref", "my-branch", "platform"]) + + assert result.exit_code == 0, result.output + assert builds == [_head(origin)] + assert (origin.parent / "platform" / "dist" / "my-branch" / "service").exists() diff --git a/otdf-sdk-mgr/tests/test_platform_installer.py b/otdf-sdk-mgr/tests/test_platform_installer.py index 5a62964e7..3f2f5a6de 100644 --- a/otdf-sdk-mgr/tests/test_platform_installer.py +++ b/otdf-sdk-mgr/tests/test_platform_installer.py @@ -13,6 +13,7 @@ _resolve_platform_ref, install_platform_source, ) +from otdf_sdk_mgr.refs import ref_slug @pytest.mark.parametrize( @@ -20,8 +21,12 @@ [ ("v0.9.0", "service/v0.9.0"), ("0.9.0", "service/v0.9.0"), + ("v0.9.0-rc.1", "service/v0.9.0-rc.1"), + # Anything else that isn't a version is a ref for git to resolve. ("main", "main"), ("HEAD", "HEAD"), + ("my-branch", "my-branch"), + ("release/v1.2.3", "release/v1.2.3"), ("service/v0.9.0", "service/v0.9.0"), ("a" * 40, "a" * 40), ("b" * 64, "b" * 64), @@ -114,6 +119,9 @@ def fake_expand_short_sha(short: str) -> str: # Mutable refs: use ref_slug ("main", "main"), ("refs/pull/42/head", "refs--pull--42--head"), + ("my-branch", "my-branch"), + # A branch named like a version, not release v1.2.3's dist. + ("release/v1.2.3", "release--v1.2.3"), ], ) def test_dist_name_derivation( @@ -125,17 +133,26 @@ def test_dist_name_derivation( assert dist_dir.name == expected_dist_name +@pytest.mark.parametrize( + "ref,dist_name", + [ + ("v0.9.0", "v0.9.0"), + ("service/v0.9.0", "v0.9.0"), + ("a" * 40, "v" + "a" * 40), + ], +) def test_immutable_ref_with_complete_marker_skips_rebuild( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, ref: str, dist_name: str ): - """An existing immutable build (binary + .complete) is reused, not rebuilt.""" + """An existing release or SHA build (binary + .complete) is reused, not + rebuilt, without touching the bare repo.""" calls = _stub_installer(monkeypatch, tmp_path) - dist_dir = tmp_path / "dist" / "v0.9.0" + dist_dir = tmp_path / "dist" / dist_name dist_dir.mkdir(parents=True) (dist_dir / "service").write_text("prebuilt") (dist_dir / ".complete").write_text("") - install_platform_source("v0.9.0") + install_platform_source(ref) assert calls["build_service"] == [] assert calls["ensure_worktree"] == [] @@ -155,20 +172,23 @@ def test_immutable_ref_without_complete_marker_rebuilds( assert len(calls["build_service"]) == 1 +@pytest.mark.parametrize("ref", ["main", "my-branch", "release/v1.2.3", "sdk/v0.4.0"]) def test_mutable_ref_drops_existing_binary_and_rebuilds( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, ref: str ): - """A mutable ref (branch) re-fetches and rebuilds even if a binary exists.""" + """A branch re-fetches and rebuilds even if a binary exists, whatever its + name looks like. So does any ref that isn't provably a release or a SHA + before the bare repo is refreshed: `sdk/v0.4.0` is a tag in the + monorepo, but `release/v1.2.3` is shaped the same.""" calls = _stub_installer(monkeypatch, tmp_path) - dist_dir = tmp_path / "dist" / "main" + dist_dir = tmp_path / "dist" / ref_slug(ref) dist_dir.mkdir(parents=True) stale_binary = dist_dir / "service" stale_binary.write_text("stale") (dist_dir / ".complete").write_text("") - install_platform_source("main") - - assert calls["ensure_worktree"] == ["main"] + assert install_platform_source(ref) == dist_dir + assert calls["ensure_worktree"] == [ref] assert len(calls["build_service"]) == 1 diff --git a/otdf-sdk-mgr/tests/test_refs.py b/otdf-sdk-mgr/tests/test_refs.py index da77f78bf..a6b73c9af 100644 --- a/otdf-sdk-mgr/tests/test_refs.py +++ b/otdf-sdk-mgr/tests/test_refs.py @@ -2,7 +2,7 @@ import pytest -from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_ref, ref_slug +from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_ref, is_mutable_target, ref_slug @pytest.mark.parametrize( @@ -51,6 +51,29 @@ def test_is_mutable_ref(ref, expected): assert is_mutable_ref(ref) is expected +@pytest.mark.parametrize( + "ref,target,expected", + [ + # Branches origin has, whatever the name looks like. + ("main", "refs/remotes/origin/main", True), + ("release/v1.2.3", "refs/remotes/origin/release/v1.2.3", True), + ("v1.2.3", "refs/remotes/origin/v1.2.3", True), + # Explicit refs: anything but a tag can move. + ("refs/pull/42/head", "refs/pull/42/head", True), + ("refs/heads/release/v1.2.3", "refs/heads/release/v1.2.3", True), + ("refs/tags/v1.2.3", "refs/tags/v1.2.3", False), + # Names upstream_ref passed through: as is_mutable_ref says. + ("v1.2.3", "v1.2.3", False), + ("service/v0.9.0", "service/v0.9.0", False), + ("a" * 40, "a" * 40, False), + # Not on origin; resetting to it fails loudly instead of reusing. + ("gone", "gone", True), + ], +) +def test_is_mutable_target(ref, target, expected): + assert is_mutable_target(ref, target) is expected + + @pytest.mark.parametrize( "inp,expected", [