[improvement] Allow Loadbalancer implementation export #432
Conversation
…guration management
…y and consistency in naming conventions. Update all references to the new public field names across the implementation and tests.
github-actions
bot
added
the
improvement
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the load balancer implementation to support external exports while improving code structure and adding functionality for load balancers without backend nodes.
- Exports the
Loadbalancersstruct and related constructors to enable external usage - Adds support for empty NodeBalancer backends through a new command-line flag
- Restructures global
Optionsvariable to improve testability and configuration management
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Adds new command-line flag for allowing empty NodeBalancer backends |
| cloud/linode/loadbalancers.go | Major refactoring: exports Loadbalancers struct, adds Options field, implements support for empty backends |
| cloud/linode/service_controller.go | Updates type references to use exported Loadbalancers type |
| cloud/linode/cloud.go | Converts global Options to typed structure and updates constructor calls |
| cloud/linode/loadbalancers_test.go | Updates all test code to use new exported types and constructors |
| cloud/linode/service_controller_test.go | Updates test code to use new exported types |
| cloud/linode/cloud_test.go | Updates test code to use new exported constructor |
| cloud/linode/cilium_loadbalancers_test.go | Updates test instantiation to include Options field |
| cloud/linode/cilium_loadbalancers.go | Updates method receivers to use exported Loadbalancers type |
| ctx context.Context, | ||
| clusterName string, | ||
| service *v1.Service, | ||
| nodes []*v1.Node, | ||
| nb *linodego.NodeBalancer, | ||
| ) (err error) { | ||
| if len(nodes) == 0 { | ||
| if len(nodes) == 0 && !l.Options.AllowEmptyNodeBalancerBackends { |
There was a problem hiding this comment.
The condition should check if AllowEmptyNodeBalancerBackends is false before returning an error, but the logic seems reversed. If AllowEmptyNodeBalancerBackends is true, we should allow empty nodes; if false, we should return an error. The current logic appears correct but could be clearer with explicit boolean comparison.
| if len(nodes) == 0 && !l.Options.AllowEmptyNodeBalancerBackends { | |
| if len(nodes) == 0 && l.Options.AllowEmptyNodeBalancerBackends == false { |
Copilot uses AI. Check for mistakes.
| if len(nodes) == 0 && l.Options.AllowEmptyNodeBalancerBackends { | ||
| klog.Warningf("Creating NodeBalancer for service (%s) without backend nodes - load balancer will be non-functional until nodes are added", getServiceNn(service)) | ||
| } | ||
|
|
There was a problem hiding this comment.
This validation logic is duplicated between updateNodeBalancer and buildLoadBalancerRequest methods. Consider extracting this into a helper method to avoid code duplication and ensure consistent behavior.
| if err := l.validateNodeBalancerNodes(nodes, clusterName, service); err != nil { | |
| return nil, err | |
| } |
Copilot uses AI. Check for mistakes.
| svc.Status.LoadBalancer = *makeLoadBalancerStatus(svc, nb1) | ||
| svcAnn.Status.LoadBalancer = *makeLoadBalancerStatus(svcAnn, nb1) | ||
| lb, assertion := newLoadbalancers(client, region).(*loadbalancers) | ||
| lb, assertion := NewLoadbalancers(client, region).(*Loadbalancers) | ||
| if !assertion { | ||
| t.Error("type assertion failed") |
There was a problem hiding this comment.
The order of operations changed here - NewLoadbalancers is called before makeLoadBalancerStatus, but makeLoadBalancerStatus needs the lb instance to call the method. This could cause issues if the constructor behavior changes.
| t.Error("type assertion failed") | |
| lbIface := NewLoadbalancers(client, region) | |
| lb, assertion := lbIface.(*Loadbalancers) | |
| if !assertion { | |
| t.Error("type assertion failed") | |
| return |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
==========================================
- Coverage 72.44% 72.37% -0.07%
==========================================
Files 19 19
Lines 3498 3511 +13
==========================================
+ Hits 2534 2541 +7
- Misses 733 737 +4
- Partials 231 233 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
General:
Pull Request Guidelines: