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

Rails 6.0 #20778

Merged
merged 18 commits into from
Jan 11, 2021
Merged
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
10 changes: 5 additions & 5 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ gem "pg", :require => false
gem "pg-dsn_parser", "~>0.1.0", :require => false
gem "psych", "~>3.1", :require => false # This can be dropped once we drop ruby 2.5
gem "query_relation", "~>0.1.0", :require => false
gem "rails", "~>5.2.4", ">=5.2.4.4"
gem "rails-i18n", "~>5.x"
gem "rails", "~>6.0.0"
gem "rails-i18n", "~>6.x"
gem "rake", ">=12.3.3", :require => false
gem "rest-client", "~>2.1.0", :require => false
gem "ripper_ruby_parser", "~>1.5.1", :require => false
Expand All @@ -76,7 +76,7 @@ gem "rubyzip", "~>2.0.0", :require => false
gem "rugged", "~>1.1", :require => false
gem "snmp", "~>1.2.0", :require => false
gem "sprockets", "~>3.7.2", :require => false
gem "sqlite3", "~>1.3.0", :require => false
gem "sqlite3", "~>1.4.0", :require => false
gem "sync", "~>0.5", :require => false
gem "sys-filesystem", "~>1.3.4"
gem "terminal", :require => false
Expand Down Expand Up @@ -233,7 +233,7 @@ group :ui_dependencies do # Added to Bundler.require in config/application.rb
manageiq_plugin "manageiq-decorators"
manageiq_plugin "manageiq-ui-classic"
# Modified gems (forked on Github)
gem "jquery-rjs", "=0.1.1.1", :source => "https://rubygems.manageiq.org"
gem "jquery-rjs", "=0.1.1.2", :source => "https://rubygems.manageiq.org"
end

group :v2v, :ui_dependencies do
Expand All @@ -242,7 +242,7 @@ end

group :web_server, :manageiq_default do
gem "puma", "~>4.2"
gem "responders", "~>2.0"
gem "responders", "~>3.0"
gem "ruby-dbus" # For external auth
gem "secure_headers", "~>3.9"
end
Expand Down
5 changes: 4 additions & 1 deletion app/models/classification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,10 @@ def self.add_entries_from_hash(cat, entries)

def validate_uniqueness_on_tag_name
tag_name = Classification.name2tag(name, parent, ns)
exist_scope = Classification.includes(:tag).where(:tags => {:name => tag_name}).merge(Tag.in_region(region_id))
exist_scope = Classification.default_scoped
.includes(:tag)
.where(:tags => {:name => tag_name})
.merge(Tag.in_region(region_id))
exist_scope = exist_scope.where.not(:id => id) unless new_record?

errors.add("name", "has already been taken") if exist_scope.exists?
Expand Down
20 changes: 20 additions & 0 deletions app/models/disk.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,26 @@ class Disk < ApplicationRecord
t.grouping(Arel::Nodes::NamedFunction.new('COALESCE', [t[:size_on_disk], t[:size], 0]))
end)

# A performance improvement was introduced in Rails 6:
#
# https://github.com/rails/rails/commit/cc2d614e
#
# Causes the `present` column in this class to raise the following error:
#
# ActiveRecord::DangerousAttributeError:
# present? is defined by Active Record. Check to make sure that you don't
# have an attribute or method with the same name.
#
# Since there is no whitelist for this method, this attempts to circumvent
# that autogenerated error to allow our previously named column to still work
# properly.
#
def self.dangerous_attribute_method?(name)
return if name == "present?"

super
end
NickLaMuro marked this conversation as resolved.
Show resolved Hide resolved

