DSPX 2655 simplified workflow#438
Conversation
dmihalcik-virtru
commented
Apr 17, 2026
dmihalcik-virtru
commented
- feat(xtest): support platform-embedded otdfctl for migration to monorepo
- fixup ruff format
- fix(xtest): update remaining otdfctl references for platform monorepo migration
- fixup pkg.go removes tag prefixes IIRC
- refactor(xtest): consolidate .version and .module-path into single .version file
- feat(sdk-mgr): wire --source option through install artifact command
- fix(setup-cli-tool): avoid script injection by using env vars for inputs and step outputs
- Apply suggestion from @gemini-code-assist[bot]
- feat(setup-cli-tool): support multiple platform-source Go versions
- fix: address PR review findings for platform otdfctl migration
- fix(sdk-mgr): accept "standalone" as valid Go source in go_module_path
- docs(xtest): clarify auto mode resolves releases from standalone
- fix(setup-cli-tool): require SHA match in auto-detect platform fallback
- fix: harden validation and error handling for platform otdfctl migration
- fixup ruff format
- refactor(xtest): extract composite actions from xtest.yml
- Simplify xtest workflow with local runner
otdfctl is moving from opentdf/otdfctl into opentdf/platform. This updates the test infrastructure to auto-detect when the platform checkout contains otdfctl/ and build from there instead of the standalone repo. Key changes: - xtest.yml: new otdfctl-source input (auto/standalone/platform) and detection step that checks for otdfctl/go.mod in the platform dir - setup-cli-tool: new platform-otdfctl-dir input; symlinks platform source into sdk/go/src/ for head builds instead of separate checkout - otdf-sdk-mgr: resolve.py supports go_source="platform" to resolve against the platform repo with otdfctl/ tag infix; installers write .module-path file for the new Go module path - cli.sh/otdfctl.sh: read .module-path to use the correct module path (github.com/opentdf/platform/otdfctl) for go run fallback Backward compatible: old releases still resolve from the standalone repo; .module-path absence defaults to the original module path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… migration - artifactLink in xtest.yml now uses the correct pkg.go.dev module path based on the source field (platform vs standalone) - registry.py list_go_versions() queries both the standalone otdfctl repo and the platform repo (otdfctl/ prefixed tags), deduplicating with platform entries taking precedence - README.md documents both module paths for Go release installs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ersion file The Go SDK version config now uses a single .version file with format `module-path@version` (e.g., `github.com/opentdf/otdfctl@v0.24.0`) instead of separate .version and .module-path files. Shell wrappers fall back to the standalone module path for legacy bare-version files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allows specifying the source repo for Go CLI installs (e.g., --source platform) to support the otdfctl migration from standalone repo to the platform monorepo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uts and step outputs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Replace global sdk_source/sdk_repo with per-version checkout decisions
so that multiple Go head versions from opentdf/platform at different
SHAs can be tested against each other (e.g. a platform PR vs main).
- Add platform-otdfctl-sha input for SHA-based matching of the existing
platform checkout; versions with a different SHA get a fresh checkout
of opentdf/platform into platform-src/{tag}/ with a symlink from
otdfctl/ into src/{tag}/
- Wire OTDFCTL_SOURCE to the resolve-versions job when otdfctl-source
is explicitly 'platform', so the resolver returns platform SHAs
- Preserve backward-compatible auto-detect behavior: when platform-
otdfctl-dir is set but version-info lacks source:"platform", all
head Go versions still symlink to the existing dir
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Pass --source flag through artifact install step so platform-sourced Go versions get the correct module path in .version files - Strip tag infix (e.g., otdfctl/v0.24.0 → v0.24.0) before normalizing in install_go_release to avoid producing invalid version strings - Move step output expansion to env: block in detect-otdfctl to prevent script injection (consistent with 0c1b428) - Narrow except Exception to GitCommandError in registry.py platform tag query so programming bugs propagate instead of being swallowed - Handle StopIteration explicitly in resolve.py with a clear "main branch not found" error message - Validate source parameter in go_module_path() to reject typos - Add symlink target existence checks after ln -sfn in action.yaml - Add ::warning:: annotation when auto-detect fallback skips SHA verification - Fix typo and minor comment/doc improvements Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The workflow input and CLI advertise "standalone" as a valid source, but go_module_path rejected it with a ValueError. Add "standalone" to the valid set so it maps to the default standalone module path, consistent with go_git_url, go_tag_infix, and go_install_prefix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The otdfctl-source description implied auto detects from the platform checkout, but release resolution runs before that checkout exists. Update the input description and inline comment to accurately reflect the two-phase behavior: releases resolve from standalone, platform detection applies only to head builds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In auto mode, setup-cli-tool previously symlinked all head Go versions to the platform checkout without verifying the commit SHA. This meant otdfctl-ref=my-feature could silently test the platform-embedded otdfctl instead of opentdf/otdfctl@my-feature. Now the fallback only reuses the platform checkout when the resolved SHA matches; otherwise it falls through to a standalone checkout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validate PLATFORM_DIR before using it in detect-otdfctl to prevent wrong-repo SHA when the directory is missing. Remove dead cross-repo SHA comparison in auto-detect fallback (standalone and platform repos have different commit histories). Add input validation to all go_* config helpers and the OTDFCTL_SOURCE env var. Treat pre-warm failure as a hard error for the platform module path. Use array pattern for source_args to prevent shell word-splitting. Improve observability with ::warning:: annotations and version-override logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace ~300 lines of inline workflow steps with three reusable composite actions and two new CLI commands, reducing xtest.yml from 770 to 524 lines. New composite actions: - setup-test-environment: consolidates otdfctl detection, platform version lookup, key management check, root key extraction, and multikas support - setup-sdk-clients: wraps setup-cli-tool with SDK-appropriate caching, go.mod/java .env fixups, and make builds (one call per SDK) - setup-kas-instances: starts all 6 KAS instances via otdf-local ci start-kas New CLI commands: - otdf-sdk-mgr go-fixup: bridges client go.mod to server shared modules for head builds (parallels existing java-fixup) - otdf-local ci start-kas: starts KAS instances in CI and emits GITHUB_OUTPUT lines for log file paths Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 |
|
X-Test Failure Report |
![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 the xtest orchestration system to facilitate local replay of CI tests and improves SDK version management by supporting both standalone and platform-embedded Go otdfctl releases. The changes include new CI-specific commands, enhanced version resolution in otdf-sdk-mgr, and specialized GitHub Actions for test environment preparation. Technical feedback identifies several critical issues, including disconnected configuration fallback logic in the CI module, unsafe substring matching for branch resolution, and potential runtime errors in file operations and shell scripts caused by unsanitized tags or fragile JSON iteration in Bash.
| kas_template = platform_dir / "opentdf-kas-mode.yaml" | ||
| platform_config = platform_dir / "opentdf-dev.yaml" | ||
| if not kas_template.exists(): | ||
| # Fall back to opentdf.yaml if opentdf-kas-mode.yaml doesn't exist | ||
| kas_template_alt = platform_dir / "opentdf.yaml" | ||
| if kas_template_alt.exists(): | ||
| print_info( | ||
| f"Using {kas_template_alt} as KAS template (opentdf-kas-mode.yaml not found)" | ||
| ) | ||
| else: | ||
| print_error( | ||
| f"Neither opentdf-kas-mode.yaml nor opentdf.yaml found in {platform_dir}" | ||
| ) | ||
| raise typer.Exit(1) | ||
|
|
||
| if not platform_config.exists(): | ||
| # Try opentdf.yaml as fallback | ||
| platform_config_alt = platform_dir / "opentdf.yaml" | ||
| if platform_config_alt.exists(): | ||
| platform_config = platform_config_alt | ||
|
|
||
| # Build settings with CI-specific overrides | ||
| # We use a fresh xtest_root derived from this package's location | ||
| settings = Settings( | ||
| platform_dir=platform_dir, | ||
| ) |
There was a problem hiding this comment.
The fallback logic for kas_template and platform_config appears to be disconnected from the rest of the orchestration.
- The
kas_templatevariable (lines 135-148) is checked for existence but never used again in the function. If the intention is to use this template for KAS instances, it should be passed to theSettingsor theKASManager. - The
platform_configvariable (lines 150-154) is updated with a fallback path, but it is not passed to theSettingsconstructor (lines 158-160). If theSettingsclass defaults toopentdf-dev.yaml, it will ignore the fallback found here, causing_prepare_kas_templateto fail or modify the wrong file.
| "tag": "main", | ||
| } | ||
| try: | ||
| sha, _ = next(tag for tag in all_heads if "refs/heads/main" in tag) |
There was a problem hiding this comment.
The substring match "refs/heads/main" in tag is dangerous as it will match any branch containing that string (e.g., refs/heads/main-v2 or refs/heads/feature/main-fix). It should be an exact match to ensure the correct branch is resolved.
| sha, _ = next(tag for tag in all_heads if "refs/heads/main" in tag) | |
| sha, _ = next(tag for tag in all_heads if tag[1] == "refs/heads/main") |
| if sdk == "go" and version.source == "platform": | ||
| if version.sha == platform_sha and (platform_dir / "otdfctl").is_dir(): | ||
| src_dir.parent.mkdir(parents=True, exist_ok=True) | ||
| src_dir.unlink(missing_ok=True) |
There was a problem hiding this comment.
Using unlink() on src_dir will raise an IsADirectoryError if the path exists as a real directory (e.g., from a previous run where git worktree add created a directory instead of a symlink). Since this path is intended to be replaced by a symlink or a worktree, it is safer to use a helper that handles both files/symlinks and directories.
| src_dir.unlink(missing_ok=True) | |
| shutil.rmtree(src_dir, ignore_errors=True) |
| workspace, | ||
| [ | ||
| "--html", | ||
| str(results_dir / f"helper-{config.options.focus_sdk}-{platform_tag}.html"), |
There was a problem hiding this comment.
The platform_tag is used directly in the HTML report filename. If the tag contains slashes (e.g., otdfctl/v0.1.0 or a feature branch name), the pytest command will fail because it will attempt to write to a non-existent subdirectory. You should sanitize the tag before using it in a filename.
| str(results_dir / f"helper-{config.options.focus_sdk}-{platform_tag}.html"), | |
| str(results_dir / f"helper-{config.options.focus_sdk}-{_sanitize_name(platform_tag)}.html"), |
| && contains(fromJSON(inputs.platform-heads), inputs.platform-tag) | ||
| shell: bash | ||
| run: | | ||
| for row in $(echo "$VERSION_INFO" | jq -c '.[]'); do |
There was a problem hiding this comment.
Iterating over JSON objects in a Bash for loop using jq -c is fragile because it relies on word splitting. If any field in the JSON (like a tag or error message) contains a space, the loop will break the object into multiple iterations. It is safer to use a while read loop.
echo "$VERSION_INFO" | jq -c '.[]' | while read -r row; do| echo "version=$PLATFORM_TAG" >> "$GITHUB_OUTPUT" | ||
| exit | ||
| fi | ||
| PLATFORM_VERSION=$(go run ./service version 2>&1) |
There was a problem hiding this comment.
Capturing stderr into the PLATFORM_VERSION variable is risky. If go run emits any warnings (e.g., about the Go version or dependency deprecations), they will be appended to the version string, which will likely cause the subsequent awk semver check (line 126) to fail or produce incorrect results.
PLATFORM_VERSION=$(go run ./service version)
echo "version=$PLATFORM_VERSION" >> "$GITHUB_OUTPUT"1 participant
