NotificationsYou must be signed in to change notification settings
Fork924
Star10.1k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 3 commits intocoder:mainfromjamezrin:main

Dec 13, 2024

Merged

feat: add support for specifying LoadBalancer class name#15838

johnstcn merged 3 commits intocoder:mainfromjamezrin:main

Dec 13, 2024

Conversation

Copy link

Contributor

jamezrin commentedDec 11, 2024•
edited
Loading

This PR adds support for configuring the loadBalancerClass via the chart values.

feat: add support for specifying LoadBalancer class name

43bcd2a

cdr-botbot added the communityPull Requests and issues created by the community. label

Dec 11, 2024

github-actionsbot assignedjamezrin

Dec 11, 2024

Copy link

github-actionsbot commentedDec 11, 2024•
edited
Loading

All contributors have signed the CLA ✍️ ✅
_{Posted by theCLA Assistant Lite bot.}

Copy link

ContributorAuthor

jamezrin commentedDec 11, 2024

I have read the CLA Document and I hereby sign the CLA

cdrci2 added a commit to coder/cla that referenced this pull request

Dec 11, 2024

@cdr-bot[bot] has signed the CLA incoder/coder#15838

f886744

jamezrin changed the title~~Add support for specifying LoadBalancer class name~~feat: Add support for specifying LoadBalancer class name

Dec 11, 2024

jamezrin changed the title~~feat: Add support for specifying LoadBalancer class name~~feat: add support for specifying LoadBalancer class name

Dec 11, 2024

matifali requested review fromdeansheather andjohnstcn

December 12, 2024 07:04

deansheather reviewed

Dec 12, 2024

View reviewed changes

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: ""

Copy link

Member

deansheatherDec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can this be changed toloadBalancerClass to match the field in the k8s yaml?

Copy link

ContributorAuthor

jamezrinDec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done

johnstcn requested changes

Dec 12, 2024

View reviewed changes

Copy link

Member

johnstcn left a 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 inhelm/coder/tests/testdata/. You can copy an existing golden file and modify it to suit.
Add the corresponding test case inhelm/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!