docs(service): add ADR and spec for config annotations#3506
docs(service): add ADR and spec for config annotations#3506jrschumacher wants to merge 1 commit into
Conversation
Signed-off-by: Ryan Schumacher <jschumacher@virtru.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 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 establishes a formal framework for synchronizing platform configuration with deployment artifacts. By introducing a structured annotation layer on Go configuration structs, the changes enable the automated generation of Helm values, JSON schemas, and documentation, significantly reducing the risk of configuration drift between the application source of truth and Kubernetes deployment manifests. Highlights
New Features🧠 You can now enable Memory (public preview) 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. 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 the 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 counterproductive. 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. Go structs hold the truth we seek, With tags to make the future peak. Helm values flow from code so clean, No more drift in the machine. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ADR-0001 and a comprehensive specification for generating Helm deployment artifacts from Go configuration structs using custom annotations. The proposed system aims to eliminate drift between application code and Helm charts by making the Go source the single source of truth. The review feedback focuses on refining the annotation specification, specifically recommending the prohibition of commas in tag values to prevent parsing issues, suggesting pipe-separated lists for aliases to align with Go idioms, adding sync.RWMutex to the list of ignored runtime types, and clarifying the syntax for overriding sensitive field detection.
| * Tokens are trimmed of surrounding whitespace. | ||
| * Empty tokens are ignored. | ||
| * Duplicate keys are invalid unless a key explicitly allows repeated values. | ||
| * Values SHOULD avoid commas. If comma-containing values are needed, move that metadata to a sidecar registry rather than overloading struct tags. |
There was a problem hiding this comment.
The grammar defines tokens as comma-separated (line 52), which means any value containing a comma (e.g., in an example or deprecated message) will be incorrectly split by the parser. The specification should either strictly forbid commas in values or define an escaping mechanism.
| * Values SHOULD avoid commas. If comma-containing values are needed, move that metadata to a sidecar registry rather than overloading struct tags. | |
| * Values MUST NOT contain commas. If comma-containing values are needed, move that metadata to a sidecar registry rather than overloading struct tags. |
| | `sensitive` | boolean | Field contains a secret or credential and MUST NOT be emitted as a literal non-secret Helm value. | | ||
| | `internal` | boolean | Runtime/internal field that is not operator-facing. Implies exclusion from Helm values unless overridden. | | ||
| | `deprecated` | string | Marks the field deprecated. The value SHOULD identify a version, replacement path, or reason. | | ||
| | `alias` | config path | Legacy config path accepted for compatibility. May be repeated if the parser supports repeated keys. | |
There was a problem hiding this comment.
Standard Go struct tag parsing logic typically does not support duplicate keys within a single tag string (e.g., config:"alias=a,alias=b"). It is more idiomatic to use a pipe-separated list for multiple values, similar to the enum key definition.
| | `alias` | config path | Legacy config path accepted for compatibility. May be repeated if the parser supports repeated keys. | | |
| | alias | pipe-separated list | Legacy config paths accepted for compatibility. | |
| * the field is unexported; | ||
| * `mapstructure:"-"` or `json:"-"` is present; | ||
| * `config:"ignore"` is present; | ||
| * the field type is known to be runtime-only, such as `sync.Mutex`, `context.Context`, function types, channels, or embedded filesystem handles. |
There was a problem hiding this comment.
Include sync.RWMutex in the list of known runtime-only types that the generator should automatically skip.
| * the field type is known to be runtime-only, such as `sync.Mutex`, `context.Context`, function types, channels, or embedded filesystem handles. | |
| * the field type is known to be runtime-only, such as sync.Mutex, sync.RWMutex, context.Context, function types, channels, or embedded filesystem handles. |
| * an exported field is unsupported and not explicitly ignored; | ||
| * a `helm.path` is invalid or collides with an incompatible field; | ||
| * `config:"sensitive"` is combined with `helm:"source=configmap"`; | ||
| * a field name matches common secret terms such as `password`, `secret`, `token`, `private`, or `credential` but lacks either `config:"sensitive"` or an explicit non-sensitive override; |
There was a problem hiding this comment.
The specification mentions an "explicit non-sensitive override" to satisfy lint rules for fields with sensitive-looking names. Since boolean flags without an equals sign are treated as true (line 60), the spec should explicitly define sensitive=false as the intended syntax for this override.
1 participant
Proposed Changes
service/pkg/config/covering tags, intermediate schema, Helm projection rules, validation, and portabilityservice/adr/README.mdindex for platform-service ADRsChecklist
Testing Instructions
Documentation-only change; no tests were run.