Zeitwerk #9287

ofedoren · 2022-06-29T15:53:10Z

Based on #9148.

ofedoren · 2022-06-30T13:02:02Z

I guess it's open for discussions/suggestions :) I've checked that the applications starts with this PR (couldn't test all the functionality since my setup contains a lot of plugins, which are not Zeitwerk compatible and due to DB changes some pages aren't available), so I'd say overall we're fine after those changes. The main issue is that it's a really breaking change and plugins must be fixed in time.

I'll ping here all the people who touched this area just in case: @ezr-ondrej, @stejskalleos, @adamruzicka.

UPD: I hope someone will give me a hint how we (or me) could fix the tests since there is not much information from Jenkins and locally almost all the tests are broken, but in a different way... :/

ofedoren · 2022-06-30T13:05:10Z

app/services/setting_registry.rb

-  def known_categories
-    unless @known_descendants == Setting.descendants
-      @known_descendants = Setting.descendants
-      @known_categories = @known_descendants.map(&:name) << 'Setting'
-      @values_loaded_at = nil # force all values to be reloaded
-    end
-    @known_categories
-  end
-
  def load_values(ignore_cache: false)
    # we are loading only known STIs as we load settings fairly early the first time and plugin classes might not be loaded yet.
-    settings = Setting.unscoped.where(category: known_categories).where.not(value: nil)
+    settings = Setting.unscoped.where(category: 'Setting').where.not(value: nil)


My main concern is here. Not sure if we can do this right now, any ideas @ezr-ondrej? :)

This basically means we will no longer support any other setting then DSL based, which is IMHO time to do.
We did deprecated it

foreman/app/models/setting.rb

Line 204 in a439e1e

Foreman::Deprecation.deprecation_warning('3.4', "subclassing Setting is deprecated '#{name}' should be migrated to setting DSL "\

I'd love a separate PR for that, ideally dropping the category column from setting alltogether, for us to be explicit about the fact. I'll be back online on 11th and I'll be happy to take a look into it that week.

@ezr-ondrej I don't mind take care about deprecating the Settings, but I don't have much idea where to start ... If you point me to what needs to be done I'll push the PR

@stejskalleos @ofedoren sorry for the delay it totally slipped my mind when I couldn't get to it that week, sorry! Here is my take on it #9524, my foreman knowledge is bit rusty so I hope I got it right.

If I can do some other clean-ups that would elevate some pain from this PR, I'm happy to do so, I believe it would help the project to get this done (but I know it's pain to find and fix all the blockers)

ofedoren · 2022-06-30T13:05:55Z

config/application.rb

