Minecraft server envMap values that default to true can't be made false

[ClashTheBunny]

In this line of the envMap snippet, you can see that if a value, like onlineMode, is set to false, it won't even include the variable. The problem is that if ONLINE_MODE defaults to true, it's absense will make it true, even though it's set to false in the helm chart values.

The fix would be to remove the if and always include a variable or you could default everything to false in the minecraft container. I think you would have a big support problem if you defaulted everything to false, so I'd assume you want to just remove the if statement here and always set an env variable.

minecraft-4.5.0...minecraft-4.7.4#diff-6388d0bd6acdc6fb5c4741f9392e7f16a04efd7dcd4ebde73fe24af5db7e4c00R29

[ChipWolf]

Maybe there's a CI thing we can do here to pull the tagged image down if it's changed in values & update the deployment; or just completely delegate configuration to the image instead of duplicating efforts here

[itzg]

just completely delegate configuration to the image instead of duplicating efforts here

That's exactly what I've been trying to push for.

In any case, PRs are very welcome @ClashTheBunny 😀

[itzg]

...in the meantime I'll revert that commit. That was just me trying make helm less annoying.

[ClashTheBunny]

I really like the way you did it, but I do appreciate the revert until it can solve this edge case!

[ChipWolf]

That's exactly what I've been trying to push for.

Without a major chart version bump (breaking change), the only way I can conceive of deprecating .Values.minecraftServer predictably as a minor/patch release would be to:

• add an initContainer that uses the same tagged server image specified in the chart values to automatically migrate the env on start
  • override some of the start scripts to prevent it downloading anything or starting the server
  • after initialisation, run a new script that checks the delta between the initialised env and the configured minecraftServer values (passed to the initContainer as a ConfigMap along with the mappings)
  • dump the resulting exports into a file mounted with an emptyDir to both containers (configure the server container's entrypoint to source this file on start)
  • optionally throw a depreciation warning up in the console (perhaps print a reformatted Helm values.yaml for convenience, potentially including a value to disable the initContainer; if the values have been manually migrated, the overhead of the initContainer for automated migration is no longer necessary).

Tag a major version at some point in the future after this, repoint the docs to the variable config from the image, rip all the complexity out of the chart, winner winner 🍗

The reason I feel this method would be necessary is because the user may choose to use a different container/tag than the default, so there's no guarantee of consistency unless we check the container itself. It might sound convoluted but it could be relatively simple to implement.

Either this, or the deployment remains stuffed with unused variables with no soft migration path to the next major version. I can't [yet] conceive of any method to meet the state @itzg is trying to reach without an intermediary method as above or a harsh breaking change.

[ChipWolf]

If we can get itzg/docker-minecraft-server#1439 somewhere, it would be a neat way to avoid things like this.

Akin to API versions, the config structure could contain the schema version and could be transformed between updates predictably.

Declaring the config through the chart values could be as simple as nesting it under a values map & rendering it into a ConfigMap mounted to the container.
If required, the user could include the config within a generic pack per the FR & the config specified by the chart values could be merged in at runtime if the user has any environment-dependent overrides.

[itzg]

@billimek @gtaylor can one of you step in here and take over? I don't have the time, mental energy, or interest to maintain these charts anymore. (I have never used these charts and don't even like Helm.)

[gtaylor]

Hey @itzg, I don't think I'm interested in maintaining these. It may be time to mark these as archived and move on to other things.

Thanks for carrying them forward as long and well as you did!

[itzg]

Just an update: I am unable to easily revert the commit that introduced this regression since there are changes that followed it. My plan is to remove and disallow any of the boolean properties and require they be declared via extraEnv.

[itzg]

I had an epiphany: in PR #157 I am introducing the concept of a "default" value for the boolean fields. When those fields are "default", they will be left out of the rendered manifest. When the usual true or false, they will be included.

FYI in values.schema.json they are declared as

{
  "anyOf": [
    {"type": "string", "enum": ["default"]}, 
    {"type": "boolean"}
  ]
}