Repository navigation
fix(xtest): make otdf-sdk-mgr check out source refs where make builds them - #681
Draft
dmihalcik-virtru wants to merge 5 commits into
Draft
dmihalcik-virtru wants to merge 5 commits into
dmihalcik-virtru wants to merge 5 commits into
Conversation
… them `install tip` passes make VERSIONS=<slug>, and the SDK Makefiles build src/$VERSIONS into dist/$VERSIONS. cmd_tip strips both the `sdk--` and `otdfctl--` prefixes from the flattened ref, but checkout_sdk_branch (js, java) stripped only `sdk--` and checkout_go_from_platform only `otdfctl--`. So go `sdk/v1.2.3` was checked out into src/sdk--v1.2.3 and js or java `otdfctl/<x>` into src/otdfctl--<x>, while make built src/v1.2.3 and src/<x>. With nothing there, make failed with an error that didn't mention the mismatch; with a tree an earlier checkout left there, it built that stale tree into the dist/ directory the caller expected. Both checkouts now name the worktree with _worktree_name, the same expression cmd_tip uses. dist/ names don't change, and src/ names change only for the two broken combinations. For go that moves the platform-src/ worktree too, and the src/ symlink follows it. The new test runs cmd_tip for go, java and js with git and make stubbed at the subprocess layer, for main, feature/x, sdk/v1.2.3, otdfctl/v0.24.0, refs/pull/123/head and a full SHA. It checks that src/ holds exactly src/$VERSIONS and, for go, that the link resolves into platform-src/$VERSIONS. Without the fix, go-sdk/v1.2.3, java-otdfctl/v0.24.0 and js-otdfctl/v0.24.0 fail. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
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 |
This was referenced Oct 26, 2026
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 28, 2026
) Part 2b of the [DSPX-4372-t1 plan](#657), split out of #626. **Based on `main`** (#667 has merged). Adds a benchmark manifest: a JSON representation of the SDK(s) under test, including how it was created or installed (built from what source, what version, etc). This record that connects benchmark preparation to measurement, reviewable on its own before anything writes or reads it. - otdf-sdk-mgr install benchmark` (#626) writes it - pytest reads it (#670) through a reader that imports nothing from this package - this JSON shape is the contract between them. ### What it does - **`manifest.py`, schema `v1alpha1`.** `InstallationManifest` holds the measured arms in request order, the platform and provisioning `otdfctl` pins separately, an explicit status (`success`, `neutral`, `partial` or `failed`) with a message, and `metadata.sdk`. Each `ArtifactIdentity` records the requested alias, repository, commit, `dist/` directory, install method and build settings. - **Strict reads.** `from_dict` and `read()` validate rather than coerce: - The manifest and each arm must have exactly the keys `to_dict` writes; missing and unknown keys are reported together. - Only `platform`/`otdfctl` (no pin), `commit_sha` and `installed_path` may be `null`. `metadata.sdk` must be non-empty. - The status must match the contents: NEUTRAL has two or more arms and nothing installed, SUCCESS has one or more arms that are all installed, and any other status carries a `status_message`. - Every failure raises `ManifestError`, a `ValueError`, and names the JSON path (`sdk_arms[2].commit_sha`) and, for an enum, the allowed values. `read()` adds the file path and wraps JSON decode errors, so one `except ValueError` covers everything. - **Invariants at construction.** `ArtifactIdentity` and `ResolvedArtifact` refuse an unknown install method, an empty `dist/` name, or a `commit_sha` that isn't a full lowercase 40- or 64-character hex SHA. An unknown commit (a js release found on npm but not in git) is `null`, never `"None"` or `""`. `write()` creates the parent directory and replaces the file atomically, and removes its temp file if it fails. - **Checks that run before anything builds.** `dist_slug_for` predicts each arm's `dist/` directory, so `check_collision` can reject two refs that would build into one directory at different commits (`sdk/feature` and `feature`, or `Feature` and `feature` on a case-insensitive disk), and `is_neutral_preparation` can tell when every ref names one build. - **One slug function.** `refs.dist_slug` is shared by `install tip` and `dist_slug_for`, so the predicted directory is the one make creates. It rejects a ref that names no directory. ### Compatibility - `install tip` output paths are unchanged, except that a ref naming no directory (`--ref sdk/`) now exits 1 with an error instead of resetting `src/` itself. That checkout and `make` agree on the `src/` name for `sdk/` and `otdfctl/` refs is fixed separately, on `main`, in #681. - Nothing reads the manifest yet. The xtest reader in #670 needs no change: field names and locations are as before. ### Size About 540 changed lines of non-test code, over the plan's ~400. About 235 of them are code. The rest are the docstrings that the JSON contract needs, because its readers don't import this package. Review asked for the strict reader and those docs in this PR, rather than shipping a lenient reader and tightening it later. ### Validation - `uv run pytest`: 342 passed. ruff, ruff format and pyright are clean on ruff 0.17, with no `noqa`. - `test_manifest.py` covers: - a literal golden of the JSON shape, asserted in both directions, plus round trips of every status by dataclass equality; - every rejection: missing, unknown and mistyped keys, falsy `metadata`, a non-object top level, and the status/contents rules; - null commits, and collisions and neutrality with three arms of which only some share a commit; - aliases at one commit, case-only collisions, release slugs taken from the resolved tag, and the `write()` cleanup. - 20 hand-made mutations of the new checks each fail a test. - `TestCmdTip` checks that `install tip` builds into the directory `dist_slug_for` predicts, across SDKs and ref shapes. ### 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 (merged) | `main` | | 2b: manifest contract | **#668 (this PR)** | `main` | | 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 | Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
#668 has landed, adding refs.dist_slug for cmd_tip's VERSIONS= and manifest.dist_slug_for. Both checkouts now name src/<slug> (and go's platform-src/<slug>) with it too, and _worktree_name, the same expression copied, goes away. That also gives the checkouts dist_slug's refusal of refs that name no directory: `otdf-sdk-mgr checkout js otdfctl/` doesn't pass through cmd_tip's check, and made src/ itself the worktree, then ran git reset --hard there. The test now takes src/ from the directory make runs in, and go's checkout from GOWORK, rather than hard-coding either, so make run in the wrong directory or against another checkout's go.work fails it. It adds pr:123, so a slug taken from the unexpanded ref fails, and a test that those refs raise before any git command runs. Docstrings that said callers use the checkouts' return value as VERSIONS, and that ref_slug matches checkout_sdk_branch, are corrected. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…rings Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
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.
What
otdf-sdk-mgr install tipchecks a ref out underxtest/sdk/<lang>/src/and then runsmakewithVERSIONS=<slug>; the SDK Makefiles buildsrc/$VERSIONSintodist/$VERSIONS.cmd_tipnames the slug withrefs.dist_slug(#668), which flattens/to--and strips bothsdk--andotdfctl--, but the checkouts named the worktree differently:checkout_sdk_branch(js, java) stripped onlysdk--, sootdfctl/<x>went tosrc/otdfctl--<x>while make builtsrc/<x>;checkout_go_from_platformstripped onlyotdfctl--, sosdk/v1.2.3went toplatform-src/sdk--v1.2.3(andsrc/sdk--v1.2.3) while make builtsrc/v1.2.3.Both checkouts now name the worktree with
refs.dist_slugas well.Why it matters
When
src/<slug>didn't exist, make failed with an error that never mentioned the mismatch. When an earlier checkout had left asrc/<slug>, make silently built that stale tree into thedist/directory the caller expected, so tests ran against the wrong source.dist_slugalso rejects refs that name no directory (sdk/,otdfctl/..).cmd_tipalready checked, butotdf-sdk-mgr checkoutcallscheckout_sdk_branchdirectly. There,otdfctl/madesrc/itself the worktree and rangit reset --hardin it. It now exits 1 with "names no dist/ directory".User-visible effect
dist/names are unchanged.src/worktree names change only for the two broken combinations: go withsdk/...refs (nowsrc/<x>andplatform-src/<x>), and js/java withotdfctl/...refs (nowsrc/<x>).go/platform-src/sdk--*plus theirgo/src/sdk--*links, and{js,java}/src/otdfctl--*. Remove them withgit --git-dir=<bare repo> worktree remove <dir>, where the bare repo isgo/platform-src/platform.git,js/src/web-sdk.gitorjava/src/java-sdk.git, then delete the go links.clean --src-onlydoesn't handle them yet (see below).Validation
tests/test_tip_checkout.pyruns the realcmd_tipfor go, java and js, with only git and make stubbed at the subprocess layer, overmain,feature/x,sdk/v1.2.3,otdfctl/v0.24.0,pr:123,refs/pull/123/headand a 40-char SHA. It asserts thatsrc/under make's working directory holds exactlysrc/$VERSIONS, and for go that the link resolves into the checkoutGOWORKnames. Without the fix,go-sdk/v1.2.3,java-otdfctl/v0.24.0andjs-otdfctl/v0.24.0fail. A second test checks thatsdk/,otdfctl/,otdfctl/.andsdk/..raise before any git command runs.GOWORKpointing at another checkout, and the slug taken from the unexpandedpr:N.uv run ruff check .,uv run ruff format --check .,uv run pyrightanduv run pytest(375 passed) inotdf-sdk-mgr/.Known issues, not addressed here
sdk/v0.33.0andotdfctl/v0.33.0, or web-sdk'ssdk/v0.4.0andv0.4.0, which are different commits.clean --src-onlyfails on go'ssrc/symlinks (and then never reaches js or java), skipsgo/platform-src/, and leaves worktree registrations behind.otdf-sdk-mgr checkout goandcheckout --allalways fail, though both READMEs document them.