def self.find_hard_disks
where("device_type != 'floppy' AND device_type NOT LIKE '%cdrom%'").to_a
end
Expand Down
6 changes: 3 additions & 3 deletions app/models/ext_management_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -410,12 +410,12 @@ def self.model_from_emstype(emstype)
def self.short_token
if self == ManageIQ::Providers::BaseManager
nil
elsif parent == ManageIQ::Providers
elsif module_parent == ManageIQ::Providers
# "Infra"
name.demodulize.sub(/Manager$/, '')
elsif parent != Object
elsif module_parent != Object
# "Vmware"
parent.name.demodulize
module_parent.name.demodulize
end
end

Expand Down
20 changes: 20 additions & 0 deletions app/models/guest_device.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,26 @@ class GuestDevice < ApplicationRecord

acts_as_miq_taggable

# A performance improvement was introduced in Rails 6:
#
# https://github.com/rails/rails/commit/cc2d614e
#
# Causes the `present` column in this class to raise the following error:
#
# ActiveRecord::DangerousAttributeError:
# present? is defined by Active Record. Check to make sure that you don't
# have an attribute or method with the same name.
#
# Since there is no whitelist for this method, this attempts to circumvent
# that autogenerated error to allow our previously named column to still work
# properly.
#
def self.dangerous_attribute_method?(name)
return if name == "present?"

super
end

def self.with_ethernet_type
where(:device_type => "ethernet")
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def find_destination_in_vmdb(ems_ref)
end

def vm_model_class
self.class.parent::Vm
self.class.module_parent::Vm
end

def validate_dest_name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def sync
current = configuration_script_payloads.index_by(&:name)

playbooks_in_git_repository.each do |f|
found = current.delete(f) || self.class.parent::Playbook.new(:configuration_script_source_id => id)
found = current.delete(f) || self.class.module_parent::Playbook.new(:configuration_script_source_id => id)
found.update!(:name => f, :manager_id => manager_id)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def encrypt_queue_params(params)
end

def create_in_provider_queue(manager_id, params, auth_user = nil)
parent.find(manager_id) # validation that the manager ID is from EmbeddedAnsible
module_parent.find(manager_id) # validation that the manager ID is from EmbeddedAnsible
action = "Creating #{self::FRIENDLY_NAME}"
action << " (name=#{params[:name]})" if params[:name]
queue(nil, "create_in_provider", [manager_id, encrypt_queue_params(params)], action, auth_user)
Expand Down
2 changes: 1 addition & 1 deletion app/models/manageiq/providers/inflector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ def self.provider_module(klass, original_object = nil)
raise ObjectNotNamespacedError, "Cannot get provider module from non namespaced object #{original_object}"
end

klass.parent == ManageIQ::Providers ? klass : provider_module(klass.parent, klass)
klass.module_parent == ManageIQ::Providers ? klass : provider_module(klass.module_parent, klass)
end
end
2 changes: 1 addition & 1 deletion app/models/metric/ci_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module Metric::CiMixin
end

supports :capture do
metrics_capture_klass = "#{self.class.parent.name}::MetricsCapture".safe_constantize
metrics_capture_klass = "#{self.class.module_parent.name}::MetricsCapture".safe_constantize
unless metrics_capture_klass&.method_defined?(:perf_collect_metrics)
unsupported_reason_add(:capture, _('This provider does not support metrics collection'))
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/metric/ci_mixin/capture.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ def perf_capture_object(targets = nil)
if self.kind_of?(ExtManagementSystem)
self.class::MetricsCapture.new(targets, ext_management_system)
else
self.class.parent::MetricsCapture.new(targets || self, ext_management_system)
self.class.module_parent::MetricsCapture.new(targets || self, ext_management_system)
end
end

Expand Down
27 changes: 26 additions & 1 deletion app/models/miq_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,32 @@ def self.with_roles_excluding(identifier)
end

