fix(xtest): remove standalone otdfctl source path; route go through platform monorepo#458
fix(xtest): remove standalone otdfctl source path; route go through platform monorepo#458dmihalcik-virtru wants to merge 4 commits into
Conversation
…latform monorepo
The archived opentdf/otdfctl repo is no longer source-cloned. All Go heads
and post-v0.31.0 release tags resolve against opentdf/platform with the
otdfctl/ tag infix; bare semver inputs auto-prepend otdfctl/. Pre-v0.31.0
tags fall back to the Go module proxy for artifact install only.
Builds of the platform-embedded otdfctl now run with GOWORK pointing at
the platform checkout's go.work (auto-detected by the Makefile from each
src/{tag} symlink target), so protocol/go, sdk, and lib/* resolve to the
local platform sources instead of the otdfctl go.mod's pinned releases.
This fixes the failing platform PR scenario where a single PR adds proto
symbols and consumes them from otdfctl/.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSwitches Go resolution and installs to use the opentdf/platform monorepo (otdfctl subtree) for modern tags, adds platform worktree/symlink checkout support, removes per-version ChangesGo SDK Platform Migration
Estimated code review effort: Possibly related PRs:
Suggested reviewers:
Poem:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py`:
- Around line 122-124: The current logic that removes and recreates src_link
uses src_link.unlink() which raises IsADirectoryError when src_link is a real
directory; update the removal logic (around the
src_link.is_symlink()/src_link.exists() block) to: if src_link.is_symlink():
call src_link.unlink(); elif src_link.exists(): if src_link.is_dir(): call
shutil.rmtree(str(src_link)) else: call src_link.unlink(); then recreate the
symlink with src_link.symlink_to(...). Also add an import for shutil at the top
and ensure removal calls are safe (wrap with minimal error handling if needed).
- Around line 98-100: The current update path uses "git pull origin <ref>" which
fails for tags or detached HEADs; replace that single pull with a ref-safe
sequence: run a fetch that includes tags (e.g., _run(["git", "-C",
str(worktree_path), "fetch", "origin", "--tags"]) or equivalent), then attempt
to update the worktree by checking out the supplied ref in a way that supports
tags and commits (use "git checkout --detach <ref>" for non-branch refs) and
fall back to checking out the branch or resetting to origin/<ref> when the ref
is a branch; update the code around worktree_path, ref, and _run to perform
fetch + checkout/reset instead of the single git pull.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb5f6af1-3c9c-4bd2-a8bc-131b3ccab915
📒 Files selected for processing (11)
.github/workflows/xtest.ymlotdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/checkout.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.pyotdf-sdk-mgr/src/otdf_sdk_mgr/config.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.pyotdf-sdk-mgr/tests/test_resolve.pyxtest/sdk/go/Makefilextest/setup-cli-tool/action.yaml
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the otdfctl Go tool management from a standalone repository to the opentdf/platform monorepo. Key changes include updating the version resolution logic to automatically distinguish between archived standalone releases (pre-v0.31.0) and newer monorepo-based releases (v0.31.0+). The SDK manager now handles source builds by cloning the platform monorepo, symlinking the otdfctl subdirectory, and configuring the GOWORK environment variable to ensure local dependencies resolve correctly. Additionally, documentation and GitHub Action workflows were updated to reflect this architectural shift. I have no feedback to provide as there were no review comments to assess.
…heckout - Remove existing src/<ref> path safely when it's a real directory (not just a symlink), so upgrades from prior layouts don't hard-fail. - Update existing worktrees via fetch + checkout FETCH_HEAD so tags and detached refs work, not just branches. - Accept deprecated otdfctl-source workflow input and emit a warning so callers passing it don't break. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py (1)
101-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFetch in the linked worktree before checking out
FETCH_HEAD.Line 101 writes
FETCH_HEADvia the bare repo, but Line 102 resolvesFETCH_HEADfrom the linked worktree. In Git worktrees that pseudo-ref is worktree-scoped, so reruns against an existing checkout can still fail or pick up stale state.Suggested fix
- _run(["git", f"--git-dir={bare_repo_path}", "fetch", "origin", ref, "--tags"]) + _run(["git", "-C", str(worktree_path), "fetch", "origin", ref, "--tags"]) _run(["git", "-C", str(worktree_path), "checkout", "--force", "FETCH_HEAD"])#!/bin/bash set -euo pipefail tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT cd "$tmp" git init --bare remote.git git clone remote.git seed >/dev/null cd seed git config user.email test@example.com git config user.name test echo one > a.txt git add a.txt git commit -m "init" >/dev/null git branch -M main git push origin main >/dev/null git tag otdfctl/v0.31.0 git push origin --tags >/dev/null echo two > a.txt git commit -am "second" >/dev/null git push origin main >/dev/null cd "$tmp" git clone --bare remote.git bare.git >/dev/null git --git-dir=bare.git worktree add wt main >/dev/null echo "bare git-path FETCH_HEAD: $(git --git-dir=bare.git rev-parse --git-path FETCH_HEAD)" echo "worktree git-path FETCH_HEAD: $(git -C wt rev-parse --git-path FETCH_HEAD)" echo echo "Fetch through bare repo, then checkout FETCH_HEAD in worktree:" git --git-dir=bare.git fetch origin otdfctl/v0.31.0 --tags >/dev/null set +e git -C wt checkout --force FETCH_HEAD rc_bare=$? set -e echo "checkout exit code after bare fetch: $rc_bare" echo echo "Fetch through worktree, then checkout FETCH_HEAD in worktree:" git -C wt fetch origin otdfctl/v0.31.0 --tags >/dev/null git -C wt checkout --force FETCH_HEAD >/dev/null echo "checked out: $(git -C wt rev-parse --short HEAD)"Expected result: the two
FETCH_HEADpaths differ, and the bare-repo fetch path is the one to watch on the runner image; the worktree-local fetch should consistently make the subsequent checkout succeed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py` around lines 101 - 102, The code fetches into the bare repo but then checks out FETCH_HEAD from the linked worktree, which can be stale; update the logic in checkout.py so the worktree itself is fetched before checking out: either replace the bare-repo fetch call with a worktree-scoped fetch using _run(["git", "-C", str(worktree_path), "fetch", "origin", ref, "--tags"]) or add an additional _run(["git", "-C", str(worktree_path), "fetch", "origin", ref, "--tags"]) immediately before the existing _run(["git", "-C", str(worktree_path), "checkout", "--force", "FETCH_HEAD"]) to ensure FETCH_HEAD is resolved in the worktree context (referencing _run, bare_repo_path, worktree_path, and FETCH_HEAD).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py`:
- Around line 101-102: The code fetches into the bare repo but then checks out
FETCH_HEAD from the linked worktree, which can be stale; update the logic in
checkout.py so the worktree itself is fetched before checking out: either
replace the bare-repo fetch call with a worktree-scoped fetch using _run(["git",
"-C", str(worktree_path), "fetch", "origin", ref, "--tags"]) or add an
additional _run(["git", "-C", str(worktree_path), "fetch", "origin", ref,
"--tags"]) immediately before the existing _run(["git", "-C",
str(worktree_path), "checkout", "--force", "FETCH_HEAD"]) to ensure FETCH_HEAD
is resolved in the worktree context (referencing _run, bare_repo_path,
worktree_path, and FETCH_HEAD).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71598493-040f-44d3-803b-79f614d14964
📒 Files selected for processing (2)
.github/workflows/xtest.ymlotdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py
Encode the otdfctl module-path policy in one place (config.go_module_for_tag) and drop the `source` field that was being threaded through resolve → version-info JSON → action.yaml. Pre-v0.31.0 tags use the archived github.com/opentdf/otdfctl module path; everything else uses github.com/opentdf/platform/otdfctl. Resolve always tries the platform monorepo first and falls back once to the archive repo. The CI workflow drops the now-meaningless `otdfctl-source` input and its warning step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py (1)
1-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
ruff formatto fix formatting.The pipeline indicates this file needs reformatting. Run
uv run ruff format .from theotdf-sdk-mgrdirectory to resolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py` around lines 1 - 240, This file (installers.py) is misformatted; run the project's formatter (ruff format) over the repository and commit the changes so the file conforms to style checks; specifically reformat installers.py (affecting functions like install_go_release, install_js_release, install_java_release and related top-level definitions such as INSTALLERS, install_release, latest_stable_version) by running the ruff format command used in CI and then add/commit the updated file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py`:
- Around line 1-240: This file (installers.py) is misformatted; run the
project's formatter (ruff format) over the repository and commit the changes so
the file conforms to style checks; specifically reformat installers.py
(affecting functions like install_go_release, install_js_release,
install_java_release and related top-level definitions such as INSTALLERS,
install_release, latest_stable_version) by running the ruff format command used
in CI and then add/commit the updated file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68a19464-4e63-4003-8f76-ed1dcf6cf449
📒 Files selected for processing (8)
.github/workflows/xtest.ymlotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/config.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.pyotdf-sdk-mgr/tests/test_resolve.pyxtest/setup-cli-tool/action.yaml
💤 Files with no reviewable changes (1)
- .github/workflows/xtest.yml
|
1 participant
The archived
opentdf/otdfctlrepo is no longer source-cloned. otdfctl is now sourced exclusively fromopentdf/platform, with pre-v0.31.0 releases falling back to the Go module proxy for artifact install.What changed
Resolution
resolve()for go always queriesopentdf/platformfirst (using theotdfctl/tag infix). Only if the tag isn't there does it fall back once to the archivedopentdf/otdfctlrepo.v0.24.0are looked up asotdfctl/v0.24.0in platform; theotdfctl/prefix is optional.refs/heads/...), SHAs, PR refs (refs/pull/N),main, andlatestalways resolve against platform.Artifact install (
go install)config.go_module_for_tag():github.com/opentdf/otdfctl@<tag>(archived module proxy, still served by proxy.golang.org).github.com/opentdf/platform/otdfctl@<tag>.cli.sh/otdfctl.shread.versionandgo run <module>@<tag>at runtime — no pre-built binary needed for releases.Source builds (head, branches, PRs)
opentdf/platformand symlinkplatform-src/<ref>/otdfctl→src/<ref>.GOWORKby looking for ago.workfile alongside the resolvedsrc/<tag>symlink target. With GOWORK in scope,protocol/go,sdk, andlib/*resolve to the platform checkout's local sources instead of the otdfctlgo.mod's pinned releases. This fixes the failing-PR scenario where one platform PR adds proto symbols and consumes them fromotdfctl/.Version listing
otdf-sdk-mgr versions list gonow only queries the platform repo'sotdfctl/*tags (one network call, not two). Theinstall_methodfield usesgo_module_for_tag()so pre-v0.31.0 entries correctly point at the archived module.CI workflow & action
otdfctl-sourceworkflow input is removed entirely (it wasauto/platform/standalonebefore; routing is now invariant and inferred).setup-cli-tool/action.yamlno longer threads a--sourceflag throughinstall artifact, and the "platform vs standalone" branch in the source-checkout step is gone — for go, source builds always targetopentdf/platform.replace otdfctl go.mod packagesstep that usedgo mod edit -replaceto splice platform modules is removed; GOWORK handles it.Net impact
Compared to
main: +331 / −437 = −106 lines net. Removing support for the archived repo is now a net deletion, not a net addition.Test plan
cd otdf-sdk-mgr && uv run pytest -v— 54/54 passotdf-sdk-mgr versions list go— only platform tags listed; pre-v0.31.0 entries show the archived module path ininstall_methodotdf-sdk-mgr install artifact --sdk go --version v0.24.0 --dist-name v0.24.0— installs via Go module proxy (archive)otdf-sdk-mgr install artifact --sdk go --version otdfctl/v0.31.0 --dist-name v0.31.0— installs via Go module proxy (platform)Summary by CodeRabbit
Documentation
Refactor
Tests