-
Notifications
You must be signed in to change notification settings - Fork 24
adr(docs): Proto Template ADR #2763
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @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! This pull request introduces an Architectural Decision Record (ADR) that outlines a strategic approach to eliminate value duplication within Protobuf definitions. The document proposes and justifies the adoption of 'Proto Template Generation with Preprocessing' as the preferred method. This solution aims to enhance the maintainability, consistency, and developer experience of Protobuf schemas by centralizing common validation rules and constants, thereby reducing the risk of errors and simplifying future updates. 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. Proto templates rise, Duplication now will cease, Code generation. 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 introduces an Architecture Decision Record (ADR) for using a template-based approach to generate Protobuf files, aiming to reduce value duplication. The ADR is well-structured and detailed. My review includes suggestions to correct a Go code example, clarify a regex pattern, improve a Makefile example, and use more precise terminology for a diagram. I've also noted that one of the considered options lacks a detailed pros and cons analysis, which would make the ADR more complete.
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:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
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:
|
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:
|
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:
|
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:
|
| * 🟥 **Bad**, because increases message size and serialization overhead | ||
| * 🟥 **Bad**, because makes proto definitions harder to read and understand | ||
|
|
||
| ### Option 2: Proto Template Generation with Preprocessing |
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.
Can you please capture an option 3 (common objects like IdFqnIdentifier), which maintains the oneof restriction/validation and is already in use in some places? https://github.com/opentdf/platform/blob/main/service/common/common.proto#L8-L29 This seems like it's probably the cheapest and most maintainable solution?
| ``` | ||
|
|
||
| * 🟩 **Good**, because eliminates all duplication while maintaining readable templates | ||
| * 🟩 **Good**, because generates standard proto files compatible with all existing tools |
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.
Can you please mention how an external publish of the protos to a proto registry like Buf would fit into this? I assume after protos are generated in CI?
| * 🟩 **Good**, because template syntax is intuitive and widely understood | ||
| * 🟨 **Neutral**, because requires build tooling changes and developer training | ||
| * 🟨 **Neutral**, because adds template validation as a new concern | ||
| * 🟥 **Bad**, because introduces preprocessing complexity to the build pipeline |
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.
I think one note here is if you update a proto file and want to consume the gencode locally, you'll also need to have run the preprocessing script. Today the generated Go code is checked in with git, so the validation would need to check that all gencode has no difference between a reprocess from source to gencode and that checked in.
Also, today, other language SDKs or clients can pull *.proto files directly and generate their own connect/gRPC clients, which would go away with this plan if builds/pre-processing are strictly happening in CI.
Proposed Changes
Checklist
Testing Instructions