fix(core): replace opentdf-public client with cli-client #2422
Conversation
github-actions
bot
added
external-contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @alkalescent, 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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR, titled fix(core): DSPX-611 - replace opentdf-public client with cli-client, addresses ticket DSPX-611 by replacing the usage and definition of the opentdf-public Keycloak client ID with cli-client. This change is reflected in example benchmark code and the Keycloak data used for setup.
Highlights
- Client ID Replacement: The primary change is the replacement of the
opentdf-publicKeycloak client ID withcli-clientacross the codebase where it was referenced. - Keycloak Client Removal: The definition for the now-unused
opentdf-publicclient has been removed from the Keycloak setup data file. - Benchmark Updates: Example benchmark files (
benchmark_decision.goandbenchmark_decision_v2.go) have been updated to use the newcli-clientID.
Changelog
- examples/cmd/benchmark_decision.go
- Updated the
Entity_ClientIdfromopentdf-publictocli-clientin therunDecisionBenchmarkfunction (line 46).
- Updated the
- examples/cmd/benchmark_decision_v2.go
- Updated the
Entity_ClientIdfromopentdf-publictocli-clientin therunDecisionBenchmarkV2function (line 61).
- Updated the
- service/cmd/keycloak_data.yaml
- Removed the entire client definition block for the
opentdf-publicclient (lines 84-93).
- Removed the entire client definition block for the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Old client name goes,
New name arrives, fresh and clean,
Code now knows the way.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request successfully updates the client ID from opentdf-public to cli-client across the specified files, which aligns with the PR title's objective. The changes in the benchmark examples and the removal of the old client definition in the Keycloak data seem correct based on the goal of this change. However, the checklist indicates that unit tests, integration tests, and documentation have not been updated. These are important steps to ensure the examples and the overall system function correctly with the new client ID and that users are properly informed.
Summary of Findings
- Missing Unit Tests: The pull request description indicates that unit tests have not been added or updated. Changes like updating client IDs in examples might require corresponding updates or additions to unit tests to ensure the examples continue to function as expected and that the underlying logic handling these client IDs is still correct. This is a medium severity issue as it impacts the reliability of the code and examples.
- Missing Integration Tests: The pull request description indicates that integration tests have not been added or updated. Integration tests are crucial for verifying that the updated client ID works correctly within the broader system context, especially when interacting with services like Keycloak and the authorization service. This is a medium severity issue as it could lead to unexpected failures in integrated workflows.
- Missing Documentation Updates: The pull request description indicates that documentation has not been added or updated. If the documentation (e.g., READMEs, setup guides, example descriptions) mentions the old
opentdf-publicclient ID, it will now be incorrect and could confuse users. Documentation should be updated to reflect the use of thecli-client. This is a medium severity issue as it impacts the usability and clarity for anyone trying to use the examples or set up the system.
Merge Readiness
The core code changes to replace the client ID are straightforward and appear correct. However, the lack of updated unit tests, integration tests, and documentation, as indicated by the unchecked checklist items, means the pull request is not yet ready to be merged. These missing components are important for verifying the correctness of the change in practice and ensuring the project remains maintainable and user-friendly. I recommend addressing the testing and documentation updates before merging. I am unable to approve this pull request; please ensure other reviewers assess and approve the changes.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Standard Benchmark Metrics Skipped or Failed |
strantalis
changed the title
🤖 I have created a release *beep* *boop* --- ## [0.7.0](service/v0.6.0...service/v0.7.0) (2025-06-24) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ### Features * **authz:** Add caching to keycloak ERS ([#2466](#2466)) ([f5b0a06](f5b0a06)) * **authz:** auth svc registered resource GetDecision support ([#2392](#2392)) ([5405674](5405674)) * **authz:** authz v2 GetBulkDecision ([#2448](#2448)) ([0da3363](0da3363)) * **authz:** cache entitlement policy within authorization service ([#2457](#2457)) ([c16361c](c16361c)) * **authz:** ensure logging parity between authz v2 and v1 ([#2443](#2443)) ([ef68586](ef68586)) * **core:** add cache manager ([#2449](#2449)) ([2b062c5](2b062c5)) * **core:** consume RPC interceptor request context metadata in logging ([#2442](#2442)) ([2769c48](2769c48)) * **core:** DSPX-609 - add cli-client to keycloak provisioning ([#2396](#2396)) ([48e7489](48e7489)) * **core:** ERS cache setup, fix cache initialization ([#2458](#2458)) ([d0c6938](d0c6938)) * inject logger and cache manager to key managers ([#2461](#2461)) ([9292162](9292162)) * **kas:** expose provider config from key details. ([#2459](#2459)) ([0e7d39a](0e7d39a)) * **main:** Add Close() method to cache manager ([#2465](#2465)) ([32630d6](32630d6)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** Restrict deletion of pc with used key. ([#2414](#2414)) ([3b40a46](3b40a46)) * **sdk:** allow Connect-Protocol-Version RPC header for cors ([#2437](#2437)) ([4bf241e](4bf241e)) ### Bug Fixes * **core:** remove generics on new platform cache manager and client ([#2456](#2456)) ([98c3c16](98c3c16)) * **core:** replace opentdf-public client with cli-client ([#2422](#2422)) ([fb18525](fb18525)) * **deps:** bump github.com/casbin/casbin/v2 from 2.106.0 to 2.107.0 in /service in the external group ([#2416](#2416)) ([43afd48](43afd48)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to 0.5.0 in /service ([#2470](#2470)) ([3a73fc9](3a73fc9)) * **deps:** bump github.com/opentdf/platform/sdk from 0.4.7 to 0.5.0 in /service ([#2473](#2473)) ([ad37476](ad37476)) * **deps:** bump the external group across 1 directory with 2 updates ([#2450](#2450)) ([9d8d1f1](9d8d1f1)) * **deps:** bump the external group across 1 directory with 2 updates ([#2472](#2472)) ([d45b3c8](d45b3c8)) * only request a token when near expiration ([#2370](#2370)) ([556d95e](556d95e)) * **policy:** fix casing bug and get provider config on update. ([#2403](#2403)) ([a52b8f9](a52b8f9)) * **policy:** properly formatted pem in test fixtures ([#2409](#2409)) ([54ffd23](54ffd23)) --- 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
Checklist
Testing Instructions