Skip to content

Allow configuring labels, annotations, and ports for the multiplayer Service #222

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.

Sign up for GitHub

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

Merged
jjlgao merged 4 commits into from
Jun 17, 2025
Merged

Allow configuring labels, annotations, and ports for the multiplayer Service #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a939ffc
configure labels, annotations, ports config for multiplayer Service
abooij Mar 12, 2025
3a9b91f
copy changes to the other values.yaml
abooij Apr 9, 2025
484704d
push all changes to multiplayer subvalues
jjlgao May 9, 2025
68a6e9a
version bump
jjlgao May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/retool/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v2
name: retool
description: A Helm chart for Kubernetes
type: application
version: 6.4.12
version: 6.4.13
maintainers:
- name: Retool Engineering
email: [email protected]
Expand Down
23 changes: 23 additions & 0 deletions charts/retool/templates/deployment_multiplayer_ws.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,37 @@ apiVersion: v1
kind: Service
metadata:
name: {{ template "retool.multiplayer.name" . }}
labels:
{{- include "retool.labels" . | nindent 4 }}
{{- if .Values.multiplayer.service.labels }}
{{- range $key, $value := .Values.multiplayer.service.labels }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
{{- if .Values.multiplayer.service.annotations }}
{{- with .Values.multiplayer.service.annotations }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
{{- end }}
spec:
selector:
retoolService: {{ template "retool.multiplayer.name" . }}
ports:
- name: http-server
protocol: TCP
{{- if .Values.multiplayer.service.externalPort }}
Copy link
Contributor

@jjlgao jjlgao Apr 9, 2025

Choose a reason for hiding this comment

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

There's no need for an if/else here since you defined a default value for externalPort in values.yaml! Feel free to just have the port: {{ .Values.service.externalPort }} here.

Copy link
Contributor Author

@abooij abooij Apr 9, 2025

Choose a reason for hiding this comment

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

With this if/else, it's backwards compatible in case people don't have the externalPort in their current values.yaml. If we don't have it, then if people quietly upgrade their chart without adding to values.yaml, things would likely break.

Copy link
Contributor

@jjlgao jjlgao May 9, 2025

Choose a reason for hiding this comment

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

Ah, thanks for the explanation!

Apologies for being unclear, since you set a default value in values.yaml, it should carry over if they did not define it. I thought our default values.yaml would still act as the final fallback set of values in the event that someone didn't have a value set, but I'm hazy now and don't remember for sure.

Since you're leaving this check, I did notice that you've used .Values.multiplayer.service.externalPort as the if condition, but .Values.service.externalPort as the actual value being set. Should they match?

Copy link
Contributor Author

@abooij abooij May 11, 2025

Choose a reason for hiding this comment

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

Oh yeah that was a mistake, it should've all referred to the multiplayer part of the config. Thanks for fixing it directly.

port: {{ .Values.multiplayer.service.externalPort }}
{{- else }}
port: 80
{{- end }}
{{- if .Values.multiplayer.service.internalPort }}
targetPort: {{ .Values.multiplayer.service.internalPort }}
{{- else }}
targetPort: 3001
{{- end }}
---
apiVersion: apps/v1
kind: Deployment
Expand Down
6 changes: 6 additions & 0 deletions charts/retool/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,12 @@ multiplayer:
- path: /api/multiplayer
port: 80

service:
externalPort: 80
internalPort: 3001
annotations: {}
labels: {}

codeExecutor:
# enabled by default from Chart version 6.0.15 and Retool image 3.20.15 onwards
# explicitly set other fields as needed
Expand Down
6 changes: 6 additions & 0 deletions values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,12 @@ multiplayer:
- path: /api/multiplayer
port: 80

service:
externalPort: 80
internalPort: 3001
annotations: {}
labels: {}

codeExecutor:
# enabled by default from Chart version 6.0.15 and Retool image 3.20.15 onwards
# explicitly set other fields as needed
Expand Down
Loading