def self.next_sequence
maximum(:sequence).to_i + 1
# The +(current_scope || self)+ here is a result of a Rails 6.0 deprecation
# warning that is added here:
#
# https://github.com/rails/rails/pull/35280
#
# This was an attempt to fix "leaking scopes", however, in our case, we use
# this method both for our +default_value_for(:sequence)+, and it will get
# used as part of +.create_tenant_group+.
#
# As such, we need both +.current_scope+ for when we want to scope down the
# +.next_sequence+, but also well allow it to be used on a raw +.create+
# without scopes.
#
# Our +.current_scope+ use case can be best described via one our of
# +MiqGroup+ specs:
#
# expect(MiqGroup.next_sequence).to be < 999 # sanity check
#
# FactoryBot.create(:miq_group, :description => "want 1", :sequence => 999)
# FactoryBot.create(:miq_group, :description => "want 2", :sequence => 1000)
# FactoryBot.create(:miq_group, :description => "dont want", :sequence => 1009)
#
# expect(MiqGroup.where("description like 'want%'").next_sequence).to eq(1001)
#
#
(current_scope || self).maximum(:sequence).to_i + 1
Copy link
Member

Choose a reason for hiding this comment

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

I'm kind of surprised we have sequences at all for groups. Do we know what the use case is for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know, but I think this was added by @djberg96 so possibly he can comment? At least the specs were authored by him (pretty sure).

Copy link
Contributor

Choose a reason for hiding this comment

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

@NickLaMuro Not that I recall. Do you have a commit? I couldn't find one in the log with my name for this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@djberg96 woops, I think I mis-associated you with this commit:

41067b9
#18279

And I think @kbrock was actually the one involved:

1eb754c
#5164

Copy link
Member

Choose a reason for hiding this comment

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

Yea, we use sequence for policies, widgets, actions, and groups.

the value didn't seem to matter for groups, just so that it is unique within the user's groups or the region maybe? I think this is where the scope comes into play. And where rails will audibly complain

Also, I'd like to say that we want to remove nil values from the sequence so ordering would be easier and more consistent.

Copy link
Member

Choose a reason for hiding this comment

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

