itzg / minecraft-server-charts

MIT License
260 stars 139 forks source link

Max world size inconsistent between Docker image and Helm default values.yaml #143

Open JJGadgets opened 1 year ago

JJGadgets commented 1 year ago

I am switching my Minecraft from my Docker host to my Kubernetes cluster. On the Docker side, I can't find any defaults for the envvar of the maximum world size, and when /worldborder get is run in-game, it shows what I understand to be the Minecraft default of 5999968, or roughly 60 million. Meanwhile, the Helm Chart's default values.yaml shows that the Helm default is set to maxWorldSize: 10000.

This could cause an imported world to be significantly altered to fit the new constraints, which is undesired. Could the default value for maxWorldSize be changed to the Minecraft default or at least be consistent with the behavior observed when the same image is deployed on Docker?

itzg commented 1 year ago

Oh wow, I didn't notice all of those were preset to non-default values. I inherited this chart so was long before my time. ALL of those values need to be made conditional, which looks like a effort 😡.

I don't even use Helm, so I would recommend either managing the manifest files directly or using the built in kustomize basing off of https://github.com/itzg/docker-minecraft-server/tree/master/kustomize

JJGadgets commented 1 year ago

For me personally I've set this value manually in my HelmRelease (I use FluxCD rather than helm install directly): https://github.com/JJGadgets/Biohazard/blob/main/kube/3-deploy/2-apps/minecraft/3-install.yaml#L35

Would you be open to a PR? I'll see what I can do, as far as I see there are others using this Helm Chart too so it would be beneficial.

itzg commented 1 year ago

Definitely open to a PR. I'm pondering whether

Either way the repetition in deployment.yaml hurts my eyes. With either of the above I'd like a template-macro to be used that is given the values variable and corresponding environment variable name. The conditional can be implemented once in the macro.

Actually, I see you using a lot of extraEnv entries. I'd really prefer it all be done that way since the maintenance of value mappings here in the chart is highly redundant maintenance.

JJGadgets commented 1 year ago

Actually, I see you using a lot of extraEnv entries. I'd really prefer it all be done that way since the maintenance of value mappings here in the chart is highly redundant maintenance.

If envvars are the way forward, and I believe so too since the minecraftServer block simply maps to envvars anyway, perhaps this Chart could take an approach similar to the "app-template" Chart by @bjw-s where the Chart is a generic template that implements values for commonly used components, accepts most images that simply need "bare essentials" such as a Service, Deployment/similar, persistence, ingress etc, and leaves configuration of the service itself to either envvars or a volume mount, which from my understanding is similar to what you're looking for.

This way, the parts of the minecraftServer block that pertains to configuration of the server's behavior itself can be removed in favor of envvars, and the rest of the components that pertain to configuring only the Kubernetes components or other components can adopt a similar style to app-template.

An example of me using app-template with envvar configuration can be found in my repo: https://github.com/JJGadgets/Biohazard/blob/main/kube/3-deploy/2-apps/whoogle/2-install.yaml (I patch the Chart specs from outside this HelmRelease cuz I'm lazy to repeat it and it's homelab :p)

gilesknap commented 1 year ago

@JJGadgets I think I like your suggestion. I have quite a few charts to test your PR against if that helps.

It seems like the proposal is a breaking change for existing charts. Any ideas on how to mitigate that?

itzg commented 1 year ago

I totally agree with you also.

It's the backward compatibility that is now the big challenge. It'll probably take a whole brand new chart to get people migrating to the cleaner baseline.

That's partly why I added first class support for kustomize since it started out clean and kustomize in general encourages clean, natural manifests

https://github.com/itzg/docker-minecraft-server/tree/master/kustomize

billimek commented 1 year ago

If you want to adopt the app-template "chart" from @bjw-s, there is nothing to do beyond deprecate this chart because it would replace this one.

You would probably want to share an example chart configuration to use with 'app-template' to deploy and run minecraft-server, but there is not migration path forward from this chart beyond not using it anymore.

Also keep in mind that you will beholden to whatever changes @bjw-s may make app-template "chart" which may or may not break how folks run the minecraft-server. It's essentially a template of a template for deploying via helm.

itzg commented 1 year ago

Thank you for snapping me back to reality @billimek

Alternate charts are always welcome to be used. That's the beauty of open source.

I think I'll try out the macro idea to allow for null'ing out values that one would want to fallback to server.properties default.