Improve GitLab::CurrentSettings to handle edge cases
There are a number of problems with the current implementation of GitLab::CurrentSettings
, and this module has grown to be quite complicated and error-prone:
- If a new column is added to
ApplicationSetting
, the module will always return the cached copy, which may lead to missing configuration values. We don't invalidate this in the migrations. - If any migration is pending (including post migrations), 9.3 RC2 returns an
OpenStruct
set of settings. This resulted in blocking 9.3 RC2, causing the sign-in page to disable password sign-in altogether (see https://gitlab.com/gitlab-org/gitlab-ce/issues/33736#note_32561501 for more details). - We attempted to disable the migration check in item 2 in !12236 (merged). However, if no
ApplicationSetting
is present in the database, thenApplicationSetting.create_from_defaults
will fail because the model will attempt to write to a column that does not yet exist. This is happening in https://gitlab.com/gitlab-org/gitlab-ce/builds/18898170. - It uses poorly named predicate methods such as
retrieve_settings_from_database_cache?
that actually returns more than justtrue
orfalse
. We should fix this.
Let me see if I can identify the problems we're trying to solve with this module:
- We want to cache the
ApplicationSetting
aggressively since this is accessed all the time. - There may be a migration present that adds/removes/changes a column to the setting.
- Initializers (e.g. metrics) check the settings to see if things are enabled. We are trying to return a reasonable default if a column hasn't been added to the database.
- Rake tasks, especially
rake db:migrate
, shouldn't ever fail trying to loadApplicationSetting
.
Some ideas on how to improve this module:
- Since we want to cache the application settings and we don't want to check the migrations table repeatedly, we may need to change
ApplicationSetting::CACHE_KEY
to include a version. Perhaps we should cache the latest schema migration in the table and dynamically use that in the key. - If a new column is added to the
ApplicationSetting
schema but the migration hasn't been applied yet, we should load existing values and fill in defaults for the non-existing values. - Ensure that Rake tasks always get a reasonable set of configuration values, and they should never attempt to insert a row.
Attempted this in gitlab-org/gitlab-ce!12250. - @rspeicher suggested that perhaps this module is doing too much, but given all the problems we're trying to solve I'm not sure how we get around not having some module like this.
/cc: @rymai, @rspeicher, @nick.thomas, @techguru, @lbennett
Edited by Stan Hu