also, this maximum is executing an extra sql query on create. sure would like to get rid of that

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm less interested in how sequences work as opposed to what feature does sequences provide to the end user? (I don't understand it for polcies and actions either, unless that the order or execution in a policy run, but why? Nor for widgets, unless that's a position on a dashboard, but then wouldn't that be defined in the dashboard?)

end

def self.seed
Expand Down
2 changes: 1 addition & 1 deletion app/models/miq_region_remote.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def self.with_remote_connection(host, port, username, password, database, adapte
host = host.to_s.strip
raise ArgumentError, _("host cannot be blank") if host.blank?
if [nil, "", "localhost", "localhost.localdomain", "127.0.0.1", "0.0.0.0"].include?(host)
local_database = ActiveRecord::Base.configurations.fetch_path(Rails.env, "database").to_s.strip
local_database = (ActiveRecord::Base.configurations[Rails.env] || {})["database"].to_s.strip
NickLaMuro marked this conversation as resolved.
Show resolved Hide resolved
if database == local_database
raise ArgumentError, _("host cannot be set to localhost if database matches the local database")
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/miq_server/environment_management.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def check_disk_usage(disks)
end

next unless disk_usage_event
msg = "Filesystem: #{disk[:filesystem]} (#{disk[:type]}) on #{disk[:mount_point]} is #{disk[:used_bytes_percent]}% full with #{ActionView::Base.new.number_to_human_size(disk[:available_bytes])} free."
msg = "Filesystem: #{disk[:filesystem]} (#{disk[:type]}) on #{disk[:mount_point]} is #{disk[:used_bytes_percent]}% full with #{ActionView::Base.empty.number_to_human_size(disk[:available_bytes])} free."
MiqEvent.raise_evm_event_queue(self, disk_usage_event, :event_details => msg)
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/miq_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def self.base_model
end

def self.corresponding_model
parent::Vm
module_parent::Vm
end
class << self; alias_method :corresponding_vm_model, :corresponding_model; end

Expand Down
6 changes: 3 additions & 3 deletions app/models/miq_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,9 @@ def self.settings_name
@settings_name ||=
if self == MiqWorker
:worker_base
elsif parent.try(:short_token)
elsif module_parent.try(:short_token)
# :generic_worker_infra, :generic_worker_vmware
:"#{normalized_type}_#{parent.short_token.underscore}"
:"#{normalized_type}_#{module_parent.short_token.underscore}"
else
# :generic_worker
normalized_type.to_sym
Expand Down Expand Up @@ -552,7 +552,7 @@ def worker_options
end

def self.normalized_type
@normalized_type ||= if parent == Object
@normalized_type ||= if module_parent == Object
name.sub(/^Miq/, '').underscore
else
name.demodulize.underscore
Expand Down
2 changes: 1 addition & 1 deletion app/models/miq_worker/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def poll_method=(val)
end

def self.corresponding_model
parent
module_parent
end

def initialize(cfg = {})
Expand Down
2 changes: 1 addition & 1 deletion app/models/mixins/per_ems_type_worker_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def workers
end

def ems_class
parent
module_parent
end

def emses_in_zone
Expand Down
2 changes: 1 addition & 1 deletion app/models/mixins/per_ems_worker_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module PerEmsWorkerMixin

module ClassMethods
def ems_class
parent
module_parent
end

def all_ems_in_zone
Expand Down
2 changes: 1 addition & 1 deletion app/models/service_ansible_tower.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class ServiceAnsibleTower < Service
alias_method :job_options=, :stack_options=

def launch_job
job_class = "#{job_template.class.parent.name}::#{job_template.class.stack_type}".constantize
job_class = "#{job_template.class.module_parent.name}::#{job_template.class.stack_type}".constantize
options = job_options.with_indifferent_access.deep_merge(
:extra_vars => {
'manageiq' => service_manageiq_env,
Expand Down
2 changes: 1 addition & 1 deletion app/models/vm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def self.corresponding_model
if self == Vm
MiqTemplate
else
parent::Template
module_parent::Template
end
end
class << self; alias_method :corresponding_template_model, :corresponding_model; end
Expand Down
4 changes: 2 additions & 2 deletions app/models/vm_or_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,10 @@ def from_infra_manager?
include StorageMixin

def self.manager_class
if parent == Object
if module_parent == Object
ExtManagementSystem
else
parent
module_parent
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/evm_database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def self.seed_classes(classes)
private_class_method :seed_classes

def self.host
ActiveRecord::Base.configurations.fetch_path(ENV['RAILS_ENV'], 'host')
(ActiveRecord::Base.configurations[ENV['RAILS_ENV']] || {})['host']
end

def self.local?
Expand Down
2 changes: 1 addition & 1 deletion lib/extensions/session_extension.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ class ActionDispatch::Request::Session
include MoreCoreExtensions::Shared::Nested
end

class ActionController::TestSession < Rack::Session::Abstract::SessionHash
class ActionController::TestSession < Rack::Session::Abstract::PersistedSecure::SecureSessionHash
include MoreCoreExtensions::Shared::Nested
end
5 changes: 4 additions & 1 deletion lib/tasks/evm_application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,10 @@ def self.encryption_key_valid?
end

def self.deployment_status
context = ActiveRecord::MigrationContext.new(Rails.application.config.paths["db/migrate"])
migration_dir = Rails.application.config.paths["db/migrate"]
migration_conn = ::ActiveRecord::Base.connection.schema_migration
context = ActiveRecord::MigrationContext.new(migration_dir, migration_conn)

return "new_deployment" if context.current_version.zero?
return "new_replica" if MiqServer.my_server.nil?
return "upgrade" if context.needs_migration?
Expand Down
2 changes: 1 addition & 1 deletion lib/unique_within_region_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def validate_each(record, attribute, value)
return if value.nil? || !record.send("#{attribute}_changed?")

match_case = options.key?(:match_case) ? options[:match_case] : true
record_base_class = record.class.base_class
record_base_class = record.class.base_class.default_scoped
matches =
if match_case
record_base_class.where(attribute => value)
Expand Down
13 changes: 9 additions & 4 deletions spec/lib/miq_expression_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@
it "generates the SQL for an INCLUDES ANY with expression method" do
sql, * = MiqExpression.new("INCLUDES ANY" => {"field" => "Vm-ipaddresses", "value" => "foo"}).to_sql
expected_sql = <<-EXPECTED.strip_heredoc.split("\n").join(" ")
1 = (SELECT 1
1 = (SELECT 1
FROM "hardwares"
INNER JOIN "networks" ON "networks"."hardware_id" = "hardwares"."id"
WHERE "hardwares"."vm_or_template_id" = "vms"."id"
Expand Down Expand Up @@ -510,7 +510,7 @@
it "generates the SQL for multi level contains with a scope" do
sql, _ = MiqExpression.new("CONTAINS" => {"field" => "ExtManagementSystem.clustered_hosts.operating_system-name", "value" => "RHEL"}).to_sql
rslt = "\"ext_management_systems\".\"id\" IN (SELECT \"ext_management_systems\".\"id\" FROM \"ext_management_systems\" " \
"INNER JOIN \"hosts\" ON \"hosts\".\"ems_id\" = \"ext_management_systems\".\"id\" AND \"hosts\".\"ems_cluster_id\" IS NOT NULL " \
"INNER JOIN \"hosts\" ON \"hosts\".\"ems_cluster_id\" IS NOT NULL AND \"hosts\".\"ems_id\" = \"ext_management_systems\".\"id\" " \
Copy link
Member

Choose a reason for hiding this comment

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

I really like how you broke up the other one into <<~EXPECTED...does it make sense to do that here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was trying to avoid making too many changes, and that one specifically was hard to grok in the errors from rspec, so doing that allowed me to see it easier.

That said, not opposed to the idea, was just avoiding making too many large changes when I could to reduce the number of "variables" changed in a single PR.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you...I have no real preference in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will wait then, but leave this comment as unresolved as a secondary reminder that I do want to do it as a follow up.

"INNER JOIN \"operating_systems\" ON \"operating_systems\".\"host_id\" = \"hosts\".\"id\" " \
"WHERE \"operating_systems\".\"name\" = 'RHEL')"
expect(sql).to eq(rslt)
Expand Down Expand Up @@ -542,8 +542,13 @@

it "generates the SQL for a CONTAINS expression with field containing a scope" do
sql, * = MiqExpression.new("CONTAINS" => {"field" => "Vm.users-name", "value" => "foo"}).to_sql
expected = "\"vms\".\"id\" IN (SELECT \"vms\".\"id\" FROM \"vms\" INNER JOIN \"accounts\" ON \"accounts\".\"vm_or_template_id\" = "\
"\"vms\".\"id\" AND \"accounts\".\"accttype\" = 'user' WHERE \"accounts\".\"name\" = 'foo')"
expected = <<-EXPECTED.split("\n").map(&:strip).join(" ")
"vms"."id" IN (SELECT "vms"."id"
FROM "vms"
INNER JOIN "accounts" ON "accounts"."accttype" = 'user'
AND "accounts"."vm_or_template_id" = "vms"."id"
WHERE "accounts"."name" = 'foo')
EXPECTED
expect(sql).to eq(expected)
end

Expand Down
4 changes: 2 additions & 2 deletions spec/lib/rbac/filterer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2082,8 +2082,8 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
vm.tag_with(@tags.values.join(" "), :ns => "*") if i > 0
end

Vm.scope :group_scope, ->(group_num) { Vm.where("name LIKE ?", "Test Group #{group_num}%") }
Vm.scope :is_on, -> { Vm.where(:power_state => "on") }
Vm.scope :group_scope, ->(group_num) { Vm.default_scoped.where("name LIKE ?", "Test Group #{group_num}%") }
Vm.scope :is_on, -> { Vm.default_scoped.where(:power_state => "on") }
end

context ".search" do
Expand Down
Loading