search

za419 / CadenceBot

A Discord bot for Cadence Radio
MIT License
2 stars 1 forks source link

Persistent live reconfiguration #43

Open za419 opened 3 years ago

za419 commented 3 years ago

Add five commands to Cadence config related to persistence of configuration:

Additional effort

Loading sequence

It must be possible to reload config, and also we need an extra step to read in persistent-config.json. The current load sequence is:

  1. Read in default-config.json. If it can't be loaded, fail
  2. Read in config.json. If it can't be loaded, let it be empty.
  3. Copy all properties that aren't in config.json over from default-config.json
  4. If dynamic bans are enabled, read in bans.json and append any bans from that list to the list of bans from the other files.

This is a three-file load. This feature requires a fourth file:

  1. Read in default-config.json. If it can't be loaded, fail
  2. Read in config.json. If it can't be loaded, let it be empty.
  3. Read in persistent-config.json. If it can't be loaded, let it be empty.
  4. Copy all properties that aren't in persistent-config.json over from config.json
  5. Copy all properties that aren't in the merged config over from default-config.json
  6. If dynamic bans are enabled, read in bans.json and append any bans from that list to the list of bans from the other files.

The decision to split the saved configuration into a new file is, at the moment, a dubious one - If both config.json and persistent-config.json can only be edited by the owner of a CadenceBot instance (or their proxy), and both have the same scope, why add a new file that will take up space, slow loads, and add complexity to the already-overbuild CadenceBot config system?

At first glance, the reason would be to support reset and restore. This feature is however dubious - If we do our job properly, then backup-and-restore of config amounts to a timesaving measure to restore temporarily-disabled long-winded settings. This hardly merits the cost of supporting it.

The real reason is to support #44 - When configuration is set per-server instead of globally, then there shall be a separation of concerns: default-config and config will together describe the global configuration, and then persistent-config will describe the state for servers which choose to override global configuration. It simply makes more sense to add the new file here, rather than modify the existing flow, and then have to recreate that flow again in the next issue.

.gitignore

Two files must be added to .gitignore: persistent-config.json and persistent-config.backup.json

Notes (design reasoning)

Lack of diff-encoding

Certainly it could be done to implement save such that it only saves those settings which differ from the merged standard configuration. This would add some complexity, but it would help ensure that configuration stays 'current' - If the instance admin updates a setting in config, it automatically propagates to persistent-config, and the same goes for default-config. I decided against this - When this becomes per-server, lacking diff-encoding will keep server configs stable. However, certain options don't make sense to set per-server - They should be stripped before save when we get there. See #44. This is not diff-encoding as much as it is simply banning these options in persistent-config.

Full load instead of incremental load

It would be entirely possible to re-implement load such that it performs an incremental reload - i.e. it only reloads persistent-config. Naively, this suffers from a fault of not restoring settings that were deleted in persistent-config - This could of course be worked around by modifying the initial load to save the result of "config over default-config", then apply persistent-config over that - Then, incremental loading would simply overwrite that load. That would be entirely reasonable. I don't really have anything against this - But as an instance admin, I find myself editing config and then restarting my instance entirely to apply the changes more often than I'd like - And I would like to be able to use load to avoid that interruption.

Dependencies

This issue cannot be worked on until #42 is complete.

za419 commented 3 years ago

Update: Add spec for Cadence config backup, which backs up the current config without resetting it.

za419 commented 3 years ago

@ken, RFC:

Proposed update: Alter Cadence config restore to swap persistent-config and persistent-config.backup, as opposed to destroying the current config and replacing it with the backup. This allows an admin to notice that the backup is bad, and restore the original configuration - issuing Cadence config restore twice should be a no-op (faulty storage and buggy loads not interfering)