Add warning for local settings changes

[Fryguy]

This PR adds a warning when any settings.local.yml file has changes in it. Because these changes aren't necessarily expected, they can break things if the user is not aware.

In my case, I had a settings.local.yml file present and had forgotten about it, and then test cases kept failing and I couldn't figure out why. This file is intentionally ignored by .gitignore, so locally it looked like nothing was changed.

@agrare Please review. Not sure about the wording.

Example screenshots:

With changes to config/settings.local.yml:

[Fryguy]

I considered making this test and/or dev only, but I think it's useful to know if someone's touched this file in production too, so I left it for everything.

[kbrock]

This is similar to our bundler warning. I wonder if we want the option to turn this off

[Fryguy]

I wonder if we want the option to turn this off

I still don't understand disabling warnings even on the bundler inject side 😉

[agrare]

@Fryguy what is the difference between config/settings.local.yml and config/settings/development.yml? I don't see a warning with just the latter and that seems to work fine where config/settings.local.yml causes the failure you hit

[agrare]

@Fryguy I tried adding your prototype/ems_workflows to config/settings/development.yml and it also wipes out the whole Settings.prototype entry but without any warnings:

$ git diff
diff --git a/config/settings/development.yml b/config/settings/development.yml
index f453567d82..c9462bcc53 100644
--- a/config/settings/development.yml
+++ b/config/settings/development.yml
@@ -3,3 +3,6 @@
   :level_rails: debug
 :webservices:
   :url: http://localhost:4000
+prototype:
+  ems_workflows:
+    enabled: true

$ rails c
** ManageIQ master, codename: Spassky
Loading development environment (Rails 7.0.8.4)
>> Settings.prototype
=> #<Config::Options ems_workflows=#<Config::Options enabled=true>>

[Fryguy]

@agrare config/settings/development.yml is a committed file (i.e. it's not a local file, but an actual settings file we might commit) whereas config/settings/development.local.yml is a local file and would be checked

[Fryguy]

Note that I opened #23168 for the separate bug that string keys clobber symbol keys.

[agrare]

I turn the bundler-inject warnings off also. I added the overrides I don't need to be warned that they're there everytime I open a rails console IMO. If the user manually added the files/overrides it seems odd that we'd warn them that they did that.

[Fryguy]

This might be easier to see the difference...the first 3 are committed files, the bottom 3 are "local" overrides. This PR only warns on the local overrides.

> Settings.instance_variable_get(:@config_sources).select { |s| s.try(:path)&.to_s&.start_with?(Dir.pwd) }
=>
[#<Config::Sources::YAMLSource:0x0000000115fce3e0 @path="/Users/jfrey/dev/manageiq/config/settings.yml">,
 #<Config::Sources::YAMLSource:0x0000000115fcd4e0 @path="/Users/jfrey/dev/manageiq/config/settings/development.yml">,
 #<Config::Sources::YAMLSource:0x0000000115fcd4b8 @path="/Users/jfrey/dev/manageiq/config/environments/development.yml">,
 #<Config::Sources::YAMLSource:0x0000000116866f48 @path="/Users/jfrey/dev/manageiq/config/settings.local.yml">,
 #<Config::Sources::YAMLSource:0x0000000116866f20 @path="/Users/jfrey/dev/manageiq/config/settings/development.local.yml">,
 #<Config::Sources::YAMLSource:0x0000000116866ef8 @path="/Users/jfrey/dev/manageiq/config/environments/development.local.yml">]

[Fryguy]

If the user manually added the files/overrides it seems odd that we'd warn them that they did that.

There's no indication anywhere that the change is present, and it's easily forgotten (as in my case). Additionally, settings.local.yml affects all envs including test. This is the same reason we warn on plugins overrides - it's easy to forget that you've overridden a plugin, and it can create confusing situations if you're not aware. I think plugins are a little different in that a dev focused on an area might regularly override some (in your case provider plugins) however local settings really shouldn't be regularly used (i.e. if you need more "permanent" settings, then just commit it to the database as SettingsChanges).

[kbrock]

I always have settings/development.local.yml overridden:

:ems:
  :ems_workflows:
    :runner: "docker"
:prototype:
  :ems_workflows:
    :seed_builtin_workflows: true
:server:
  :asynchronous_notifications: false
#  :session_store: redis
:session:
  :interval: 60
  # redis://[:password@]host[:port][/db-number]
  :redis_url: redis://127.0.0.1/3
:workers:
  :worker_base:
    :schedule_worker:
# useless session clear (too much noise)
      :session_timeout_interval: 1.hour
# timer goes off every 3 minutes, reduce frequency (maybe 20.minutes)
#       :performance_collection_interval: 10.minutes
      :performance_collection_interval: 20.minutes
      :performance_collection_start_delay: 5.minutes
:smtp:
#  :authentication: login
#  :password: ''
#  :user_name: evmadmin
  :authentication: 'plain'
  :domain: example.com
  :from: cfadmin@example.com
  :host: 127.0.0.1
  :port: '1025'

I deal with enough databases that using SettingsChange isn't a viable alternative for me. I tend to have different databases for different vmware simulator values and have to blow it away since refresh / metrics is sticky and I need to wipe it out often.

I never touch settings.local.yml

[kbrock]

For some people, this extra noise is expensive.

If you are adding noise to the screen for the developer's "convenience", also add the feature to be quiet and get out of the developer's way.

I do tend to leave the bundler warnings on - but it is very annoying and I frequently am tempted to turn it off.

[Fryguy]

I closed because both @kbrock and @agrare seem to be using it in reasonable ways (I assumed no one used it regularly and the "correct" way is to just persist changes), and I agree with not creating noise for things that we are expected to use.

However, I just noticed @kbrock used settings/development.local.yml, which is the "correct" file, as opposed to settings.local.yml.

@agrare @kbrock Would you be ok if I reopened and changed to only warn on settings.local.yml? That file really shouldn't be used, and using settings/development.local.yml feels correct for more long term type settings (if not persisting).

[agrare]

Should we not load from that file then if it shouldn't be used? It looks like we explicitly pull from it https://github.com/ManageIQ/manageiq/blob/master/lib/vmdb/settings.rb#L175

[Fryguy]

@agrare It's a good question...I did consider it. It's part of how the config gem works under the covers, so I don't want to break their contract.

The reason we explicitly list it is because we have plugins, so we need to enumerate all of the plugins, because the config gem doesn't support engines natively (and also because we have those database-level changes which needs to get in between file-level changes and local overrides)

Also, it's not really wrong to use it, per se (which is why I keep using air-quotes around the word "correct"). It's a useful tool, but as with all tools, it has downsides, namely that it's easily forgettable with no indication of change and also modifies all of dev/test/prod envs. For other use cases, other files are better, such as settings/development.local.yml, which is a better choice for more permanent settings in dev like how @kbrock is using it.

[Fryguy]

Updated the commit to only look at the settings.local.yml and not the other environment-specific local files.

[agrare]

Oh okay I didn't realize that was one of their default files they look for also, yeah :+1: for warning on that one then

[Fryguy]

@agrare I'm glad you pointed me to the config gem. It seems recent versions changed it so that settings.local.yml specifically file is not loaded in test environments. I'm going to change the way our settings code works by leveraging more of the core gem, including this whole test-doesn't-load-locals thing instead. Then, I think we don't need a warning.