Skip to content

Draft: Refactor Gitlab::CurrentSettings so it has less ApplicationSetting-specific code Part I

What does this MR do and why?

This is part One of two MR's related to a small refactoring of GitLab::CurrentSetting class. This class has too many responsibilities:

  • A single entry point for Application Settings
  • Taking care of available settings in case the database or Redis is not available, taking care of settings in test context

In the near future, we want this class to also read from Organization Settings, so we want to remove the ApplicationSetting-specific code.

This will happen in two MR's:

  • This MR:
    • Introduce Gitlab::ApplicationSettingFetcher and port the part that is using the cached (Redis) version of ApplicationSettings
  • Next MR:
    • port the code that handles uncached version of ApplicationSettings (missing database connection, missing Redis, test environment)

The code change is behind a feature flag considering the risk

How to set up and validate locally

  • All the tests should pass (using label pipeline:run-all-rspec )
  • With feature flag refactor_current_settings_caching enabled and also disabled:
    • 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
  • gdk start and then test the web application locally

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