✨ WIP: add support for receiving kubeconfig in op-con/catd options#2562
✨ WIP: add support for receiving kubeconfig in op-con/catd options#2562grokspawn wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds a --kubeconfig CLI option to let operator-controller and catalogd connect to a non-default Kubernetes API server (e.g., HyperShift scenarios), instead of always using the default controller-runtime config loading behavior.
Changes:
- Add
--kubeconfigflag tooperator-controllerandcatalogd. - When
--kubeconfigis provided, build the REST config from the specified kubeconfig file before creating the controller-runtime manager.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/operator-controller/main.go | Adds --kubeconfig flag and uses it to construct the REST config passed to ctrl.NewManager. |
| cmd/catalogd/main.go | Adds --kubeconfig flag and uses it to construct the REST config passed to ctrl.NewManager. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2562 +/- ##
==========================================
- Coverage 68.65% 64.25% -4.41%
==========================================
Files 131 131
Lines 9333 9352 +19
==========================================
- Hits 6408 6009 -399
- Misses 2436 2864 +428
+ Partials 489 479 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
grokspawn
force-pushed
the
hypershift-adaptation
branch
from
c3f5ed2 to
1d14a74
Compare
grokspawn
force-pushed
the
hypershift-adaptation
branch
from
1d14a74 to
d68417a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| return err | ||
| } | ||
| } else { | ||
| restConfig = ctrl.GetConfigOrDie() |
There was a problem hiding this comment.
run() now has two different failure behaviors for loading cluster config: the kubeconfig path returns an error, but the default path uses ctrl.GetConfigOrDie() which panics instead of returning an error. Since run() already returns error, prefer using a non-panicking config loader and propagate the error so startup failures are handled consistently.
| restConfig = ctrl.GetConfigOrDie() | |
| restConfig, err = ctrl.GetConfig() | |
| if err != nil { | |
| setupLog.Error(err, "unable to load in-cluster configuration") | |
| return err | |
| } |
| return err | ||
| } | ||
| } else { | ||
| restConfig = ctrl.GetConfigOrDie() |
There was a problem hiding this comment.
Similar to operator-controller: the kubeconfig branch returns an error, but the default branch calls ctrl.GetConfigOrDie() which panics on failure. Since run() returns error, prefer consistently returning an error for config-load failures rather than panicking.
| restConfig = ctrl.GetConfigOrDie() | |
| restConfig, err = ctrl.GetConfig() | |
| if err != nil { | |
| setupLog.Error(err, "unable to load in-cluster kubeconfig") | |
| return err | |
| } |
There was a problem hiding this comment.
There is a point that GetConfigOrDie() is no longer necessary since it's not in the NewManager() call
There was a problem hiding this comment.
On the other hand, it's established behavior if using the default path. While I can't foresee that someone would be expecting a panic outcome, it's been a precedent for a long time, and doesn't have to change here. Another alternative is even forcing a panic with an alternative kubeconfig, so that the branching behaviors align again.
Description
Adds the ability for op-con / catd to interact with a non-default apiserver
Reviewer Checklist