Improve the way Sponge configuration files are loaded/saved by lucko · Pull Request #1957 · SpongePowered/Sponge

lucko · 2018-06-26T12:25:03Z

World and dimension configurations are now merged with their "parent" config on load
Each value within a config file is not written to the file if the setting is also present in the parent config with the same value
The 'config-enabled' property is no longer needed, values from world/dimension configs will always override.

phit · 2018-06-26T14:39:50Z

this is gonna break everything for existing configs on API7 as config-enabled is not taken into account during migration, nor if config-enabled even did anything for that config

you gonna end up with hundreds of global settings being overwritten for no reason with things in the world/dim configs that were never active to begin with

dualspiral · 2018-06-26T14:56:15Z

Each value within a config file is not written to the file if the setting is also present in the parent config with the same value

I don't like this if it's going to remove entries from the world/dim confs. I think that if something is specified in a world config by a user, it should stay that way.

If you set an option to A in the global conf and B in the world conf, fine. You might later then change the global conf to B, that does not mean that the world conf entry should be removed. You might temporarily change an entry in the global conf to test something, only to change it back 30 minutes later.

By adding an entry to the world files, you are making an explict choice, we should not be removing that explict choice. I agree with not adding to the file, but we should not remove from the file.

lucko · 2018-06-26T15:16:55Z

this is gonna break everything for existing configs on API7 as config-enabled is not taken into account during migration, nor if config-enabled even did anything for that config

Yep you're right, I had already thought of this and will account for it in the migration.

I agree with not adding to the file, but we should not remove from the file.

I can't think of a way to implement this so we can have both.

The reason for stripping values out of the config is that it means existing configurations, saved by the old system, will be cleaned up. Perhaps we could make it a one-time thing though?

dualspiral · 2018-06-26T15:24:32Z

The reason for stripping values out of the config is that it means existing configurations, saved by the old system, will be cleaned up.

The problem is then we don't know which options should have been explicitly kept. It's easy if we have a config-enabled set to false, because you can wipe the file - and you can base it on the config-enabled setting. If true, I don't think we should touch it.

Maybe add a /sponge config -w/-d cleanup equse command that allows for it to be done manually? I imagine that we're gonna have to publicise this when it's done anyway, so we could advertise a manual step then.

There is no ideal solution, I agree, but I think we need to err on the side of caution when removing anything.

phit · 2018-06-26T15:46:48Z

It's easy if we have a config-enabled set to false, because you can wipe the file

not even, some configs are used even when the config is disabled

dualspiral · 2018-06-26T16:04:14Z

Well, that’s that then!

gabizou · 2018-06-26T17:19:18Z

not even, some configs are used even when the config is disabled

~~That line only sets the default value, which then pulls the actual value from the config, whether it's enabled or not. It's not just overriding and setting it to true.~~

Edit: Ignore me.

bloodmc · 2018-06-27T17:14:40Z

After further discussion on this issue. Here is what should happen

If config-enabled is false, clear it.
If config-enabled is true, compare key/value and ONLY save if different from parents. This will ONLY be done on first pass (initial migration) so we can clean up all the configs.
After migration, anything added to Dim/World configs will be saved even if they are identical to parents.
Add a cleanup argument to sponge config command to allow admins to force a cleanup where World/Dim configs will have their key/value compared with parents and cleared if same.

lucko · 2018-06-27T20:50:23Z

I've implemented the latest set of proposed changes.

ryantheleach · 2018-06-28T01:15:16Z

I have to ask that this please be introduced on either a major version change, or at minimum a minor version change. e.g. 7.1 or 7.2.

The docs will need to be versioned in order to migrate cleanly.

phit · 2018-06-28T04:03:38Z

@ryantheleach the format doesn't change, and this doesn't affect api at all
it will only change what is saved inside the world and dimension config and remove duplicate values that are stored in parents

just looked over the docs at https://docs.spongepowered.org/stable/en/server/getting-started/configuration/sponge-conf.html
theres nothing that needs to be changed there as it doesnt explain how overriding works at all
we can simply add a note to config-enabled in there that it is no longer necessary in versions newer than X

heck that page is already outdated because newer configs were added recently

dualspiral · 2018-06-28T10:02:04Z

I have to ask that this please be introduced on either a major version change, or at minimum a minor version change. e.g. 7.1 or 7.2.

I feel like I need to clear something up with versioning here so that anyone else wondering the same thing can see what the thought processes are.

Note that config is an server implementation detail. We only version the API with semver, not the server software. It makes no sense to tie the two together. A release of API 7.1 means a release of the API, not the implementation that goes with it. It's a contract for the developer, not the server owner, and configuration is nothing to do with the developer.

Given that we've never versioned our implementation other than with a build number and API revision that it targets (we're always talking about SF 3002 or SV APIX-Y) - our server software (not our API) runs on a continuous delievery track. Each "minor" version should really be seen as each recommended build.

Indeed, we need this so that the API can function as intended. It won't change the config layout - this is a safe change. We've made other impl changes that have caused behaviour differences in "stable" (read, stable API targetting) builds before and this will continue.

gabizou · 2018-06-28T15:35:34Z

One thing I've been saying we need to do: Version our config files. Configurate supports versioning, we have a "version" of 1. If we took the time to version the configurations so that it would have versioning and we can document those changes, the docs could then serve that up for server administrators to consume.

