-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add support for provider manager column #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for provider manager column #660
Conversation
Summary of ChangesHello @strantalis, 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 enhances key management provider configurations by introducing a new 'manager' attribute. This change is propagated through the command-line interface, allowing users to specify a manager during creation and update operations, and is supported by corresponding modifications in the backend handler logic and updated Go module dependencies. The addition aims to provide more granular control and metadata for provider configurations. Highlights
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. 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. Footnotes
|
There was a problem hiding this 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 adds support for a manager field to the key management provider configuration. The core logic changes are sound, but there are a few areas that need attention. The go.mod file includes replace directives that seem intended for local development and should be removed to avoid breaking builds. For a complete user experience, the new manager field should be added to the output tables of the create, update, get, and list commands. Additionally, the documentation examples for the create and update commands should be updated to include the new --manager flag.
dcbfa66 to
067e94e
Compare
c-r33d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add bats tests to provider_config.bats
I'm surprised the tests are passing with a new required flag not being passed in by some of the BATS tests? It looks like the job checked out the most recent commit to this PR, but surprising. 🤔 |
jakedoublev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given feedback that we don't have an e2e manager/provider combo in opentdf/platform, we cannot test this e2e
🤖 I have created a release *beep* *boop* --- ## [0.25.0](v0.24.0...v0.25.0) (2025-10-06) ### Features * add support for provider manager column ([#660](#660)) ([fe4e50b](fe4e50b)) * **core:** Add legacy flag to import and list. ([#641](#641)) ([ffd0dc0](ffd0dc0)) * **core:** Add obligation triggers ([#656](#656)) ([8f6087f](8f6087f)) * **core:** Adds policy-mode encrypt param ([#633](#633)) ([9e83016](9e83016)) * **core:** Create/Update triggers via obligation values. ([#658](#658)) ([2a2f0c6](2a2f0c6)) * **core:** obligations defs + vals CRUD ([#639](#639)) ([3a3df0d](3a3df0d)) ### Bug Fixes * **core:** add missing port flag ([#638](#638)) ([c9bb4e5](c9bb4e5)) * **core:** Clarifies not_found in attrs ([#649](#649)) ([d46bd0f](d46bd0f)) * **core:** render kas-registry key list-mappings table rows ([#663](#663)) ([fb39718](fb39718)) --- 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: Jake Van Vorhis <[email protected]>
Wires through the provider manager feature added in opentdf/platform#2601