@@ -115,7 +115,7 @@ class Application < Rails::Application
    config.autoload_paths += %W(#{config.root}/app/models/compute_resources)
    config.autoload_paths += %W(#{config.root}/app/models/fact_names)
    config.autoload_paths += %W(#{config.root}/app/models/lookup_keys)
-    config.autoload_paths += %W(#{config.root}/app/models/host_status)
+    # config.autoload_paths += %W(#{config.root}/app/models/host_status)


This is from previous commits. My guess is we can remove this line completely.

and all the above IMHO. we should not add subfolders of app/* as all the folders are roots for autoloading and if the files follow the naming patterns expected by autoloaders, it should work without this.

ofedoren · 2022-06-30T13:08:44Z

config/initializers/zeitwerk.rb

+  'ec2' => 'EC2',
+  'aws' => 'AWS',
+  'gce' => 'GCE',
+  'aix' => 'AIX',
+  'nxos' => 'NXOS',
+  'vrp' => 'VRP',
+  'sso' => 'SSO',
+  'puppet_ca_certificate' => 'PuppetCACertificate',
+  'url_resolver' => 'URLResolver',
+  'ztp_record' => 'ZTPRecord',
+  'aaaa_record' => 'AAAARecord',
+  'ptr4_record' => 'PTR4Record',
+  'ptr6_record' => 'PTR6Record'


This is basically workaround for /lib directory. Also, could please someone tell me why do we even have /lib in eager_load_paths? And also we have there code which is tightly bound to the whole application and per my thinking and Rails suggestions we should move it to app/lib directory or somehow refactor.

We (mainly @stejskalleos) has started to work on exactly this and I believe /lib is not autoloaded anymore, we were not sure where everywhere we would need to add require statements, but ideal state is /lib content being explicitly required where needed (or from application.rb) ... and indeed if there are some strictly application related files that belongs to lib, those should be in app/lib

stejskalleos · 2022-07-27T08:01:04Z

app/models/compute_resources/foreman/model/vmware.rb

@@ -818,3 +817,5 @@ def cachekey_with_cluster(key, cluster_id = nil)
    end
  end
 end
+
+require 'fog_extensions/vsphere/mini_servers'


Question - why this change? Is there a difference between require at the beginning of file and at the end of the file?

This is weird... At the time I've been writing this, I've faced circular dependency in loading Vmware constant, but just to remember why exactly I did that, I've put this line back, run the zeitwerk:check again and all seems good. So, indeed, there is no need to put this line to the end :/ Reverted.

stejskalleos · 2022-07-27T13:58:07Z

config/initializers/zeitwerk.rb

+  'ptr6_record' => 'PTR6Record'
+)
+
+Rails.autoloaders.main.ignore(


@adamruzicka, @ofedoren Do you guys know what's the difference between Rails.autoloaders.main and ``
Rails docs doesn't say much, after while of searching still couldn't find the difference. From first look they return same object but then why there are two methods?

Rails.autoloaders.main => #<Zeitwerk::Loader:0x00000000073af3e8 @autoloaded_dirs=[], @autoloads={}, @collapse_dirs=#<Set: {}>, @collapse_glob_patterns=#<Set: {}>, @eager_load_exclusions=#<Set: {}>, @eager_loaded=false, @ignored_glob_patterns=#<Set: {}>, @ignored_paths=#<Set: {}>, @inflector=ActiveSupport::Dependencies::ZeitwerkIntegration::Inflector, @initialized_at=2022-07-27 15:53:58.924070552 +0200, @lazy_subdirs={}, @logger=nil, @mutex=#<Thread::Mutex:0x00000000073aefb0>, @mutex2=#<Thread::Mutex:0x00000000073aef88>, @on_load_callbacks={}, @on_setup_callbacks=[], @on_unload_callbacks={}, @reloading_enabled=false, @root_dirs={}, @setup=false, @tag="rails.main", @to_unload={}> Rails.autoloaders.once => #<Zeitwerk::Loader:0x000000000af8bd80 @autoloaded_dirs=[], @autoloads={}, @collapse_dirs=#<Set: {}>, @collapse_glob_patterns=#<Set: {}>, @eager_load_exclusions=#<Set: {}>, @eager_loaded=false, @ignored_glob_patterns=#<Set: {}>, @ignored_paths=#<Set: {}>, @inflector=ActiveSupport::Dependencies::ZeitwerkIntegration::Inflector, @initialized_at=2022-07-27 15:54:07.749815776 +0200, @lazy_subdirs={}, @logger=nil, @mutex=#<Thread::Mutex:0x000000000af8b6c8>, @mutex2=#<Thread::Mutex:0x000000000af8b628>, @on_load_callbacks={}, @on_setup_callbacks=[], @on_unload_callbacks={}, @reloading_enabled=false, @root_dirs={}, @setup=false, @tag="rails.once", @to_unload={}>

https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#config-autoload-paths talks about which paths are managed by which autoloader

ekohl · 2022-07-30T18:49:24Z

app/services/proxy_status/puppetca.rb

@@ -1,5 +1,5 @@
 module ProxyStatus
-  class PuppetCA < Base
+  class Puppetca < Base


In #9328 I chose to make CA an acronym and name the file puppet_ca instead. Didn't notice this PR until now.

ekohl · 2022-07-30T18:51:06Z

config/initializers/zeitwerk.rb

+  Rails.root.join('app/registries/foreman/access_permissions.rb'),
+  Rails.root.join('app/registries/foreman/settings.rb'),
+  Rails.root.join('app/registries/foreman/settings')


I wondered if these shouldn't be initializers.

ekohl · 2022-07-30T22:03:49Z

config/application.rb

@@ -104,7 +104,7 @@ class Application < Rails::Application
    Dir["#{Rails.root}/config/routes/**/*.rb"].each do |route_file|
      config.paths['config/routes.rb'] << route_file
    end
-
+    config.load_defaults 6.0


I had to add

# Rails 5 changed this to true config.active_record.belongs_to_required_by_default = false

I opened #9382 to do this as a separate preparation step.

theforeman-bot · 2022-11-26T01:23:03Z

Thank you for your contribution, @ofedoren! This PR has been inactive for 3 months, closing for now.
Feel free to reopen when you return to it. This is an automated process.

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Jun 29, 2022

ofedoren force-pushed the zeitwerk-ofedoren branch from 23c6e10 to fd1b1a2 Compare June 29, 2022 16:04

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 29, 2022

stejskalleos self-assigned this Jun 30, 2022

ofedoren added Breaking change This PR may break plugins - maintainers try to apply it as best effort labels Jun 30, 2022

ofedoren commented Jun 30, 2022

View reviewed changes

adamruzicka and others added 3 commits July 1, 2022 12:11

Zeitwerk groundwork

327d617

More inflections

54d3737

Setting loading

362dae5

ofedoren force-pushed the zeitwerk-ofedoren branch from fd1b1a2 to deb4dce Compare July 1, 2022 12:12

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jul 1, 2022

stejskalleos reviewed Jul 27, 2022

View reviewed changes

Multiple fixes

b719dee

ofedoren force-pushed the zeitwerk-ofedoren branch from deb4dce to b719dee Compare July 27, 2022 11:43

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jul 27, 2022

stejskalleos reviewed Jul 27, 2022

View reviewed changes

ekohl mentioned this pull request Jul 30, 2022

Rails 7 & Ruby 3.1 #9328

Closed

ekohl reviewed Jul 30, 2022

View reviewed changes

theforeman-bot added the Inactive label Nov 26, 2022

theforeman-bot closed this Nov 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zeitwerk #9287

Zeitwerk #9287

ofedoren commented Jun 29, 2022

ofedoren commented Jun 30, 2022 •

edited

Loading

ofedoren Jun 30, 2022

ezr-ondrej Jul 4, 2022

stejskalleos Jul 27, 2022

ezr-ondrej Nov 26, 2022

ofedoren Jun 30, 2022

ezr-ondrej Jul 4, 2022

ofedoren Jun 30, 2022

ezr-ondrej Jul 4, 2022

stejskalleos Jul 27, 2022

ofedoren Jul 27, 2022

stejskalleos Jul 27, 2022

adamruzicka Jul 27, 2022

ekohl Jul 30, 2022

ekohl Jul 30, 2022

ekohl Jul 30, 2022

ekohl Aug 25, 2022

theforeman-bot commented Nov 26, 2022

Zeitwerk #9287

Zeitwerk #9287

Conversation

ofedoren commented Jun 29, 2022

ofedoren commented Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Nov 26, 2022

ofedoren commented Jun 30, 2022 •

edited

Loading