I'm not keen on bumping minor versions, but what we can do is bump "bug fix" in semver, in the sense that we'd be at something like 7.1.9 at the moment (somewhere in the realm of 8 previous recommended builds?) Then we can further differentiate the versioning scheme between SF and SV as the "recommended changes" would be differentiable for developers and documenters.

bloodmc · 2018-06-28T18:43:03Z

    }

+    private void populateInstance() throws ObjectMappingException {
+        this.configMapper.populate(this.data.getNode(this.modId));


This needs to set mappedInstance or getConfig will cause NPE.

phit · 2018-06-28T20:07:53Z

okay i pulled last commit and tried it on some existing config i have
zip with before and after folder https://phit.link/s/sponge_config_saving_refactor.zip

seems to not work as intended, lots of duplication and it even seems to pull a lot of stuff from the global.conf to all subconfigs

branch I build my test jar with is here https://github.com/phit/SpongeCommon/commits/phit/test-config

* World and dimension configurations are now merged with their "parent" config on load * Each value within a config file is not written to the file if the setting is also present in the parent config with the same value * The 'config-enabled' property is no longer needed, values from world/dimension configs will always override.

lucko · 2018-07-02T15:33:37Z

@phit Can you try testing again with the latest chances.

blood and I worked through the remaining issues last night (hopefully) :)

phit · 2018-07-02T15:41:08Z

will do in a bit, would be nice if you wouldn't squash and force push until its ready to merge, makes it really hard to follow what was changed/fixed

lucko · 2018-07-02T15:41:41Z

Err sure, I won't do that from now on :)

phit · 2018-07-02T16:10:56Z

okay, just a few minor issues by the looks of it, but it works as expected, haven't tested the configs actually being applied, just looked at the files

before and after https://phit.link/s/configstuff3.zip

my only minor complaint is that stuff like module config, or stuff like plugins-dir i staying behind in the world config, even though they don't even do anything in that file, as it's a global only config

not sure if that can be easily fixed with this PR though, otherwise this looks good to me!

bloodmc · 2018-07-03T00:59:43Z

@phit Can you test overriding some settings to make sure they work as expected? So put a setting in a world config then try one in dimension. Thanks!

phit · 2018-07-03T02:38:41Z

will do when i wake up in a few hours

phit · 2018-07-03T14:58:34Z

worked as expected, tested with entity activation range and even auto-populate was fine

* World and dimension configurations are now merged with their "parent" config on load * Each value within a config file is not written to the file if the setting is also present in the parent config with the same value * The 'config-enabled' property is no longer needed, values from world/dimension configs will always override.

lucko added type: enhancement An improvement status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc system: config labels Jun 26, 2018

lucko force-pushed the feature/improved-configs branch from 24352b1 to 231e92f Compare June 26, 2018 12:25

lucko added the status: wip Work in progress label Jun 26, 2018

lucko force-pushed the feature/improved-configs branch from 231e92f to 4ae2421 Compare June 27, 2018 20:49

lucko removed the status: wip Work in progress label Jun 27, 2018

bloodmc reviewed Jun 28, 2018

View reviewed changes

lucko force-pushed the feature/improved-configs branch from 4ae2421 to a569f21 Compare June 28, 2018 19:38

lucko force-pushed the feature/improved-configs branch from a569f21 to ec2614a Compare July 2, 2018 15:33

bloodmc merged commit 7551c24 into bleeding Jul 4, 2018

bloodmc mentioned this pull request Jul 4, 2018

WorldProperties - GeneratorModifiers not Saving #1506

Closed

lucko deleted the feature/improved-configs branch July 4, 2018 07:54

ImMorpheus mentioned this pull request Aug 2, 2018

sponge -g/-w reload not working #2036

Closed

phit mentioned this pull request Aug 27, 2018

auto-populate broken since config rework SpongePowered/SpongeForge#2393

Closed

Uh oh!

Uh oh!

Conversation

lucko commented Jun 26, 2018

Uh oh!

phit commented Jun 26, 2018

Uh oh!

dualspiral commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucko commented Jun 26, 2018

Uh oh!

dualspiral commented Jun 26, 2018

Uh oh!

phit commented Jun 26, 2018

Uh oh!

dualspiral commented Jun 26, 2018

Uh oh!

gabizou commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bloodmc commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucko commented Jun 27, 2018

Uh oh!

ryantheleach commented Jun 28, 2018

Uh oh!

phit commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dualspiral commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabizou commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bloodmc Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

lucko Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

phit commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucko commented Jul 2, 2018

Uh oh!

phit commented Jul 2, 2018

Uh oh!

lucko commented Jul 2, 2018

Uh oh!

phit commented Jul 2, 2018

Uh oh!

bloodmc commented Jul 3, 2018

Uh oh!

phit commented Jul 3, 2018

Uh oh!

phit commented Jul 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dualspiral commented Jun 26, 2018 •

edited

Loading

gabizou commented Jun 26, 2018 •

edited

Loading

bloodmc commented Jun 27, 2018 •

edited

Loading

phit commented Jun 28, 2018 •

edited

Loading

dualspiral commented Jun 28, 2018 •

edited

Loading

gabizou commented Jun 28, 2018 •

edited

Loading

phit commented Jun 28, 2018 •

edited

Loading