Skip to content

Split-off ApplicationSetting specific code from Gitlab::CurrentSetting

Rutger Wessels requested to merge 394801-refactor-current_settings into master

What does this MR do and why?

This MR is a refactor of Gitlab::CurrentSettings . This class provides read access to ApplicationSetting. A common usage is something like this: Gitlab::CurrentSettings.max_import_size.megabytes.

As part of Organizations introduction, we will migrate some of the settings from ApplicationSetting to OrganizationSetting model. So some settings are on the Instance level while other can be specific for an Organization

I am preparing a MR that will modify Gitlab::CurrentSettings: it will read from OrganizationSetting model and will use ApplicationSetting as fallback. This way, we do not have to change the usages of Gitlab::CurrentSettings. But I felt the CurrentSettings class has too much responsibility

Currently, the CurrentSetting class is not only caching and returning settings but also has logic that takes care of availability of settings in case of missing database connection, missing application setting record and application settings for test environment.

This MR will split off the ApplicationSetting specific part into a new Gitlab::ApplicationSettingFetcher

Implementation

  • Moved code to ApplicationSettingFetcher class.
  • Public interface (method names, arguments) did not change
  • Test coverage increased: all public methods are now under test

How to set up and validate locally

  • All the tests should pass (using label pipeline:run-all-rspec )
  • Edge case: AppliationSetting can be used while the database is not available. This is supported only for rake tasks
    • gdk stop postgresql && bundle exec rake gitlab:shell:setup (it can be aborted before it does something), this should not raise ActiveRecord::ConnectionNotEstablished: could not connect to server

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #417763 (closed)

Edited by Rutger Wessels

Merge request reports