Store applied config in stage CR status#280
Conversation
…rEmbeddingsGenerator status Add AppliedConfig field to all three stage status structs so the resolved defaults are visible in the CR status after a successful reconciliation.
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 improves observability by exposing the effective configuration used by the controllers in the status of their respective custom resources. By recording the resolved configuration, users can easily verify the settings applied during reconciliation, particularly when relying on default values. 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 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. 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 adds an AppliedConfig field to the status of ChunksGenerator, DocumentProcessor, and VectorEmbeddingsGenerator to track the applied configuration during reconciliation, along with updating the corresponding controllers, CRD schemas, and deepcopy code. The review feedback correctly points out that AppliedConfig should be defined as a pointer in the API types to allow the omitempty JSON tag to work properly. Additionally, the controllers should use .DeepCopy() instead of a direct assignment to prevent shallow copying of configuration structs that contain reference types.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| LastAppliedGeneration int64 `json:"lastAppliedGeneration,omitempty"` | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| FilesProcessed int64 `json:"filesProcessed,omitempty"` | ||
| AppliedConfig ChunksGeneratorConfig `json:"appliedConfig,omitempty"` |
There was a problem hiding this comment.
Defining AppliedConfig as a struct value instead of a pointer means it cannot be omitted when empty (even with omitempty), because Go's JSON encoder does not omit empty structs unless they are pointers. This results in an empty configuration block (e.g., with empty strings or zero values) being serialized in the status of newly created resources before they are ever reconciled.
Changing this to a pointer (*ChunksGeneratorConfig) allows it to be properly omitted from the status until a successful reconciliation occurs.
| AppliedConfig ChunksGeneratorConfig `json:"appliedConfig,omitempty"` | |
| AppliedConfig *ChunksGeneratorConfig `json:"appliedConfig,omitempty"` |
| Jobs []Job `json:"jobs,omitempty"` | ||
| PermanentlyFailingFiles []string `json:"permanentlyFailingFiles,omitempty"` | ||
| FilesProcessed int64 `json:"filesProcessed,omitempty"` | ||
| AppliedConfig DocumentProcessorConfig `json:"appliedConfig,omitempty"` |
There was a problem hiding this comment.
Defining AppliedConfig as a struct value instead of a pointer means it cannot be omitted when empty (even with omitempty), because Go's JSON encoder does not omit empty structs unless they are pointers. This results in an empty configuration block (e.g., with empty strings or zero values) being serialized in the status of newly created resources before they are ever reconciled.
Changing this to a pointer (*DocumentProcessorConfig) allows it to be properly omitted from the status until a successful reconciliation occurs.
| AppliedConfig DocumentProcessorConfig `json:"appliedConfig,omitempty"` | |
| AppliedConfig *DocumentProcessorConfig `json:"appliedConfig,omitempty"` |
| LastAppliedGeneration int64 `json:"lastAppliedGeneration,omitempty"` | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| FilesProcessed int64 `json:"filesProcessed,omitempty"` | ||
| AppliedConfig VectorEmbeddingsGeneratorConfig `json:"appliedConfig,omitempty"` |
There was a problem hiding this comment.
Defining AppliedConfig as a struct value instead of a pointer means it cannot be omitted when empty (even with omitempty), because Go's JSON encoder does not omit empty structs unless they are pointers. This results in an empty configuration block (e.g., with empty strings or zero values) being serialized in the status of newly created resources before they are ever reconciled.
Changing this to a pointer (*VectorEmbeddingsGeneratorConfig) allows it to be properly omitted from the status until a successful reconciliation occurs.
| AppliedConfig VectorEmbeddingsGeneratorConfig `json:"appliedConfig,omitempty"` | |
| AppliedConfig *VectorEmbeddingsGeneratorConfig `json:"appliedConfig,omitempty"` |
| successMessage := fmt.Sprintf("successfully reconciled chunks generator: %s", chunksGeneratorCR.Name) | ||
| if err := controllerutils.StatusPatch(ctx, r.Client, chunksGeneratorCR, func() { | ||
| chunksGeneratorCR.Status.FilesProcessed += filesProcessed | ||
| chunksGeneratorCR.Status.AppliedConfig = chunksGeneratorCR.Spec.ChunksGeneratorConfig |
There was a problem hiding this comment.
Assigning the configuration directly (chunksGeneratorCR.Status.AppliedConfig = chunksGeneratorCR.Spec.ChunksGeneratorConfig) performs a shallow copy. Since the configuration struct contains reference types (such as slices or pointers), a shallow copy will cause the Status and Spec fields to share the same underlying memory. Any subsequent mutations or updates could lead to unintended side-effects or race conditions.
Using .DeepCopy() (once AppliedConfig is updated to a pointer) ensures a safe, decoupled copy of the configuration is stored in the status.
| chunksGeneratorCR.Status.AppliedConfig = chunksGeneratorCR.Spec.ChunksGeneratorConfig | |
| chunksGeneratorCR.Status.AppliedConfig = chunksGeneratorCR.Spec.ChunksGeneratorConfig.DeepCopy() |
|
|
||
| successMessage := fmt.Sprintf("successfully reconciled document processor: %s", documentProcessorCR.Name) | ||
| if err := controllerutils.StatusPatch(ctx, r.Client, documentProcessorCR, func() { | ||
| documentProcessorCR.Status.AppliedConfig = documentProcessorCR.Spec.DocumentProcessorConfig |
There was a problem hiding this comment.
Assigning the configuration directly (documentProcessorCR.Status.AppliedConfig = documentProcessorCR.Spec.DocumentProcessorConfig) performs a shallow copy. Since the configuration struct contains reference types (such as slices or pointers), a shallow copy will cause the Status and Spec fields to share the same underlying memory. Any subsequent mutations or updates could lead to unintended side-effects or race conditions.
Using .DeepCopy() (once AppliedConfig is updated to a pointer) ensures a safe, decoupled copy of the configuration is stored in the status.
| documentProcessorCR.Status.AppliedConfig = documentProcessorCR.Spec.DocumentProcessorConfig | |
| documentProcessorCR.Status.AppliedConfig = documentProcessorCR.Spec.DocumentProcessorConfig.DeepCopy() |
| successMessage := fmt.Sprintf("successfully reconciled vector embeddings generator: %s", vectorEmbeddingsGeneratorCR.Name) | ||
| if err := controllerutils.StatusPatch(ctx, r.Client, vectorEmbeddingsGeneratorCR, func() { | ||
| vectorEmbeddingsGeneratorCR.Status.FilesProcessed += filesProcessed | ||
| vectorEmbeddingsGeneratorCR.Status.AppliedConfig = vectorEmbeddingsGeneratorCR.Spec.VectorEmbeddingsGeneratorConfig |
There was a problem hiding this comment.
Assigning the configuration directly (vectorEmbeddingsGeneratorCR.Status.AppliedConfig = vectorEmbeddingsGeneratorCR.Spec.VectorEmbeddingsGeneratorConfig) performs a shallow copy. Since the configuration struct contains reference types (such as slices or pointers), a shallow copy will cause the Status and Spec fields to share the same underlying memory. Any subsequent mutations or updates could lead to unintended side-effects or race conditions.
Using .DeepCopy() (once AppliedConfig is updated to a pointer) ensures a safe, decoupled copy of the configuration is stored in the status.
| vectorEmbeddingsGeneratorCR.Status.AppliedConfig = vectorEmbeddingsGeneratorCR.Spec.VectorEmbeddingsGeneratorConfig | |
| vectorEmbeddingsGeneratorCR.Status.AppliedConfig = vectorEmbeddingsGeneratorCR.Spec.VectorEmbeddingsGeneratorConfig.DeepCopy() |
…rEmbeddingsGenerator status Add AppliedConfig pointer field to all three stage status structs so the resolved defaults are visible in the CR status after a successful reconciliation. Use DeepCopy() to avoid shared slice/pointer memory between spec and status.
2 participants
Summary
appliedConfigfield toDocumentProcessorStatus,ChunksGeneratorStatus, andVectorEmbeddingsGeneratorStatusTest plan
make manifests generatepassesmake lintpasses (no new Go lint issues)make testpasses