-
Notifications
You must be signed in to change notification settings - Fork 19
fix(policy)!: Make legacy flag optional #2647
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
Conversation
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.
Summary of Changes
Hello @c-r33d, 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 modifies the AsymmetricKey
structure to make the legacy
flag optional. This change involves updating the Protocol Buffer definition to explicitly mark the legacy
field as optional, adjusting the corresponding gRPC documentation, and updating existing Go test cases to correctly handle the legacy
field as a pointer to a boolean, reflecting its new optional nature. The change aims to provide more flexibility for this flag, which is noted as not currently being used by downstream clients.
Highlights
- Protocol Buffer Definition Update: The
legacy
field in theCreateKeyRequest
message withinkey_access_server_registry.proto
has been changed from a requiredbool
to anoptional bool
. - Documentation Update: The gRPC documentation (
docs/grpc/index.html
) has been updated to reflect that thelegacy
field is now optional. - Test Case Adaptations: Existing Go test cases in
service/integration/kas_registry_key_test.go
andservice/policy/kasregistry/key_access_server_registry_keys_test.go
have been updated to use a pointer to a boolean (&legacyTrue
) for theLegacy
field, aligning with its new optional status.
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 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.
Optional field, Now a choice, not a must, Code adapts with grace.
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.
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 makes the legacy
flag in AsymmetricKey
optional. The changes correctly update the protobuf definition, the generated documentation, and the test files to reflect this change. The use of optional
in the proto file and pointers to booleans in the Go test code is appropriate for making the field optional.
My main feedback is on a minor code style point in the tests to improve readability and maintainability by avoiding a new package-level variable. Overall, the changes are well-implemented and achieve the stated goal.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
d9c8d9a
to
c69a054
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
/gemini review |
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 correctly makes the legacy
flag in the AsymmetricKey
message optional. The change is consistently applied across the Protobuf definitions, database layer, tests, and documentation. The implementation is sound and achieves the stated goal. I have one minor suggestion to improve code conciseness in the database access layer.
Closing because we decided it was fine to have the field omitted when false. |
Proposed Changes
1.) Make
legacy
flag optional withinAsymmetricKey
.Doing this because a field of type
bool
with a value offalse
will be omitted from a JSON response, when usingomitempty.
Important
Currently not used within any downstream clients.
Only located within release v0.7 protocol/go
Plan on backporting.
Checklist
Testing Instructions