Instellingen migreren moet rekening houden met GlobalSettings

Fout
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

Ik denk dat we in dit geval in de toekomst moeten kijken naar DISCOURSE_... in Env tijdens deze migraties.

@ted Ik denk dat het de moeite waard is om achteraf op te volgen, omdat het het patroon voor de toekomst zal vaststellen.

Ik weet niet zeker of GlobalSetting is geladen of iets is waar we op willen leunen, maar een blik werpen op

ENV['DISCOURSE_AUTOMATIC_BACKUPS_ENABLED'] en het laten winnen van de instelling in de DB is waarschijnlijk verstandig.

@tgxworld / @ted misschien moeten we hier een “migratiehulp” voor toevoegen?

Bijvoorbeeld:

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