Add Units test for config controller#220
Conversation
![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 unit tests for the ControllerConfig reconciler and refactors the reconciliation logic to strictly require the UnstructuredSecret field. Feedback highlights a critical anti-pattern regarding the use of package-level global variables for state management, which prevents parallel testing and compromises thread safety. Additionally, the reviewer suggests moving validation logic to the API level using Kubebuilder markers and recommends adopting structured logging with more accurate terminology in error messages.
| ingestionBucket = "" | ||
| dataStorageBucket = "" | ||
| cacheDirectory = "" |
There was a problem hiding this comment.
The tests rely on package-level global variables (ingestionBucket, dataStorageBucket, cacheDirectory) to manage state. This approach is not thread-safe, as multiple reconciliation workers will race to update these variables. Furthermore, it prevents tests from being run in parallel. These values should be stored as fields within the ControllerConfigReconciler struct or passed as arguments to the functions that need them. Using global variables for state derived from a Custom Resource is a significant anti-pattern in Kubernetes operators and can lead to unpredictable behavior if multiple resources exist.
| }, nil | ||
| } | ||
| if config.Spec.UnstructuredSecret == "" { | ||
| return ctrl.Result{}, errors.New("UnstructuredSecret not configured") |
There was a problem hiding this comment.
Instead of implementing manual checks and complex error handling for the missing UnstructuredSecret within the controller logic, rely on Kubernetes API validation markers. Use +kubebuilder:validation:Required in the CRD definition to enforce this constraint. This aligns with the project's preference for API-level validation over redundant controller-side checks.
References
- Rely on Kubernetes API validation markers (e.g., +kubebuilder:validation:Minimum) to enforce constraints on configuration fields, rather than implementing redundant manual checks within the controller logic.
| } | ||
| if err := r.Get(ctx, | ||
| types.NamespacedName{Name: config.Spec.UnstructuredSecret, Namespace: req.Namespace}, unstructuredSecret); err != nil { | ||
| logger.Error(err, fmt.Sprintf("error fetching AWS secret %s, retrying in 10 seconds ", config.Spec.UnstructuredSecret)) |
There was a problem hiding this comment.
The log message refers to an "AWS secret", but the field is UnstructuredSecret. It is better to use a generic term like "unstructured secret". Also, consider using structured logging by passing the secret name as a key-value pair instead of using fmt.Sprintf in the message, and remove the trailing space in the log string.
| logger.Error(err, fmt.Sprintf("error fetching AWS secret %s, retrying in 10 seconds ", config.Spec.UnstructuredSecret)) | |
| logger.Error(err, "error fetching unstructured secret, retrying in 10 seconds", "secret", config.Spec.UnstructuredSecret) |
1 participant
No description provided.