Skip to content

Remove Settingslogic gem

Kassio Borges requested to merge kassio/remove-settingslogic into master

What does this MR do and why?

FAQ

  • Why?
  • Why not fork Settingslogic, as proposed?
    • IMHO creating a fork would be maybe a faster solution, but it could bring more work in the long run. The wider community can use the Rails built-in config_for for custom yaml config files, which became available in Rails version 4.2.1. Therefore, I don't see a need for us to maintain a gem for a built-in Rails feature that we would use on a single project.
  • Why not Rails::Application.config_for then?
    • For backward compatibility and because gitlab.yml:
      • Rails::Application.config_for has a special meaning for the shared key, which is a valued shared among rails environments, while we already use it for shared file storage settings
      • We have high usage of method calling for nested config keys (Settings.config1.config2.config3), while rails way is to methodify only first level keys (Settings.config1[:config2][:config3]).

TODO

  • Raise exception when settings doesn't exist
  • Make CI green

Problems

  • Current implementation is still inheriting from Hash, indirectly through ::ActiveSupport::OrderedOptions
    Due to some test failures, I changes GitlabSettings::Options to not inherit from ::ActiveSupport::OrderedOptions, instead it holds an instance of Hash and delegates methods to it with #method_missing and implement some other methods to ensure the desired behavior.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

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

Edited by Rémy Coutable

Merge request reports