העברת הגדרות צריכה לקחת בחשבון GlobalSettings

Bug
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

אני מניח שבמקרה הזה, בעתיד עלינו לבדוק את DISCOURSE_... בסביבה במהלך ההגירות הללו.

@ted אני מניח שכדאי לעקוב אחר זה רטרואקטיבית כי זה יבסס את התבנית להמשך.

אני לא בטוח אם GlobalSetting נטען או אם זה משהו שאנחנו רוצים להסתמך עליו, אבל הצצה ל-

ENV['DISCOURSE_AUTOMATIC_BACKUPS_ENABLED'] וגורם לו לנצח על ההגדרה ב-DB זה כנראה חכם.

@tgxworld / @ted אולי כדאי להוסיף “עוזר הגירה” לכך?

לדוגמה:

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