lloesche / valheim-server-docker

Valheim dedicated gameserver with automatic update, World backup, BepInEx and ValheimPlus mod support
https://hub.docker.com/r/lloesche/valheim-server
Apache License 2.0
1.94k stars 271 forks source link

How should Valheim Plus config updates be handled? #158

Closed kachunkachunk closed 3 years ago

kachunkachunk commented 3 years ago

[edit: This is not so much an issue/bug, but a question and suggestion]

When Valheim Plus updates, it often comes with configuration changes, and a new config file. Still, I've been wondering - how does the update checker service handle the new config file that's included with each updated V+ release that's obtained and installed?

I assume that it doesn't overwrite the config file that's already generated or used when the container is instantiated, so as V+ updates and the server restarts, the old configuration file continues to load. Thus you don't use the new sections or key=value properties, post-restart. If my assumption is correct, that's probably fine (and sane), since V+ will assume defaults for any undefined config file properties. If that's incorrect, and the new defaults are loaded instead, it may pose issues for users and I'd have to suggest avoiding that. For instance, maybe the max item stack size decreased from the user value to defaults, and thus all their item stacks will truncate.

I've otherwise been thinking that the environment variable config generation is a pretty smart way of making sure some things are persistent across versions (assuming key=value pairs don't change drastically). At least on container creation. Maybe I can suggest that you take the new V+ config file (included with each update and populated with defaults), read in the container environment variables or old config file, and merge the key=value pairs into the new config? It's easier said than done, though; I have not yet figured out a good process to get one config's set into another config (without depending on sorting, expecting the same sections or line numbers, etc). I've tried and did not really come up with a simple solution to this with shell scripting. : /

But, in case it helps, I have written this shell script that will read in a config file (default or customized) and it into a list of environment variables that you can load with docker run. You're free to do whatever you want with it, if you find it or any part of it helpful (note that the link/branch may be invalid in some days/weeks). I'm unsure yet if I need to rely on environment variables, or config files, so would be nice to know what your thoughts are on everything concerning updates vs configs.

lloesche commented 3 years ago

@kachunkachunk great question! Here is how it currently works:

When downloading/updating V+ if the config folder /config/valheimplus/ does not exist it is created and default configs (valheim_plus.cfg and BepInEx.cfg) copied from the ZIP archive. If the folder exists files in it are not touched.

If a user defines sections using the VPCFG_ prefixed env variables then only those are written into the config file. However if an existing config file is found it is read in first and then the env config values are merged on top of the existing config. A new config file is written and the old one moved to the .old extension.

By default all sections are disabled so for the mod itself there is no upside in copying a new config as any potentially new entries would be disabled regardless.

The existing code already has a config = get_config(config_file) function that we could use to first read one config file and then another and merge the two configs with a single update() call. That way we could ensure new sections get added to the config. But it'd be for the sole purpose of the user being able to look at them/know about them. They would not serve any technical purpose as they are disabled by default.

I don't think I would want to turn the entire large config into env variables though (I'm not sure that was your plan with the script). A lot of users might only want to have a handful of settings changed from their defaults.

Maybe instead of trying to make assumptions or build clever merging strategies the sane thing to do could be, whenever we download a new V+ update we take its default config file and copy it to /config/valheimplus/valheim_plus.cfg.default

That way it does not interfere with anything the user has configured but if they are interested in looking at the defaults and learn about new config options it makes it easy for them. What do you think?

kachunkachunk commented 3 years ago

Thanks for the explanation and details! Let me know if I've misunderstood anything (I'm not a programmer, sadly), but reiterating for my own understanding:

  1. If /config/valheimplus/valheim_plus.cfg already exists, it is not overwritten upon installing or updating V+, and as a result, can become out-of-date (missing new parameters), over time.
  2. If you use VPCFG environment variables, on first install/update, the default configuration file is copied from the archive, and the VPCFG environment variables are merged into the installed file. However, subsequent V+ updates may still result in an out-of-date V+ configuration file (missing new parameters), over time. Edit: Not sure what happens if you delete the existing file. On each update, I think a new file is produced, updated with the VPCFG environment variables' contents (that's awesome). What about if the file is deleted before restarting the container? Is it missing until an update?

On that note, "out-of-date" sounds more consequential than it really is. New parameters remain disabled by default, that is true. But indeed, users may miss seeing what the latest available options might be, which also poses supportability issues on the V+ side (why am I not seeing X or Y settings?).

So, assuming I did understand everything correctly so far, your description (or proposal?) of the update() call sounds like a good way to address this issue: Take a new default config file, and merge user settings into it (make a copy of it for reference, as you proposed, too).

By the way, ~tomorrow, we should see another V+ release, and like all releases:

These kinds of variables basically just melted my mind as I imagine trying to figure out a way to merge two files with so many differences, but I am hopeful you have a good process/function down (and generally agree with my line of thinking). On top of that, the upcoming release refactored or redesigned some parameters, so instead of flat rate +/- values to some things (like Stamina consumption), these have turned into % modifiers. Such logic changes will be rare, but it's something I guess we can call out.

Overall, if you had to do any kind of work on this, it's best waiting until after these changes release (~tomorrow). From thereon, I would hope that the config should see a lot less drastic volatility, going forward.

In summary, I suppose this is now a feature request that:

  1. Unzips a new V+ default config file as valheim_plus.cfg.default
  2. Renames the existing user valheim_plus.cfg file as valheim_plus.cfg.old
  3. Merges the valheim_plus.cfg.old and valheim_plus.cfg.default files, producing an updated/merged valheim_plus.cfg, containing all current parameters - even if the new ones are disabled by default. This all ensures that a user can see, and conveniently enable, new preferences.
lloesche commented 3 years ago

If /config/valheimplus/valheim_plus.cfg already exists, it is not overwritten upon installing or updating V+

Not quite. If /config/valheimplus/ already exists the installer will assume you know what you're doing and won't touch the contents of that directory. It does not matter whether there's config files in it or not. Only if that directory is absent (so typically on 1st install) will the default config directory be copied from the archive.

Not sure what happens if you delete the existing file.

The values in VPCFG env variables will always be written to the config file. On each container start and on updates. If you delete the existing config file then it's content can't be merged and all that'll be in the generated config file are the values you specified via VPCFG env variables.

The general idea is that you either configure everything via VPCFG env vars or everything in the config file. Maybe I should remove the whole merging function to make that more explicit.