HIVE-3134: Axe the direct dependency on the Machine API operator#2884
HIVE-3134: Axe the direct dependency on the Machine API operator#2884dlom wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dlom: This pull request references HIVE-3134 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (39)
💤 Files with no reviewable changes (35)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoved large sets of vendored vSphere/cloud-config/ipam/gcfg/warnings code and simplified vSphere machinepool actuator by replacing external provider-spec helpers with local helpers and trimming go.mod indirect dependencies. No exported APIs were added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@dlom: This pull request references HIVE-3134 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/machinepool/vsphereactuator.go (2)
156-171: Remove commented-out debug statement.The commented-out
klogline on line 169 should be removed. If debug logging is needed in the future, it can be added back.♻️ Suggested fix
if err := json.Unmarshal(rawExtension.Raw, &spec); err != nil { return nil, fmt.Errorf("error unmarshalling providerSpec: %v", err) } - // klog.V(5).Infof("Got provider spec from raw extension: %+v", spec) return spec, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/machinepool/vsphereactuator.go` around lines 156 - 171, Remove the leftover commented debug statement inside providerSpecFromRawExtension: delete the commented-out klog line ("// klog.V(5).Infof(\"Got provider spec from raw extension: %+v\", spec)") so the function contains only active code and returns; if future debug tracing is needed, add a proper conditional log call back in place using klog within providerSpecFromRawExtension.
119-128: Unusedschemeparameter ingetVSphereOSImage.The
scheme *runtime.Schemeparameter is no longer used since the localproviderSpecFromRawExtensiononly usesjson.Unmarshal. Consider removing it to avoid confusion, though this would require updating the call site inNewVSphereActuator.♻️ Suggested refactor
-func getVSphereOSImage(masterMachine *machineapi.Machine, scheme *runtime.Scheme, logger log.FieldLogger) (string, error) { +func getVSphereOSImage(masterMachine *machineapi.Machine, logger log.FieldLogger) (string, error) {And update the call site in
NewVSphereActuator:- osImage, err := getVSphereOSImage(masterMachine, scheme, logger) + osImage, err := getVSphereOSImage(masterMachine, logger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/machinepool/vsphereactuator.go` around lines 119 - 128, The getVSphereOSImage function has an unused parameter scheme; remove the unused scheme *runtime.Scheme parameter from getVSphereOSImage's signature and update every call site (notably NewVSphereActuator where getVSphereOSImage is invoked) to pass only the masterMachine and logger (and adjust any variable names if necessary); ensure imports and any function declarations match the new two-argument signature and run go build/tests to catch remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/machinepool/vsphereactuator.go`:
- Around line 156-171: Remove the leftover commented debug statement inside
providerSpecFromRawExtension: delete the commented-out klog line ("//
klog.V(5).Infof(\"Got provider spec from raw extension: %+v\", spec)") so the
function contains only active code and returns; if future debug tracing is
needed, add a proper conditional log call back in place using klog within
providerSpecFromRawExtension.
- Around line 119-128: The getVSphereOSImage function has an unused parameter
scheme; remove the unused scheme *runtime.Scheme parameter from
getVSphereOSImage's signature and update every call site (notably
NewVSphereActuator where getVSphereOSImage is invoked) to pass only the
masterMachine and logger (and adjust any variable names if necessary); ensure
imports and any function declarations match the new two-argument signature and
run go build/tests to catch remaining references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0585f76e-fc74-4d12-871f-2e4c26985295
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
go.modpkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/actuator.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/machine_scope.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/reconciler.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/session.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/tag_ids_caching_client.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/util.govendor/github.com/openshift/machine-api-operator/pkg/util/ipam/util.govendor/gopkg.in/gcfg.v1/LICENSEvendor/gopkg.in/gcfg.v1/READMEvendor/gopkg.in/gcfg.v1/doc.govendor/gopkg.in/gcfg.v1/errors.govendor/gopkg.in/gcfg.v1/read.govendor/gopkg.in/gcfg.v1/scanner/errors.govendor/gopkg.in/gcfg.v1/scanner/scanner.govendor/gopkg.in/gcfg.v1/set.govendor/gopkg.in/gcfg.v1/token/position.govendor/gopkg.in/gcfg.v1/token/serialize.govendor/gopkg.in/gcfg.v1/token/token.govendor/gopkg.in/gcfg.v1/types/bool.govendor/gopkg.in/gcfg.v1/types/doc.govendor/gopkg.in/gcfg.v1/types/enum.govendor/gopkg.in/gcfg.v1/types/int.govendor/gopkg.in/gcfg.v1/types/scan.govendor/gopkg.in/warnings.v0/LICENSEvendor/gopkg.in/warnings.v0/READMEvendor/gopkg.in/warnings.v0/warnings.govendor/k8s.io/cloud-provider-vsphere/LICENSEvendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_ini_legacy.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_yaml.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/consts_and_errors.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_common.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_ini_legacy.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_yaml.govendor/modules.txt
💤 Files with no reviewable changes (35)
- vendor/gopkg.in/warnings.v0/LICENSE
- vendor/gopkg.in/gcfg.v1/LICENSE
- vendor/gopkg.in/gcfg.v1/README
- vendor/k8s.io/cloud-provider-vsphere/LICENSE
- vendor/gopkg.in/gcfg.v1/types/doc.go
- vendor/gopkg.in/gcfg.v1/doc.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_ini_legacy.go
- vendor/gopkg.in/warnings.v0/README
- vendor/gopkg.in/gcfg.v1/types/scan.go
- vendor/gopkg.in/gcfg.v1/types/enum.go
- vendor/gopkg.in/gcfg.v1/errors.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_yaml.go
- vendor/gopkg.in/gcfg.v1/token/serialize.go
- vendor/modules.txt
- vendor/gopkg.in/gcfg.v1/read.go
- vendor/gopkg.in/gcfg.v1/types/int.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/consts_and_errors.go
- vendor/gopkg.in/warnings.v0/warnings.go
- vendor/gopkg.in/gcfg.v1/token/token.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_common.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/machine_scope.go
- vendor/gopkg.in/gcfg.v1/types/bool.go
- vendor/gopkg.in/gcfg.v1/scanner/errors.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/session.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/actuator.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_ini_legacy.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_yaml.go
- vendor/gopkg.in/gcfg.v1/set.go
- vendor/gopkg.in/gcfg.v1/scanner/scanner.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/reconciler.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config.go
- vendor/github.com/openshift/machine-api-operator/pkg/util/ipam/util.go
- vendor/gopkg.in/gcfg.v1/token/position.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/util.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/tag_ids_caching_client.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2884 +/- ##
==========================================
- Coverage 50.29% 50.29% -0.01%
==========================================
Files 280 280
Lines 34323 34331 +8
==========================================
+ Hits 17264 17268 +4
- Misses 15698 15700 +2
- Partials 1361 1363 +2
🚀 New features to boost your workflow:
|
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
I would generally not be in favor of copy-pasting something that's available upstream, but these two funcs are trivial, and the decluttering of our vendor dir is worth it.
/lgtm
/hold The commented log line isn't crucial: giving you the option to address it here or later (or not at all).
| return nil, fmt.Errorf("error unmarshalling providerSpec: %v", err) | ||
| } | ||
|
|
||
| // klog.V(5).Infof("Got provider spec from raw extension: %+v", spec) |
There was a problem hiding this comment.
We should either nix this or replace it with our own logger call (which we could conceivably do in the caller's context to avoid having to pass in our logger).
openshift-ci
bot
added
do-not-merge/hold
|
/test e2e-vsphere flake during setup |
Another impact from this (this is actually how I found this case): The MAPI operator also depends on govmomi, and if/when we wanted to upgrade to the latest (installer just did yesterday), we would have also had to wait for the MAPI operator to update their dependency on it (a breaking change). Now, we are completely untied from them govmomi-wise |
dlom
force-pushed
the
HIVE-3134-remove-mapi-op-dep
branch
from
1e889dd to
6e03987
Compare
|
@2uasimojo the flake might be related to this openshift/release#76852 |
|
@dlom: This pull request references HIVE-3134 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
openshift-ci
bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, dlom 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 |
As in, we have to do something to accommodate that change? Like making sure some new piece honors our request to get an extra two IPs? Are you chasing that down? |
|
@dlom: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Revision 6e03987 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
3 participants
xref: HIVE-3134
We're pulling in thousands of lines of code from the MAPI operator just to call two 10-line functions (and only one of those happens at runtime). What if we just didn't do that? It's still remains as an indirect dependency (for now...)
/assign @2uasimojo
Summary by CodeRabbit
Refactor
Chores