Migrating settings should take GlobalSettings into account

Contribute Bug

AI-generated summary

The discussion revolves around a bug in settings migrations where database values are used without considering discourse.conf settings in app.yml. RGJ highlights that migrations like the removal of automatic_backups_enabled ignore global settings, leading to incorrect behavior when discourse.conf overrides the database value. sam suggests checking ENV['DISCOURSE_...'] during migrations and proposes a MigrationHelper utility to handle settings correctly. RGJ confirms that GlobalSetting is loaded during migrations but advises against relying on build-time ENV variables, emphasizing the need for a consistent migration pattern that respects global overrides.

1

I’ve seen a few settings migrations recently (the most recent being the removal of automatic_backups_enabled ) where the migration uses database values only to calculate a new value. This ignores any settings made in discourse.conf via app.yml.

Code:

INSERT INTO site_settings (name, data_type, value, created_at, updated_at)
      SELECT 'backup_frequency', 3, NULL, 'NOW()', 'NOW()'
      WHERE EXISTS (
        SELECT 1
        FROM site_settings
        WHERE name = 'automatic_backups_enabled'
        AND VALUE = 'f'
        LIMIT 1
      )
      ON CONFLICT (name) DO UPDATE SET value = NULL, updated_at = 'NOW()';

which ignores automatic_backups_enabled = false in discourse.conf and as a result, does not keep backups disabled when that setting is present.

This specific ship has sailed, but it might be good to keep in mind that this pattern causes issues with settings that are overridden globally.

4 Likes
2

I guess in this case, in future we should look at DISCOURSE_... in Env during these migrations.

@ted I guess it is worth following up retroactively cause it will establish the pattern going forward.

I am not sure if GlobalSetting is loaded or is something we want to lean on, but taking a peak at

ENV['DISCOURSE_AUTOMATIC_BACKUPS_ENABLED'] and having it win over the setting in the DB is probably wise.

@tgxworld / @ted maybe we should add a “migration helper” for this?

Eg:

MigrationHelper.get_setting('abc')
MigrationHelper.set_setting('abc', '123')
3 Likes
4

Yes, GlobalSetting is loaded during migrations :+1:

I don’t think you want to lean on a build time ENV var either? It makes it hard to separate the ENV var to discourse.conf step and the actual container deployment. In my other world (PHP/Laravel) this is seen as an anti-pattern.

1 Like