simplifies k8s client creation with singleton pattern #84
Conversation
WalkthroughThe changes refactor how Kubernetes clients are managed and used within the codebase. Functions that previously created their own Kubernetes clients internally are now updated to accept an existing client as a parameter. A new function is introduced to handle Kubernetes client creation, allowing a single client instance to be reused across multiple operations. Function signatures are updated accordingly, and internal logic is adjusted to use the passed-in client. Error handling is updated to address client creation failures. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
pkg/common/common.go (1)
225-271:⚠️ Potential issueFix undefined variable in RemoveInstallerEntry
There's a reference to an undefined variable
clientseton line 265. It should be using thek8sclientparameter that was passed to the function.- _, err = clientset.CoreV1().ConfigMaps(namespace).Update(context.TODO(), configMap, metav1.UpdateOptions{}) + _, err = k8sclient.CoreV1().ConfigMaps(namespace).Update(context.TODO(), configMap, metav1.UpdateOptions{})cmd/cluster.go (1)
235-243: 🛠️ Refactor suggestionUse secretName parameter instead of hardcoded value
The function accepts a
secretNameparameter but then ignores it and uses the hardcoded string "parseable-env-secret" in the deletion call. This makes the parameter redundant and potentially confusing.func deleteSecret(namespace, secretName string, k8sClient *kubernetes.Clientset) error { // Delete the secrets - err := k8sClient.CoreV1().Secrets(namespace).Delete(context.TODO(), "parseable-env-secret", metav1.DeleteOptions{}) + err := k8sClient.CoreV1().Secrets(namespace).Delete(context.TODO(), secretName, metav1.DeleteOptions{}) if err != nil { return fmt.Errorf("failed to delete secret '%s': %v", secretName, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cluster.go(5 hunks)pkg/common/common.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/cluster.go (1)
pkg/common/common.go (3)
CreateK8sClient(93-105)ReadInstallerConfigMap(59-85)RemoveInstallerEntry(225-271)
🔇 Additional comments (4)
pkg/common/common.go (2)
93-105: Good implementation of the K8s client creation functionThe
CreateK8sClientfunction effectively centralizes the Kubernetes client creation logic, making it reusable across the codebase. The error handling is comprehensive with clear error messages that will help with debugging.
58-85: Well-refactored function to accept client as parameterGood job refactoring
ReadInstallerConfigMapto accept a K8s client as a parameter instead of creating it internally. This promotes the singleton pattern and improves resource utilization.cmd/cluster.go (2)
194-196: Good use of the singleton patternWell done passing the Kubernetes client to
RemoveInstallerEntry. This is consistent with the singleton pattern being implemented throughout the codebase.
158-161: Good reuse of the Kubernetes clientGood job reusing the created Kubernetes client across multiple operations, which was the main goal of this refactoring.
Signed-off-by: Harisudarsan <[email protected]>
|
@nitisht Please review |
|
Can you sign the CLA? Also the build seems to fail - can you check |
Signed-off-by: Harisudarsan <[email protected]>
Sure |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
@nitisht I'm facing lint errors for already existing code? are you facing it as well? |
|
@nitisht I have created a seperate PR for fixing lint errors please check it out |
K8s client was created multiple time within a single function and created directly from config each time. This PR simplifies the client creation and creates one client per command. It also introduces distinct error regarding client creation which is useful for debugging.
Summary by CodeRabbit