itzg / minecraft-server-charts

MIT License
279 stars 144 forks source link

Add a configmap for the server.properties #159

Closed fultux closed 1 year ago

fultux commented 1 year ago

Hello, I just tested the deployment of the chart on a Kubernetes instance with persistent storage and from what I've seen it seems that when you restart no modification you do on the values.yaml are reported in the server.properties. From what I've seen, values are stored in environment variables, would that be doable to make the server.properties a configmap in which we would be able to modify values with a helm deployement ?

itzg commented 1 year ago

That's already possible with the envFrom value

https://github.com/itzg/minecraft-server-charts/blob/155c89a656281e68091adb189f5502d23a544df8/charts/minecraft/values.yaml#L356

fultux commented 1 year ago

Isn't it going to have the same behaviour because from what I read from here https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables it will only replace the environment variables and don't change the actual config ? At the moment I'm able to modify the environment variable already by changing the values to an already deployed chart but even if I restart the container no change. So I guess the automation that put the values in the properties only do it at the installation of the chart. I may be totally wrong also.

itzg commented 1 year ago

I'm now confused about what you're attempting and what you're expecting. Can you list out the steps, commands, and values.yaml you're using what didn't happen as expected at each step?

fultux commented 1 year ago

So, I did it through Rancher to test things but it would be the same as doing a helm upgrade --install each time:

itzg commented 1 year ago

Thank you, that cleared up my understanding. I didn't know this was still the default in the chart (since it differs from the image default), but you'll need to set minecraft. overrideServerProperties to true to apply subsequent env changes to the properties file.

I'll also change the default in the baseline values since the discrepancy is misleading.

fultux commented 1 year ago

I did some tests with the solution you provided and It works. Thanks for your help!