Config refactor

[drewburr]

Opening up a conversation to rework configuration handling. After working through the minecraft Chart, I feel there is opportunity to simplify things the administrative side without impacting overall functionality. The intention is to make it easier to manage environment variables and reduce the overall boat within the deployment definition.

The path I'm taking here is moving environment variables into a set of configmaps, one for the server and another for mc-backup, then referencing these configmaps using envFrom.configMapRef. The downside to this option is usually that changing the configmap doesn't trigger a restart, but projects like ArgoCD have worked around this by storing the configmap's hash in an annotation.

Another relatively significant change that comes with these updates is that I'm passing environment variables in, regardless of their definition. What I found is that docker-minecraft-server does a very good job of consistently referencing environment variables in ways that treat an unassigned variable identically to the assignment of an empty string.

In contrast, I found that mc-image-helper does not so smoothly handle empty string assignment. In the current implementation, minecraftServer.forcegameMode has a default value of default. When this is set, the environment variable is completely omitted. My suggested changes does not have this logic, so we effectively get FORCE_GAMEMODE="". This translates to server.properties having force-gamemode=. The server does still default this properly, but it does muddle up server.properties in a way that may be undesirable.

There's still a decent amount of testing to be done, but wanted to open up the conversation early in the process to make sure there's agreement in the direction and intention.

[itzg]

I agree with the overall sentiment, but I'm not sure if it's fundamentally solving anything for chart maintenance or usage by shifting env vars to configMap. I do actually prefer configMap usage since it embraces what the kubernetes designers had in mind. It's what I use for my kustomize approach

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

(BTW kustomize solves the challenge you mentioned of a configMap change needing to bump the deployment.)

I have always been more concerned about the tedium of the helm value to image variable mappings, when I'd much prefer users of this chart just focus on the image variables directly. I'm jaded because I put all of my effort into maintaining, enhancing, and documenting the image and I'm really just helping to keep the lights on for this chart. (I don't use the Helm chart personally -- I use the kustomize, above)

Would you be open to setting up a fourth chart here which focuses on the improvements you want without the baggage of the current?

Also, I don't quite follow the problem with the example of forcegameMode. If there's a bug in mc-image-helper, then please offer specific suggestions there to fix it. What you're describing though seems more of a chart-induced problem.

[drewburr]

This is great context, thank you for taking the time to share!

In regards to a 4th chart, I do think that would be a good idea. There is, I'll admit, a bit of awkwardness/inconsistency when it comes to how values are being mixed and translated between the chart values and the image. I like your sentiment towards maintaining the image vs the chart. With this in mind, the new chart can be more permissive to changes at the image level and refer to the documentation site for configuration options, rather than implement every feature directly. A more permissive chart will hopefully make for less maintenance as new features come in.

For the forcegameMode option, I feel this is more a quirk of the chart rather than a bug in mc-image-helper. I was effectively setting export FORCE_GAMEMODE="", which does get applied to server.properties in a way I would expect. Instead, the chart should omit the value entirely, and that's something that can be provided in the new chart.

In terms of next steps, I can look to open a new issue and create a new feature branch to begin work. Do we have any preference in terms of naming? My first thought would be to name it minecraft-server to match the image name, but I'm open to suggestion.

[itzg]

Naming things is the hardest part 😀 Looking at the existing chart names

I'm thinking

a) your suggestion of "minecraft-server" blends with the others quite nicely and like you said better matches the name (and server emphasis) of the image
b) and perhaps that name opens a chance to blend the Java and Bedrock editions support into one chart. Doesn't have to be an initial goal but could plan and design towards it. A few parts get a little tricky like how a different backup image is needed for each flavor, but seems solvable.

[drewburr]

Funnily enough, I had the same thought on point b. I can see what could be done to blend the two together. I agree that this should be kept in mind instead of solved immediately, there would need to be considerable thought to do this properly. I'd rather get a refactor released first just to get the ball rolling.

[drewburr]

Closing in favor of #226