✨ add support for receiving kubeconfig in op-con/catd options#2562
✨ 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. |
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.
grokspawn
changed the title
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Description
Adds the ability for op-con / catd to interact with a non-default apiserver
Reviewer Checklist