-
Notifications
You must be signed in to change notification settings - Fork 874
feat: add support for specifying LoadBalancer class name #15838
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
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
helm/coder/values.yaml
Outdated
@@ -281,6 +281,9 @@ coder: | |||
# your cloud and specify it here in production to avoid accidental IP | |||
# address changes. | |||
loadBalancerIP: "" | |||
# coder.service.className -- The class name of the LoadBalancer. See: | |||
# https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-class | |||
className: "" |
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.
Can this be changed to loadBalancerClass
to match the field in the k8s yaml?
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.
Done
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.
Thanks for your contribution!
Before merging, please add some new test cases to cover this new field:
- Add a corresponding values YAML and golden file in
helm/coder/tests/testdata/
. You can copy an existing golden file and modify it to suit. - Add the corresponding test case in
helm/coder/tests/chart_test.go
- Modify the golden file and/or changes until the output is as expected.
I would also suggest modifying the service template such that if new field is not specified, no changes should be made to existing test cases.
I also agree with @deansheather 's suggestion regarding the naming of the new field.
Thanks!
Thanks for reviewing, I made the changes. The tests are passing locally, but I don't think the CI is running them, is that normal? |
It is! I just pushed the button to make them run. |
Merged, thanks @jamezrin ! |
This PR adds support for configuring the loadBalancerClass via the chart values.