Refactor restConfig and client creation #5
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
WalkthroughThe update refactors Kubernetes client configuration by introducing separate variables for kubeconfig path and context, centralizes the client loading logic with a new helper function, and removes the import alias for the controller-runtime client. All relevant function signatures and usages are updated to match these changes, improving consistency and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant ClientConfig
participant K8sClient
User->>Main: Start application
Main->>ClientConfig: loadClientConfig()
ClientConfig-->>Main: clientcmd.ClientConfig
Main->>ClientConfig: restConfig()
ClientConfig-->>Main: *rest.Config or exit on error
Main->>K8sClient: client.New(*rest.Config)
K8sClient-->>Main: client.Client
Main->>Main: Use client.Client for operations
Poem
✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
main.go (1)
556-565: Bug: shadowed variables wipe status historyInside
cmdApply,chartVerandcfgDigestare declared at L556, but re-declared inside theif !plainblock (:=).
Consequently the outer variables remain empty andmarkSuccesssends blank values.- cfgDigest := fluxchartutil.DigestValues(digest.Canonical, vals).String() - chartVer := hr.Spec.Chart.Spec.Version + cfgDigest = fluxchartutil.DigestValues(digest.Canonical, vals).String() + chartVer = hr.Spec.Chart.Spec.VersionFix ensures status and events carry the real revision information.
🧹 Nitpick comments (2)
main.go (2)
134-150:loadClientConfigshould surface errorsThe helper hides all error paths; downstream callers only find out when
ClientConfig()is invoked.
Returning(clientcmd.ClientConfig, error)would:
- Keep side-effects (like
os.Exit) out of helpers.- Allow unit tests to cover “bad kubeconfig” scenarios without needing to intercept
os.Exit.-func loadClientConfig() clientcmd.ClientConfig { +func loadClientConfig() (clientcmd.ClientConfig, error) { ... - return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(rules, overrides) + return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(rules, overrides), nil }All current call-sites already expect an error from
ClientConfig(), so propagating earlier will be mechanical.
745-747: Minor: config lookup repeated many times
restConfig()(or the improved variant returning error) is invoked in every completion/helper.
Consider caching the result once (e.g.sync.Once) to avoid repeated filesystem parses during a single command invocation.Also applies to: 1061-1065, 1085-1091, 1131-1135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.go(21 hunks)
🔇 Additional comments (5)
main.go (5)
46-47: Import-alias removal is a net positiveDropping the
ctrlclientalias and relying on the canonicalclientidentifier reduces cognitive overhead and keeps the code idiomatic.
66-68: Nit: keep CLI flag globals contiguous
kubeconfig/kubecontextare now next to the rest of the global flag vars – good.
No concerns.
99-101: Great – new flags wired into root commandThe addition of
--kubeconfigand--contextcompletes the UX-story started by the new globals.
165-167: Good – Helm inherits the explicit kubeconfigPassing the flag value to
helm.EnvSettingsfixes “cannot find KUBECONFIG” when running against remote clusters.
948-965: Signature updates propagate cleanlyAll helper functions (
markSuccess,markFailure,patchSuspend) now consume the unaliasedclient.Client.
No functional issues spotted.Also applies to: 979-993
| func restConfig() *rest.Config { | ||
| config, err := loadClientConfig().ClientConfig() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error loading kubeconfig: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| return clientcmd.BuildConfigFromFlags("", cfg) | ||
| return config | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid os.Exit in library-style helpers
restConfig() now exits the whole process on failure.
This is fine for CLI execution, but it breaks:
- Cobra shell-completion (the binary gets spawned by the shell and should fail gracefully).
- Unit tests (hard to assert an exit without
exec.Commandindirection).
Prefer returning the error and letting the caller decide when to terminate.
-func restConfig() *rest.Config {
- config, err := loadClientConfig().ClientConfig()
- if err != nil {
- fmt.Fprintf(os.Stderr, "Error loading kubeconfig: %v\n", err)
- os.Exit(1)
- }
- return config
-}
+func restConfig() (*rest.Config, error) {
+ cfg, err := loadClientConfig().ClientConfig()
+ if err != nil {
+ return nil, fmt.Errorf("load kubeconfig: %w", err)
+ }
+ return cfg, nil
+}Most call-sites can adopt cfg, err := restConfig(); if err != nil { … }.
🤖 Prompt for AI Agents
In main.go around lines 153 to 160, the restConfig function currently calls
os.Exit on error, which is inappropriate for library-style helpers as it
prevents graceful failure handling by callers. Modify restConfig to return both
the *rest.Config and an error instead of exiting. Update all call-sites to
handle the error by checking if err is non-nil and deciding how to proceed,
allowing callers to control termination or error handling.
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit