Repository navigation
fix(xtest): make otdf-sdk-mgr check out go from the platform monorepo - #676
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 23, 2026
…pted js installs (#667) Part 2a of the [DSPX-4372-t1 plan](#657), split out of #626: fixes the benchmark needs, which every `otdf-sdk-mgr` install benefits from. **Based on `main`.** ### What it does - **Bare repos refresh their branches.** `git clone --bare` records no fetch refspec, so the `fetch --all --tags` before every checkout refreshed tags only. Every branch stayed where the first clone found it, and `install tip main` or `--platform main` built that stale commit. `checkout.BARE_REFRESH` now fetches origin's branches into `refs/remotes/origin/*` with `--prune`, for the SDK and platform bare repos alike, and `checkout.upstream_ref` maps a branch name onto them. Fetching into the bare repo's own `refs/heads/*` instead would move branches that other worktrees have checked out, leaving their HEAD ahead of their files, and would fail for good once upstream replaced a branch `foo` with `foo/bar`. - **Worktrees are detached** and reset to the refreshed ref. A branch origin no longer has is refused rather than built from the clone's stale copy, and `HEAD` means origin's default branch as of the clone; a platform `HEAD` worktree used to reset to itself. - **Go source rebuilds work a second time.** `checkout_go_from_platform` reset an existing worktree to a `FETCH_HEAD` fetched into the bare repo, which no worktree can see, so every rebuild after the first failed with `pathspec 'FETCH_HEAD' did not match`. It now resets to the refreshed ref, like the SDK and platform paths. - **`install tip` reports checkout failures as errors.** A refused branch, or a failed git command with git's stderr, now prints `Error: …` and exits 1 for go, js and java, as the platform path already did, instead of a traceback. - **An interrupted js install leaves nothing behind.** #663 already removes a partial `dist/` when npm fails. A Ctrl-C mid-install still left a directory holding only `cli.sh`, which `install_release` then skipped as installed from then on. Both js install paths now remove it unless the install finished. - `AGENTS.md` claimed the bare clone mapped `refs/heads/*`, which is what hid the first bug. It and the code comments also said `git pull` fails in a worktree; it merges origin's default branch into whatever is checked out. ### Validation - `uv run pytest` in `otdf-sdk-mgr`: 207 passed, and 321 at the top of this track (#642). ruff, ruff format and pyright are clean. - `test_checkout.py` runs real git against a local origin. Every shared behaviour is parametrized over the SDK, go and platform entry points: new and existing worktrees follow their branch, `HEAD` (including beside a tag named `main`), a deleted branch is refused, tags and SHAs are checked out and then reused, and a force-pushed `refs/pull/N/head` is followed. This replaces the stubbed command-string test for platform PR refs. SDK-only tests guard other worktrees, force pushes and `x` → `x/y` renames; platform-only tests cover the helper-scripts worktree. A `CliRunner` test covers the `install tip` error path. - Mutation-checked: `symbolic-ref --short`, treating every non-origin name as a deleted branch, dropping go's explicit PR-ref fetch or its `+`, swallowing go's refusal, and dropping the `install tip` catch each fail a test. - Two new tests interrupt each js install path. - `isolated_git` keeps those tests off the developer's git config, any inherited `GIT_DIR`, and the network (`GIT_ALLOW_PROTOCOL=file`). ### Follow-ups Older problems found while reviewing this PR are fixed in a stack of drafts based on it: #673 (don't reset a directory that isn't a worktree) → #674 (branch names vs versions) → #675 (tags moved upstream) → #676 (`checkout go` / `--all`). ### Stack ([DSPX-4372-t1 plan](#657)) Two parallel tracks from `main`; they share no imports and meet in stage 5. | Part | PR | Base | |---|---|---| | 2a: bare-repo and js install fixes | **#667 (this PR)** | `main` | | 2b: manifest contract | #668 | 2a | | 2c: `install benchmark` | #626 | 2b | | 2d: `otdf-local env` exports | #642 | 2c | | 3a: arms as a tuple | #669 | `main` | | 3b: two to four arms | #658 | 3a | | 3c: pytest reads the manifest | #670 | 3b | | 4: candidate-to-candidate conclusions | #659 | 3c | | 5, 6: CI and report | #660, #661 | not yet moved onto these stacks | <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * SDK and platform worktrees now follow updated branches and pull request refs, including after force-pushes. Missing branches produce a clear error instead of a traceback. * Interrupted JavaScript installations no longer leave partial distribution files behind. * **Documentation** * Updated platform installation guidance for branch refreshes, detached worktrees, and pull request refs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
`otdf-sdk-mgr checkout go <ref>`, the README's example, always failed: the command handed every SDK to checkout_sdk_branch, which refuses go because otdfctl's source now comes from the platform monorepo through checkout_go_from_platform. `checkout --all` failed the same way, and since go is first in ALL_SDKS, it stopped before js or java was checked out. The command now sends go to checkout_go_from_platform, as installers.cmd_tip already does for `install tip`. That adds the platform worktree under sdk/go/platform-src/<slug> and links sdk/go/src/<slug> to its otdfctl/, without the build. Its errors now read like `install tip`'s. tip's handler moves into cli_install.exit_with_error, which both commands call, so a failed git command shows the stderr checkout._run captured, not only the command and its exit status. A RuntimeError, from a platform ref that predates otdfctl/ or a missing git binary, is reported rather than raised as a traceback. The command still catches ValueError, not just its subclass CheckoutError, so an unknown SDK name stays a one-line error. typer.Exit is a RuntimeError too, so the missing-SDK usage error is re-raised ahead of that clause; otherwise it would be reported a second time as a bare `Error:`. The tests drive the command through CliRunner with real git against the local origin, which now stands in for java as well: `checkout go main`, `checkout --all main`, git's stderr for a ref origin doesn't have, a go ref without otdfctl/, and each usage error reported once. The first four failed before the fix; the usage errors pass either way and guard the wider handler. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-checkout-go
branch
from
October 23, 2026 14:02
27d0715 to
779ddbc
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-moved-tags
branch
from
October 23, 2026 14:02
f0ea7c8 to
394fb42
Compare
X-Test Failure Report |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth follow-up from #667's review. Based on #675 (
DSPX-4372-sdkmgr-moved-tags).What it does
checkout go <ref>andcheckout --allwork. Thecheckoutcommand handed every SDK tocheckout_sdk_branch, which raisesValueErrorfor go now that otdfctl's source lives in the platform monorepo. Sootdf-sdk-mgr checkout go main, the README's example, always failed.checkout --allfailed on go too, and go is first inALL_SDKS, so js and java were never checked out. go now goes tocheckout_go_from_platform, asinstallers.cmd_tipalready does forinstall tip. That creates the platform worktree atsdk/go/platform-src/<slug>and symlinkssdk/go/src/<slug>to itsotdfctl/, without building.install tip's. tip's handler is nowcli_install.exit_with_error, and both commands call it. A failed git command shows the stderrcheckout._runcaptured, such asfatal: invalid reference: no-such-ref, not onlyCommand [...] returned non-zero exit status 128. ARuntimeError(a platform ref older thanotdfctl/, or no git binary) is anError:line instead of a traceback.checkoutstill catchesValueError, not only its subclassCheckoutError, socheckout ruststays a one-line "Unsupported language" error.typer.Exitis aRuntimeError, so the missing-SDK usage error is re-raised before that clause; otherwise it would print a second, emptyError:.for s in ALL_SDKS:tofor s in DEFAULT_SDKS:. This PR changes only the loop body, socli.pygets one two-line conflict (checked withgit merge-fileagainst feat(xtest): headless-browser web SDK flavour alongside js #650's head). Resolve it as:cli_install.pymerge cleanly.checkout webthen goes throughcheckout_sdk_branchand feat(xtest): headless-browser web SDK flavour alongside js #650's alias tojs, the same wayinstall tip webdoes.Validation
uv run pytestinotdf-sdk-mgr: 268 passed (6 new). ruff, ruff format and pyright are clean.TestCheckoutCommandtests run the command throughCliRunnerwith real git against a local origin. Theoriginfixture now stands in for java as well, so nothing reaches the network. The cases:checkout go main: the worktree and symlink are at origin's tip.checkout --all main: go, js and java are all at origin's tip.otdfctl/: anError:line, no traceback.--allfail with "checkout_sdk_branch does not support 'go'", and the stderr test finds no git stderr in the output. The two usage-error cases pass before and after; they guard the wider handler.typer.Exitre-raise fails the no-SDK case (a secondError:), removingRuntimeErrorfails the otdfctl case (a traceback), and narrowingValueErrortoCheckoutErrorfails the unknown-SDK case.Not done here
checkout go mainbut not--all, which only appears in--help. Left as is.install tipwith an unknown SDK name still dies with aKeyErrorfromcmd_tip'ssdk_dirs[sdk]before it gets to the checkout. That is a separate bug.Stack
Follow-ups from #667's review, each based on the one before:
checkout go/checkout --all