Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zeitwerk #9287

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/helpers/host_description_helper.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module HostDescriptionHelper
UI.register_host_description do
::UI.register_host_description do
multiple_actions_provider :base_multiple_actions
overview_fields_provider :base_status_overview_fields
overview_fields_provider :base_host_overview_fields
Expand Down
37 changes: 32 additions & 5 deletions app/models/concerns/foreman/sti.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
module Foreman
module STI
def self.prepended(base)
class << base
prepend ClassMethods
end
extend ActiveSupport::Concern

prepended do
cattr_accessor :preloaded, instance_accessor: false
end

module ClassMethods
class_methods do
# ensures that the correct STI object is created when :type is passed.
def new(*attributes, &block)
if (h = attributes.first).is_a?(Hash) && (type = h.with_indifferent_access.delete(:type)) && !type.empty?
Expand All @@ -18,6 +18,33 @@ def new(*attributes, &block)

super
end

def descendants
preload_sti unless preloaded
super
end

# Constantizes all types present in the database. There might be more on
# disk, but that does not matter in practice as far as the STI API is
# concerned.
#
# Assumes store_full_sti_class is true, the default.
def preload_sti
return [] unless base_class.connected? && base_class.table_exists?
types_in_db = base_class
.unscoped
.select(inheritance_column)
.distinct
.pluck(inheritance_column)
.compact

types_in_db.each do |type|
logger.debug("Preloading STI type #{type}")
type.constantize
end

self.preloaded = true
end
end

def save(*args)
Expand Down
2 changes: 2 additions & 0 deletions app/models/concerns/nested_ancestry_common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,5 @@ def update_matchers
lookup_values.update_all(:match => "#{obj_type}=#{title}")
end
end

require_dependency 'nested_ancestry_common/search'
2 changes: 1 addition & 1 deletion app/models/hostgroup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class Hostgroup < ApplicationRecord
property :pxe_loader, String, desc: 'Returns boot loader to be applied on each host within this host group'
property :title, String, desc: 'Returns full title of this host group, e.g. Base/CentOS 7'
end
class Jail < Safemode::Jail
class Jail < ::Safemode::Jail
allow :id, :name, :diskLayout, :puppet_server, :operatingsystem, :architecture,
:ptable, :url_for_boot, :params, :puppet_proxy, :puppet_ca_server,
:os, :arch, :domain, :subnet, :subnet6, :hosts, :realm,
Expand Down
2 changes: 1 addition & 1 deletion app/models/subnet.rb
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ def external_ipam?
end

def external_ipam_proxy(attrs = {})
@external_ipam_proxy ||= ProxyAPI::ExternalIpam.new({:url => externalipam.url}.merge(attrs)) if external_ipam?
@external_ipam_proxy ||= ProxyAPI::ExternalIPAM.new({:url => externalipam.url}.merge(attrs)) if external_ipam?
end

def ipam?
Expand Down
2 changes: 1 addition & 1 deletion app/models/taxonomy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def self.inherited(child)
end
child.const_set('Jail', jail_class)
end
child.send(:include, NestedAncestryCommon::Search)
child.send(:include, ::NestedAncestryCommon::Search)
super
end

Expand Down
4 changes: 1 addition & 3 deletions app/registries/foreman/plugin/report_scanner_registry.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
require_dependency File.expand_path('../../../services/report_scanner/puppet_report_scanner', __dir__)

module Foreman
class Plugin
class ReportScannerRegistry
DEFAULT_REPORT_SCANNERS = [
::Foreman::PuppetReportScanner,
::Foreman::ReportScanner::PuppetReportScanner,
].freeze

attr_accessor :report_scanners
Expand Down
Empty file.
20 changes: 20 additions & 0 deletions app/services/foreman/report_scanner/puppet_report_scanner.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module Foreman
module ReportScanner
class PuppetReportScanner
class << self
def identify_origin(report_data)
'Puppet' if puppet_report?(report_data['logs'] || [])
end

def add_reporter_data(report, report_data)
# no additional data apart of origin
end

def puppet_report?(logs)
log = logs.last
log && log['log'].fetch('sources', {}).fetch('source', '') =~ /Puppet/
end
end
end
end
end
4 changes: 2 additions & 2 deletions app/services/ipam.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ def self.new(type, *args)
when IPAM::MODES[:none]
IPAM::None.new(*args)
when IPAM::MODES[:dhcp]
IPAM::Dhcp.new(*args)
IPAM::DHCP.new(*args)
when IPAM::MODES[:db]
IPAM::Db.new(*args)
when IPAM::MODES[:random_db]
IPAM::RandomDb.new(*args)
when IPAM::MODES[:eui64]
IPAM::Eui64.new(*args)
when IPAM::MODES[:external_ipam]
IPAM::ExternalIpam.new(*args)
IPAM::ExternalIPAM.new(*args)
else
raise ::Foreman::Exception.new(N_("Unknown IPAM type - can't continue"))
end
Expand Down
2 changes: 1 addition & 1 deletion app/services/ipam/dhcp.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module IPAM
class Dhcp < Base
class DHCP < Base
delegate :dhcp, :dhcp_proxy, :to => :subnet
def suggest_ip
unless subnet.dhcp?
Expand Down
2 changes: 1 addition & 1 deletion app/services/ipam/external_ipam.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module IPAM
class ExternalIpam < Base
class ExternalIPAM < Base
delegate :external_ipam_proxy, :to => :subnet

