-
Notifications
You must be signed in to change notification settings - Fork 71
[feat]: Add custom prefix for load balancer #414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
==========================================
+ Coverage 75.39% 75.48% +0.09%
==========================================
Files 16 16
Lines 2983 2994 +11
==========================================
+ Hits 2249 2260 +11
Misses 545 545
Partials 189 189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a customizable prefix for Linode NodeBalancers, replacing the hardcoded “ccm-” prefix.
- Introduces a
--nodebalancer-prefixflag and enforces a maximum length. - Updates the load balancer name generator to use the new prefix.
- Propagates the flag into documentation, Helm chart values, DaemonSet template, and adds tests for length validation.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Add --nodebalancer-prefix flag and default/help text |
| docs/configuration/environment.md | Document the --nodebalancer-prefix flag |
| deploy/chart/values.yaml | Add Helm nodeBalancerPrefix value comment |
| deploy/chart/templates/daemonset.yaml | Inject --nodebalancer-prefix into DaemonSet args |
| cloud/linode/loadbalancers.go | Use Options.NodeBalancerPrefix in name formatting |
| cloud/linode/cloud_test.go | Test prefix length validation |
| cloud/linode/cloud.go | Validate NodeBalancerPrefix length in newCloud |
Comments suppressed due to low confidence (2)
deploy/chart/templates/daemonset.yaml:159
- The condition for rendering
--nodebalancer-prefixis checkingnodeBalancerBackendIPv4Subnetinstead ofnodeBalancerPrefix, so the custom prefix flag may never be included. Change theiftoif .Values.nodeBalancerPrefix.
{{- if .Values.nodeBalancerBackendIPv4Subnet }}
cloud/linode/loadbalancers.go:230
- [nitpick] It would be helpful to add a unit test for
GetLoadBalancerNamethat verifies the custom prefix is applied correctly, including edge cases like an empty string or a very long prefix.
return fmt.Sprintf("%s-%s", Options.NodeBalancerPrefix, unixNano[len(unixNano)-12:])
rahulait
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR introduces a flag that allows you to replace the "ccm-" prefix with any string of up to 20 characters.
This change allows you to set a cluster name as a prefix and trace Node Balancers more easily.
General:
Pull Request Guidelines: