feat/improve-testability #113
Conversation
• Load kubeconfig from file to enhance flexibility • Update manager initialization to use the new configuration Signed-off-by: Bastian Echterhölter <[email protected]> On-behalf-of: @SAP <[email protected]>
• Introduces KCP kubeconfig handling in the config • Simplifies lifecycle manager import in the initializer Signed-off-by: Bastian Echterhölter <[email protected]> On-behalf-of: @SAP <[email protected]>
WalkthroughCommands now load kubeconfigs from a path helper and pass the resulting rest.Config into managers/providers; several controllers switch lifecycle manager usage to the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)internal/controller/apibinding_controller.go (1)
cmd/initializer.go (2)
🔇 Additional comments (4)
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 |
![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
🧹 Nitpick comments (3)
cmd/model_generator.go (1)
64-67: Register APIs on the manager’s scheme (runtimeScheme currently unused).You add types to runtimeScheme but mgrOpts.Scheme uses scheme. Either register onto scheme or assign runtimeScheme to mgrOpts.Scheme.
Apply one of:
- runtimeScheme := runtime.NewScheme() - utilruntime.Must(appsv1.AddToScheme(runtimeScheme)) - utilruntime.Must(securityv1alpha1.AddToScheme(runtimeScheme)) + // Register required APIs on the manager's scheme + utilruntime.Must(appsv1.AddToScheme(scheme)) + utilruntime.Must(securityv1alpha1.AddToScheme(scheme))Additionally remove the now-unused
runtimeimport.cmd/initializer.go (2)
34-39: Prefer returning errors over os.Exit to honor defers.Using os.Exit here skips deferred shutdown/flush. Align with other commands and
returnthe error instead.- if err != nil { - log.Error().Err(err).Msg("unable to get KCP kubeconfig") - os.Exit(1) - } + if err != nil { + log.Error().Err(err).Msg("unable to get KCP kubeconfig") + return err + }
95-99: Clarify error message.It’s not necessarily “in cluster”; prefer “runtime client”.
- log.Error().Err(err).Msg("Failed to create in cluster client") + log.Error().Err(err).Msg("Failed to create runtime client")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/initializer.go(5 hunks)cmd/model_generator.go(2 hunks)cmd/operator.go(2 hunks)cmd/root.go(5 hunks)internal/config/config.go(1 hunks)internal/controller/apibinding_controller.go(3 hunks)internal/controller/authorization_model_controller.go(3 hunks)internal/controller/initializer_controller.go(3 hunks)internal/controller/invite_controller.go(4 hunks)internal/controller/store_controller.go(3 hunks)internal/subroutine/workspace_initializer.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
internal/controller/invite_controller.go (1)
api/v1alpha1/invite_types.go (1)
Invite(23-37)
cmd/initializer.go (1)
internal/controller/initializer_controller.go (1)
NewLogicalClusterReconciler(29-41)
cmd/root.go (1)
internal/config/config.go (1)
Config(11-44)
internal/controller/store_controller.go (1)
api/v1alpha1/store_types.go (1)
Store(44-50)
cmd/model_generator.go (1)
internal/subroutine/invite/subroutine.go (1)
New(45-68)
🔇 Additional comments (7)
cmd/operator.go (1)
128-131: Switch to restCfg for provider/manager: LGTM.Using the KCP-targeted rest.Config for apiexport.New and mcmanager.New matches the PR goal and avoids ctrl.GetConfig globals.
Also applies to: 136-141
internal/controller/authorization_model_controller.go (1)
9-9: Lifecycle manager migration: LGTM.Clean rename to mclifecycle and direct use of multicluster.LifecycleManager. Matches other controllers; no functional risk seen.
Also applies to: 23-24, 29-33, 38-39, 42-43
cmd/model_generator.go (2)
73-76: restCfg wiring into provider/manager: LGTM.Consistent with operator; error handling preserved.
Also applies to: 81-85
34-39: Kubeconfig loader edge-cases:getKubeconfigFromPathreturns an error ifkubeconfigPathis empty or the file can’t be loaded (it does not fall back to~/.kube/configor in-cluster config); exec-based auth plugins are supported byclientcmd.NewDefaultClientConfig.internal/controller/apibinding_controller.go (1)
14-15: Lifecycle manager migration: LGTM.Field/type rename is consistent and scoped. No issues spotted.
Also applies to: 24-28, 33-34, 38-39, 42-43
cmd/initializer.go (2)
64-67: restCfg provider/manager wiring: LGTM.Matches the new pattern; good.
Also applies to: 73-78
105-109: Constructor args: LGTM.Passing runtimeClient into NewLogicalClusterReconciler matches the new signature.
| restCfg, err := getKubeconfigFromPath(operatorCfg.KCP.Kubeconfig) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("unable to get KCP kubeconfig") | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Kubeconfig loader: confirm fallback/edge-cases.
Ensure getKubeconfigFromPath handles empty/missing paths (fallback to in-cluster or KUBECONFIG), and respects auth plugins and exec providers. If not, add fallback to ctrl.GetConfig().
Run to verify presence and usage:
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -C2 '\bfunc\s+getKubeconfigFromPath\s*\('
rg -nP --type=go -C2 'getKubeconfigFromPath\s*\('Length of output: 1357
Support in-cluster and environment kubeconfigs when path is unset
getKubeconfigFromPath currently errors on an empty --kcp-kubeconfig value. Replace that error path with a call to ctrl.GetConfig() so it falls back to in-cluster config or $KUBECONFIG and honors auth plugins/exec providers.
🤖 Prompt for AI Agents
In cmd/operator.go around lines 80-85, the current call to getKubeconfigFromPath
errors when operatorCfg.KCP.Kubeconfig is empty; change the logic so if
operatorCfg.KCP.Kubeconfig is empty you call ctrl.GetConfig() (which falls back
to in-cluster or $KUBECONFIG and supports auth plugins/exec providers) and
otherwise call getKubeconfigFromPath(path); propagate and log any error from
ctrl.GetConfig() or getKubeconfigFromPath as before.
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
Summary by CodeRabbit
New Features
Bug Fixes
Refactor