def suggest_ip
Expand Down
2 changes: 1 addition & 1 deletion app/services/proxy_api/external_ipam.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'uri'

module ProxyAPI
class ExternalIpam < ProxyAPI::Resource
class ExternalIPAM < ProxyAPI::Resource
def initialize(args)
@url = args[:url] + "/ipam"
super args
Expand Down
4 changes: 2 additions & 2 deletions app/services/proxy_status/puppetca.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module ProxyStatus
class PuppetCA < Base
class Puppetca < Base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def certs
fetch_proxy_data('/certs') do
api.all.map do |name, properties|
Expand Down Expand Up @@ -71,4 +71,4 @@ def api_class
end
end
end
ProxyStatus.status_registry.add(ProxyStatus::PuppetCA)
ProxyStatus.status_registry.add(ProxyStatus::Puppetca)
18 changes: 0 additions & 18 deletions app/services/report_scanner/puppet_report_scanner.rb

This file was deleted.

11 changes: 1 addition & 10 deletions app/services/setting_registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,18 +132,9 @@ def load_definitions
end
end

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)
Comment on lines -135 to +137
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

settings = settings.where('updated_at >= ?', @values_loaded_at) unless ignore_cache || @values_loaded_at.nil?
settings.each do |s|
unless (definition = find(s.name))
Expand Down
4 changes: 2 additions & 2 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Settings in config/environments/* take precedence over those specified here.
# Application configuration should go into files in config/initializers
# -- all .rb files in that directory are automatically loaded.
Expand All @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

config.autoload_paths += %W(#{config.root}/app/models/operatingsystems)
config.autoload_paths += %W(#{config.root}/app/models/parameters)
config.autoload_paths += %W(#{config.root}/app/models/taxonomies)
Expand Down
39 changes: 39 additions & 0 deletions config/initializers/zeitwerk.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
Rails.autoloaders.main.inflector.inflect(
'ui' => 'UI',
'proxy_api' => 'ProxyAPI',
'sti' => 'STI',
'dhcp' => 'DHCP',
'dns' => 'DNS',
'tftp' => 'TFTP',
'external_ipam' => 'ExternalIPAM',
'bmc' => 'BMC',
'ui_notifications' => 'UINotifications',
'ipam' => 'IPAM',
'ssh' => 'SSH',
'ssh_provision' => 'SSHProvision',
'ssh_execution_provider' => 'SSHExecutionProvider',
'keep_current_request_id' => 'KeepCurrentRequestID',
'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'
Comment on lines +16 to +28
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

Rails.autoloaders.main.ignore(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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={}>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails.root.join('lib/core_extensions.rb'),
Rails.root.join('lib/generators')
)
Rails.autoloaders.once.ignore(
Rails.root.join('app/registries/foreman/access_permissions.rb'),
Rails.root.join('app/registries/foreman/settings.rb'),
Rails.root.join('app/registries/foreman/settings')
Comment on lines +36 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered if these shouldn't be initializers.

)
2 changes: 1 addition & 1 deletion db/migrate/20140902102858_convert_ipam_to_string.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class ConvertIpamToString < ActiveRecord::Migration[4.2]
def up
change_column :subnets, :ipam, :string, :default => IPAM::MODES[:dhcp], :null => false, :limit => 255
change_column :subnets, :ipam, :string, :default => ::IPAM::MODES[:dhcp], :null => false, :limit => 255
end

def down
Expand Down
2 changes: 1 addition & 1 deletion test/models/subnet/external_ipam_test.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'test_helper'

class Subnet::ExternalIpamTest < ActiveSupport::TestCase
class Subnet::ExternalIPAMTest < ActiveSupport::TestCase
test 'external ipam is supported for IPv4' do
subnet = FactoryBot.build(:subnet_ipv4)
assert subnet.supports_ipam_mode?(:external_ipam)
Expand Down
2 changes: 1 addition & 1 deletion test/unit/ipam_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class IPAMTest < ActiveSupport::TestCase
fake_proxy = mock("dhcp_proxy")
fake_proxy.stubs(:unused_ip => {'ip' => '192.168.1.25'})
subnet.stubs(:dhcp_proxy => fake_proxy)
ipam = IPAM::Dhcp.new(:subnet => subnet, :mac => '00:11:22:33:44:55')
ipam = IPAM::DHCP.new(:subnet => subnet, :mac => '00:11:22:33:44:55')
assert_equal '192.168.1.25', ipam.suggest_ip
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/unit/report_scanner/puppet_report_scanner_test.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'test_helper'

class PuppetReportScannerTest < ActiveSupport::TestCase
subject { Foreman::PuppetReportScanner }
subject { Foreman::ReportScanner::PuppetReportScanner }

describe '.identify_origin' do
it 'returns Puppet if puppet_report' do
Expand Down