Draft: PoC: Extract ApplicationSettings into a Packwerk package

What does this MR do and why?

Related to PoC: Isolate ApplicationSettings using modular ... (#421713 - closed)

In this MR I attempt to create a Packwerk package that hosts the code related to ApplicationSetting for both Core and EE. For Packwerk to work correctly it's best that both Core and EE extensions are contained inside the same Packwerk package. For this I created modules/instance_settings package.

The goal of this package is to:

• Provide a public API InstanceSettings with safe methods for developers to use and manage instance-wide settings.
• isolate ApplicationSetting ActiveRecord model so that developers don't interact with AR model directly, which could be dangerous.

Changes:

• Moved ApplicationSetting as private constant so that no-one can reference that directly.
• Create InstanceSettings module as the public API for the package.
• Moved ApplicationSettings::UpdateService behind the InstanceSettings.update public interface. The service class is also private to the package and became an implementation detail.
  • The UpdateService had a strange design that was updating the settings object passed as input but that can sometimes differ in specs if developer references a cached version. I changed the service object to return the updated settings object with the ServiceResponse object for a consistent and predictable behavior.
• Moved some constants like ApplicationSetting::FORBIDDEN_KEY_VALUE inside the InstanceSettings package and made them private constants.

Directory structure that is going to be used

modules
└── instance_settings      # package directory
    ├── package.yml
    ├── packwerk.yml
    ├── package_todo.yml
    ├── core               # the main code goes here
    │   ├── app
    │   │   ├── models/...
    │   │   ├── services/...
    │   │   └── lib/...    # yes, the domain code from lib is moved inside the package
    │   └── spec
    │       └── models/... # specs for core code
    ├── ee                 # EE extensions live inside the same package but it's conditionally loaded
    │   ├── models/...
    │   └── spec
    │       └── models/...
    └── public             # where all public constants are placed. Packwerk automatically allowlists the constants from `public` directories
        ├── core           # public interface for Core functionalities.
        │   ├── app
        │   │   └── models/...
        │   └── spec
        │       └── models/...
        └── ee             # Public interface for EE is conditionally loaded
            ├── app
            │   └── models/...
            └── spec
                └── models/... 

Observations

• We use ApplicationSetting and Gitlab::CurrentSettings interchangeably
• It seems that Gitlab::CurrentSettings was introduced later to wrap ApplicationSetting and to use FakeApplicationSetting in some specific runtimes when ApplicationSetting is not initialized.
• Both ApplicationSetting and Gitlab::CurrentSettings use SafeRequestStore to cache the results, so we are caching it twice and the latter settings uses the former under the hood. I'm not sure if this double cache could lead to issues since we don't have a SSoT for cache expiration.
• Both ApplicationSetting and Gitlab::CurrentSettings are writable and without even permission checks! In this PoC we attempt to return a readonly object to it's explicit when we want to read settings vs when we want to write settings. However we need to have more explicit interface for unsafe operations, like when we update the settings without permissions checks as part of a rake task, etc.
• We have cases (especially in specs) where we do ApplicationSetting.current.update!(...) and also Gitlab::CurrentSettings.update!. I feel we need a consistent way to stub application settings rather than using AR methods.
• The lack of a SSoT and information hiding through privacy protection allows any developer to use settings in the most disparate ways.

How to do it in iterations?

• To make these changes more incrementally and avoid such large MRs we should:
  • First ensure that only Gitlab::CurrentSettings is used in the codebase and not ApplicationSetting
  • Create a new SSoT InstanceSettings module/package
  • Move the private classes ApplicationSetting and FakeApplicationSetting in it.
  • introduce a new constant side-by-side to the existing one and migrate usage to new constants in several small MRs. Then remove the old constant and repeat with other constants. (e.g. ApplicationSetting::UpdateService).

MR acceptance checklist

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

• I have evaluated the MR acceptance checklist for this MR.