Skip to content

DSPX 2655 gemini simplified workflow#436

Draft
dmihalcik-virtru wants to merge 28 commits intomainfrom
DSPX-2655-gemini-simplified-workflow
Draft

DSPX 2655 gemini simplified workflow#436
dmihalcik-virtru wants to merge 28 commits intomainfrom
DSPX-2655-gemini-simplified-workflow

Conversation

@dmihalcik-virtru
Copy link
Copy Markdown
Member

  • 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
  • fix: add OT_ROOT_KEY env and guard fromJson on empty outputs
  • feat(xtest): unify local and CI matrix orchestration via otdf-local suite

dmihalcik-virtru and others added 18 commits April 15, 2026 16:29
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>
1. Add OT_ROOT_KEY to ABAC test step env — the old km-check step set
   this via $GITHUB_ENV, but setup-test-environment only exposes it as
   an action output.

2. Guard fromJson() in setup-cli-tool checkout steps with fallback
   JSON ('{"sha":""}') to prevent JToken errors during Post cleanup
   when version-b/c/d outputs are empty strings.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uite

This introduces 'otdf-local suite' to manage the full test lifecycle:
- Add 'otdf-local suite generate-shard' to create declarative YAML matrix shards.
- Add 'otdf-local suite run' to orchestrate platform/SDK setup and execution.
- Extend otdf-sdk-mgr to support versioned platform checkouts into isolated worktrees.
- Simplify xtest.yml by replacing complex setup steps with suite orchestration.

These changes allow developers to reproduce any CI matrix job locally by running
the generated shard.yaml from the GHA summary.

Co-Authored-By: Gemini 2.0 Pro <noreply@google.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bc5311e9-53d5-415b-a61e-600549892a37

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DSPX-2655-gemini-simplified-workflow

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

X-Test Failure Report

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an orchestration layer for X-Test suites, adding CLI commands for CI and suite management, Pydantic models for configuration, and a SuiteRunner for automated testing. It also updates otdf-sdk-mgr to support the platform monorepo for Go SDKs and provides new GitHub Actions for environment setup. Key feedback highlights a regression in Pydantic settings initialization, unused configuration discovery logic in the CI module, missing health checks for KAS instances in the test runner, and minor code quality improvements like removing unused imports and bare exception blocks.

Comment on lines +94 to +105
_platform_dir: Path | None = None

@property
def platform_dir(self) -> Path:
"""Platform directory path."""
if self._platform_dir:
return self._platform_dir
return _find_platform_dir(self.xtest_root)

@platform_dir.setter
def platform_dir(self, value: Path) -> None:
self._platform_dir = value
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Changing platform_dir from a Pydantic field to a property with a private backing attribute (_platform_dir) breaks Pydantic's ability to initialize this value via the constructor or environment variables. Pydantic BaseSettings ignores properties during initialization, which will cause Settings(platform_dir=...) calls (like the one in ci.py) to fail or ignore the provided path. To maintain dynamic behavior while allowing overrides, consider keeping it as a field and using a model_validator or model_post_init.

Comment on lines +135 to +154
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The fallback logic for kas_template and platform_config updates local variables that are never used or passed to the Settings object. This means the discovered paths are ignored, and the Settings instance will use its own default paths, potentially leading to errors if the expected files are missing or named differently (e.g., opentdf.yaml vs opentdf-dev.yaml).

if job.requires_kas:
print_info("Starting KAS instances for ABAC tests...")
kas_manager = get_kas_manager(self.settings)
kas_manager.start_all()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The runner starts KAS instances for ABAC tests but does not wait for them to become healthy. This can lead to race conditions where tests start before the services are ready. Consider adding a health check wait loop similar to the one implemented in otdf_local/ci.py.

# Environment variables
env = os.environ.copy()
# Add otdf-local env vars
from otdf_local.cli import env as env_cmd
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The import from otdf_local.cli import env as env_cmd is unused and can be removed.

try:
config = load_yaml(self.settings.platform_config)
return get_nested(config, "services.kas.root_key") or ""
except:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using bare except: blocks as they catch all exceptions, including KeyboardInterrupt and SystemExit. It is better to catch Exception or specific exceptions related to YAML loading.

Suggested change
except:
except Exception:

@github-actions
Copy link
Copy Markdown

X-Test Failure Report

@github-actions
Copy link
Copy Markdown

X-Test Failure Report

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

X-Test Failure Report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant