迁移设置应考虑 GlobalSettings

错误
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 个赞
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 个赞
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 个赞