[DSPX-3302] (2/5) Manage platform service + install scenario in otdf-sdk-mgr#451
[DSPX-3302] (2/5) Manage platform service + install scenario in otdf-sdk-mgr#451dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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 introduces support for installing the OpenTDF platform service and scenario-driven installations. Key changes include the addition of a platform_installer module that manages source builds via git worktrees, a new cli_scenario module for manifest-based installs, and updates to existing CLI commands to handle the "platform" target. Review feedback highlights a bug in updating git worktrees from bare repositories, identifies code duplication in the CLI logic, suggests optimizing YAML parsing to avoid redundant reads, and recommends allowing real-time output for long-running build processes to improve user experience.
| _run(["git", f"--git-dir={bare}", "worktree", "add", str(worktree), branch]) | ||
| else: | ||
| print(f"Updating scripts worktree at {worktree}...") | ||
| _run(["git", "-C", str(worktree), "pull", "origin", branch]) |
There was a problem hiding this comment.
A git worktree added from a bare repository typically does not have remotes configured in its local config. Running git pull origin branch inside the worktree will likely fail with an error that 'origin' is not a git repository. Since _ensure_bare_repo() already fetches updates into the bare repository, you should update the worktree by resetting it to the tracking branch available in the bare repo.
| _run(["git", "-C", str(worktree), "pull", "origin", branch]) | |
| _run(["git", "-C", str(worktree), "reset", "--hard", f"origin/{branch}"]) |
| requested = sdks or ALL_SDKS | ||
| sdk_targets = [s for s in requested if s != "platform"] | ||
| if "platform" in requested: | ||
| version = LTS_VERSIONS.get("platform") | ||
| if version is None: | ||
| typer.echo("Warning: no LTS version defined for platform; skipping", err=True) | ||
| else: | ||
| try: | ||
| install_platform_release(version) | ||
| except PlatformInstallError as e: | ||
| typer.echo(f"Error: {e}", err=True) | ||
| raise typer.Exit(1) | ||
| if sdk_targets: | ||
| cmd_lts(sdk_targets) |
There was a problem hiding this comment.
The logic for handling the platform target is duplicated here and in the tip command (lines 82-91). This pattern makes the CLI code harder to maintain. Consider refactoring this into a shared helper function or extending the cmd_lts/cmd_tip functions to handle the platform service internally, similar to how other SDKs are handled.
| raw_kind = _peek_kind(path) | ||
| scenario: Scenario | None = None | ||
| if raw_kind == "Scenario": | ||
| scenario = load_scenario(path) | ||
| instance = scenario.instance | ||
| elif raw_kind == "Instance": | ||
| instance = load_instance(path) | ||
| else: | ||
| typer.echo(f"Error: {path} has unknown kind {raw_kind!r}", err=True) | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
The manifest file is being parsed twice: first in _peek_kind to determine the kind, and then again in load_scenario or load_instance. For better performance and cleaner code, you should parse the YAML once and then use the resulting dictionary to decide which Pydantic model to validate against.
|
|
||
| def _run(cmd: list[str], cwd: Path | None = None) -> None: | ||
| """Run a command, raising PlatformInstallError on failure.""" | ||
| result = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True) |
There was a problem hiding this comment.
Using capture_output=True for long-running operations like go build or git clone prevents real-time feedback in the terminal. The CLI will appear to be frozen until the command finishes. It is generally better to allow these commands to stream their output to the console so the user can monitor progress.
X-Test Results✅ go-v0.15.0 |
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
c6a7895 to
ebc0c15
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
ebc0c15 to
14e5c1e
Compare
|
1 participant
Summary
Second PR in the five-part stack. Promotes the OpenTDF platform service to a first-class managed package in
otdf-sdk-mgr, mirroring the existing Go/Java/JS SDK CLI flow, and adds a one-shotinstall scenarioentry point.platform_installer.py: resolvesv0.9.0to theservice/v0.9.0tag in theopentdf/platformmonorepo, creates a git worktree, and runsgo build -o xtest/platform/dist/<version>/service ./service.install_helper_scripts(main): mirrorsplatform/scripts/intoxtest/platform/scripts/. Helper scripts are shared across instances and refreshed on demand.otdf-sdk-mgr install release platform:<version>(alongsidego:,js:,java:)otdf-sdk-mgr install lts platform/install tip platformotdf-sdk-mgr install scriptsotdf-sdk-mgr install scenario <path>— installs platform pin + per-KAS pins + encrypt/decrypt SDK union from a scenarios.yaml or instance.yaml, writes<path>.installed.jsonStack
Test plan
cd otdf-sdk-mgr && uv run pytest tests/→ 57 passing (existing 51 + new 6 from PR 1)uv run otdf-sdk-mgr install --helpshows the newscenarioandscriptscommandsJira: https://virtru.atlassian.net/browse/DSPX-3302
🤖 Generated with Claude Code