feat(cli): migrate otdfctl into platform monorepo#3205
Conversation
|
Important Review skippedToo many files! This PR contains 298 files, which is 148 over the limit of 150. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (298)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on preparing the repository for the migration of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. A CLI's new home, Files copied, configs set, Migration's begun. Footnotes
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant architectural shift for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Files copied with care, New home, new purpose they bear, CLI now takes flight. Footnotes
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Code Review
This pull request introduces the otdfctl CLI by copying a large number of files into the repository. The changes include Go source code for the CLI commands, Makefiles, documentation, and end-to-end tests. My review focused on the overall structure and patterns in the newly added code. I've identified a few areas for improvement related to consistency in deprecation handling and potential performance issues with client-side pagination. Overall, the code seems well-structured, but these minor issues should be addressed to improve usability and maintainability.
There was a problem hiding this comment.
Code Review
This pull request introduces the otdfctl CLI tool by copying over a large number of files. The changes include the CLI's command structure, handlers, documentation, and end-to-end tests. My review focuses on potential issues in the newly added code. I've identified a bug in the Makefile's version handling, a significant performance issue related to client-side pagination, and a minor maintainability issue with flag parsing. Addressing these will improve the robustness and efficiency of the new CLI tool.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of files by copying the otdfctl CLI tool into the repository. My review focuses on the newly added code, identifying opportunities for improvement in terms of maintainability, correctness, and efficiency. I've pointed out areas with duplicated code that could be refactored, potential performance bottlenecks, and minor issues in test files and configuration. Overall, the changes are substantial and form a good basis for the CLI within this repository.
|
Dismissing all automated comments and alerts since this PR's purpose is to migrate not change app + CI code. |
|
is there an ADR for this change? I'm aware of the benefits but unclear on the tradeoffs of this approach, if any. Downloading the latest otdfctl package is a part of the quickstart guide: https://github.com/opentdf/docs/blob/main/static/quickstart/install.sh#L132. Is it just a matter of changing the location for where to find this, or will the whole build/release process need to change as a subcomponent of platform? |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
01cff81 to
d66bd5b
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
d66bd5b to
fc2fac5
Compare
### Proposed Changes * resolve lint issue in stacked PR branch ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
## Summary This PR tightens the namespacedpolicy planner so registered resources are the only construct that can remain soft-unresolved during planning. All other policy constructs now fail fast when their target namespace or required dependencies cannot be derived. It also cleans up resolver guard behavior so parent resolver functions own nil/source validation, while child helpers focus on namespace and existing-target resolution. For registered resources, unresolved state is now represented internally as a typed object with a machine-checkable reason code plus a human-readable message. In addition, we have merged in changes for interactively handling `Registered Resources` that are unresolved. [RR Interactive](#3317) ## What Changed - Removed top-level Unresolved handling from actions, subject condition sets, subject mappings, and obligation triggers. - Kept registered resources as the only planner construct with soft unresolved behavior. - Added a typed RR unresolved model with: - Reason - Message - Preserved the existing artifact shape by continuing to emit the unresolved message as a string in the finalized plan. - Simplified action, subject condition set, and subject mapping resolver guard logic so invalid derived entries fail fast in the parent resolver. - Removed stale unresolved dependency/result handling from subject mapping dependency resolution. - Skipped registered resources with no AAVs during derivation and documented that behavior inline. ## Why The planner had accumulated multiple soft-unresolved paths that no longer matched the intended contract. This made the resolution flow harder to reason about and made tests rely on string matching for unresolved behavior. This change makes the model stricter and simpler: - non-RR derivation/resolution failures are hard failures - RR namespace conflicts remain the only reviewable unresolved case - tests can now assert a typed unresolved reason instead of brittle strings
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
### Proposed Changes 1.) Add E2E tests for migrating namespaced policy. 2.) Breakup namespaced policy migration tests from original suite invocation, due to parallelization complications and migration logic. 3.) Add the ability to choose whether or not the startup action should provision fixtures or not. Default to true, but require false for otdfctl tests. ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive test suite for namespaced policy migration covering actions, subject-condition-sets, and subject-mappings with idempotency validation. * Improved test execution workflow with staged test execution and aggregated result reporting. * **Chores** * Updated test fixture provisioning to be configurable via workflow input. * Enhanced ignore patterns for credential files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
### Proposed Changes 1.) Reorder the interactive RR review steps. Originally we would modify the resolved actions to include ones that were needed by a RR after manual intervention. This did not account for an edge case where a RR is already created, which would then require the executor and the review code to perform more logic than necessary. Fix this by checking if RR already exists, if so do not modify resolved actions. 2.) Add better unit testing for RR interactive reviews. ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
ttschampel
left a comment
There was a problem hiding this comment.
Approving building on @biscoe916 's previous approval of the main changes + new changes since this approval looking good
🤖 I have created a release *beep* *boop* --- ## [0.14.0](opentdf/platform@service/v0.13.0...service/v0.14.0) (2026-04-21) ### ⚠ BREAKING CHANGES * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ### Features * **authz:** Namespaced policy in decisioning ([opentdf#3226](opentdf#3226)) ([0355934](opentdf@0355934)) * **cli:** migrate otdfctl into platform monorepo ([opentdf#3205](opentdf#3205)) ([5177bec](opentdf@5177bec)) * fix tracing ([opentdf#3242](opentdf#3242)) ([57e5680](opentdf@57e5680)) * **policy:** add GetObligationTrigger RPC ([opentdf#3318](opentdf#3318)) ([d68e39d](opentdf@d68e39d)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) ([bedc9b3](opentdf@bedc9b3)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ([04fd85d](opentdf@04fd85d)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) ([c0443f1](opentdf@c0443f1)) * **policy:** add sort ListSubjectMappings API ([opentdf#3255](opentdf#3255)) ([9d5d757](opentdf@9d5d757)) * **policy:** Add sort support listregisteredresources api ([opentdf#3312](opentdf#3312)) ([91a3ff3](opentdf@91a3ff3)) * **policy:** add sort support to ListAttributes API ([opentdf#3223](opentdf#3223)) ([ec3312f](opentdf@ec3312f)) * **policy:** add sort support to ListKeyAccessServer ([opentdf#3287](opentdf#3287)) ([7fae2d7](opentdf@7fae2d7)) * **policy:** Add sort support to ListNamespaces API ([opentdf#3192](opentdf#3192)) ([aac86cd](opentdf@aac86cd)) * **policy:** add sort support to listobligations api ([opentdf#3300](opentdf#3300)) ([9221cac](opentdf@9221cac)) * **policy:** add sort support to ListSubjectConditionSets API ([opentdf#3272](opentdf#3272)) ([9010f12](opentdf@9010f12)) * **policy:** add SortField proto and update PageRequest for sort support ([opentdf#3187](opentdf#3187)) ([6cf1862](opentdf@6cf1862)) * **policy:** Enforce same namespace when actions referenced downstream ([opentdf#3206](opentdf#3206)) ([4b5463a](opentdf@4b5463a)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) ([6db1883](opentdf@6db1883)) * **policy:** Namespace subject mappings and condition sets ([opentdf#3172](opentdf#3172)) ([6deed50](opentdf@6deed50)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) ([3006780](opentdf@3006780)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) ([8948018](opentdf@8948018)) * **policy:** rollback migration strategy for namespaced actions ([opentdf#3235](opentdf#3235)) ([f7e5e01](opentdf@f7e5e01)) * **policy:** Seed existing namespaces with standard actions ([opentdf#3228](opentdf#3228)) ([12136b0](opentdf@12136b0)) * **policy:** Seed namespaces with standard actions on creation + namespaced actions for obligation triggers ([opentdf#3161](opentdf#3161)) ([984d76b](opentdf@984d76b)) ### Bug Fixes * **ci:** Upgrade toolchain version to 1.25.8 ([opentdf#3116](opentdf#3116)) ([e1b7882](opentdf@e1b7882)) * **core:** do not concat slashes directly in url/file paths ([opentdf#3290](opentdf#3290)) ([114c2a7](opentdf@114c2a7)) * **deps:** bump github.com/jackc/pgx/v5 from 5.7.5 to 5.9.0 in /service ([opentdf#3316](opentdf#3316)) ([017362e](opentdf@017362e)) * **deps:** bump github.com/opentdf/platform/lib/identifier from 0.2.0 to 0.3.0 in /service ([opentdf#3162](opentdf#3162)) ([8bc5dcd](opentdf@8bc5dcd)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.16.0 to 0.17.0 in /service ([opentdf#3125](opentdf#3125)) ([29fec61](opentdf@29fec61)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.17.0 to 0.21.0 in /service ([opentdf#3220](opentdf#3220)) ([e63add2](opentdf@e63add2)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.21.0 to 0.22.0 in /service ([opentdf#3248](opentdf#3248)) ([1ebce73](opentdf@1ebce73)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.22.0 to 0.23.0 in /service ([opentdf#3271](opentdf#3271)) ([3338b8e](opentdf@3338b8e)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.23.0 to 0.24.0 in /service ([opentdf#3321](opentdf#3321)) ([78e6022](opentdf@78e6022)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.24.0 to 0.25.0 in /service ([opentdf#3333](opentdf#3333)) ([3940bf8](opentdf@3940bf8)) * **deps:** bump github.com/opentdf/platform/sdk from 0.13.0 to 0.16.0 in /service ([opentdf#3356](opentdf#3356)) ([5617077](opentdf@5617077)) * **deps:** bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp from 1.42.0 to 1.43.0 in /service ([opentdf#3282](opentdf#3282)) ([046374a](opentdf@046374a)) * **deps:** bump go.opentelemetry.io/otel/sdk from 1.42.0 to 1.43.0 in /service ([opentdf#3281](opentdf#3281)) ([56b33f2](opentdf@56b33f2)) * **deps:** bump google.golang.org/grpc from 1.77.0 to 1.79.3 in /service ([opentdf#3176](opentdf#3176)) ([3289502](opentdf@3289502)) * **deps:** remove direct github.com/docker/docker dependency ([opentdf#3229](opentdf#3229)) ([2becb27](opentdf@2becb27)) * **deps:** upgrade testcontainers-go to resolve vulns ([opentdf#3299](opentdf#3299)) ([72c6f9b](opentdf@72c6f9b)) * **ers:** include standard JWT claims in claims mode entity resolution ([opentdf#3196](opentdf#3196)) ([6d50da1](opentdf@6d50da1)) * **ers:** ldap multi-strategy ers ([opentdf#3117](opentdf#3117)) ([d3aaf1a](opentdf@d3aaf1a)) * **policy:** deprecate ListAttributeValues in favor of existing GetAttribute ([opentdf#3108](opentdf#3108)) ([7e17c2d](opentdf@7e17c2d)) * **policy:** make obligation trigger uniqueness client-aware ([opentdf#3114](opentdf#3114)) ([9265bc3](opentdf@9265bc3)) * **policy:** omit empty attribute values from create responses ([opentdf#3193](opentdf#3193)) ([d298378](opentdf@d298378)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) ([10d0c0f](opentdf@10d0c0f)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) ([c20f039](opentdf@c20f039)) * **policy:** order List* results by created_at ([opentdf#3088](opentdf#3088)) ([ea90ac2](opentdf@ea90ac2)) * **sdk:** normalize issuer URL before OIDC discovery ([opentdf#3261](opentdf#3261)) ([61f98c9](opentdf@61f98c9)) * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) ([f04a385](opentdf@f04a385)) * **sdk:** remove testcontainers from consumer dependency graph ([opentdf#3129](opentdf#3129)) ([f17dcdd](opentdf@f17dcdd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
### Proposed Changes * Add an `otdfctl)` case to `.github/scripts/work-init.sh` so the script no longer fails with `unknown component [otdfctl]` on the release-please branch for otdfctl. The new case mirrors the `examples)` handler — otdfctl is a leaf consumer with no in-workspace dependents, so the partial `go.work` only needs `./otdfctl`. `otdfctl` was migrated into the monorepo in opentdf#3205 and added to the `go` CI matrix and release-please configs, but `work-init.sh` was not updated. The `chore(main): release otdfctl 0.31.0` release-please PR fails every matrix job at the `prevent depending on unreleased upstream changes` step until this is fixed. ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions Local dry-run: ``` GITHUB_HEAD_REF=release-please--branches--main--components--otdfctl \ ./.github/scripts/work-init.sh ``` Expected: exits 0 and writes a `go.work` containing only `./otdfctl`. End-to-end: once merged, the next release-please otdfctl PR should pass the `go` matrix checks (the `prevent depending on unreleased upstream changes` step in `.github/workflows/checks.yaml`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated development environment initialization to support additional component configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [0.31.0](opentdf/platform@otdfctl/v0.30.0...otdfctl/v0.31.0) (2026-04-22) ### Features * **cli:** Add e2e tests and fix bugs. ([opentdf#3353](opentdf#3353)) ([213d843](opentdf@213d843)) * **cli:** Add E2E tests for namespaced policy. ([opentdf#3363](opentdf#3363)) ([e46f07d](opentdf@e46f07d)) * **cli:** Interactive confirmation ([opentdf#3360](opentdf#3360)) ([cd931d9](opentdf@cd931d9)) * **cli:** migrate otdfctl into platform monorepo ([opentdf#3205](opentdf#3205)) ([5177bec](opentdf@5177bec)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/sdk from 0.15.0 to 0.16.0 in /otdfctl ([opentdf#3357](opentdf#3357)) ([d829184](opentdf@d829184)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Krish Suchak <42231639+alkalescent@users.noreply.github.com>
Proposed Changes
opentdf/otdfctlintootdfctl/viagit subtree add, preserving full git history and tagsgo.workworkspaceDSPX-2655: Subtree merge + cleanup
.github/,.golangci.yaml,CONTRIBUTING.md,LICENSE)otdfctl/CHANGELOG.mdfor historical reference.gitignore,CODEOWNERS, pr-checks scopeotdfctl/*prefix (e.g.,otdfctl/v0.26.2)DSPX-2656: Module path rewrite
github.com/opentdf/otdfctl→github.com/opentdf/platform/otdfctlotdfctltogo.workworkspaceDockerfileDSPX-2657: Makefile and build scripts
DSPX-2658: CI matrix
checks.yamlgo job matrixPR Stack (DSPX-2654)
Checklist
Testing Instructions
git log --oneline --follow -M otdfctl/cmd/root.goshows pre-merge historygit tag | grep otdfctl/v0.26confirms tags importedgo build ./otdfctl/...succeeds