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

Support ruby 3 #509

Merged
merged 5 commits into from
Sep 12, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def self.miq_log_workspace(obj, _inputs)
end

def self.miq_send_email(_obj, inputs)
MiqAeMethodService::MiqAeServiceMethods.send_email(inputs["to"], inputs["from"], inputs["subject"], inputs["body"], :cc => inputs["cc"], :bcc => inputs["bcc"], :content_type => inputs["content_type"])
MiqAeMethodService::MiqAeServiceMethods.send_email(inputs["to"], inputs["from"], inputs["subject"], inputs["body"], {:cc => inputs["cc"], :bcc => inputs["bcc"], :content_type => inputs["content_type"]})
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 needed to change this as the callee's interface was changed to support ruby 2.6-3.0+ so it no longer accepts keyword arguments directly. Notice, this is because execute_with_user below now captures any kwargs as a hash and appends that to the args so no kwargs are sent to the callee in MiqAeServiceMethods.

Copy link
Member

Choose a reason for hiding this comment

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

A little surprised that you needed this change.
Seems it would have been converted to a hash automatically in ruby

Copy link
Member Author

Choose a reason for hiding this comment

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

On ruby 2/3, the send_email interface is 5 arguments via this PR. This call was passing only 4 on ruby 3 as kwargs are not an "argument".

Copy link
Member Author

@jrafanie jrafanie Sep 9, 2022

Choose a reason for hiding this comment

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

I can try to avoid changing the interface of send_email and see if I can "fix" the incoming kwargs/args to work with ruby 2/3. I thought I tried it but now that there are tests, we can experiment.

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 now stale. I figured out a way to fix send_email to accept kwargs or an options hash and treat it differently since it's really the only one doing kwargs.

end

def self.miq_snmp_trap_v1(_obj, inputs)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,20 @@ def datastore
def ldap
end

def execute(method_name, *args)
User.with_user(@workspace.ae_user) { execute_with_user(method_name, *args) }
def execute(method_name, *args, **kwargs, &block)
User.with_user(@workspace.ae_user) { execute_with_user(method_name, *args, **kwargs, &block) }
end

def execute_with_user(method_name, *args)
def execute_with_user(method_name, *args, **kwargs, &block)
# Since each request from DRb client could run in a separate thread
# We have to set the current_user in every thread.
MiqAeServiceMethods.send(method_name, *args)

# For ruby 2.6-3.0+ support, we grab any kwargs and append as a hash
# at the end of the args, as all MiqAeServiceMethods now don't accept
# kwargs. When we get to ruby 3, we can remove this and convert all to
# kwargs.
args << kwargs unless kwargs.blank?
MiqAeServiceMethods.send(method_name, *args, &block)
Copy link
Member Author

@jrafanie jrafanie Sep 8, 2022

Choose a reason for hiding this comment

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

This is really the biggest change. We capture the kwargs from ruby 2.7/3 and pass them along as a hash to the end of the args. Then, we can keep all of the existing methods as accepting options hashes except for the one that was accepting kwargs, which I changed to accept either kwargs or options.

There isn't another way to handle the situation in which existing automate code is calling execute with :a => as that will be kwargs on ruby 3 even if we expect hashes.

Once we're on ruby 3+ only, we can use **kwargs and always pass along kwargs and all the methods can be made to accept kwargs and convert to hashes if they need to send down hashes to callees.

rescue NoMethodError => err
raise MiqAeException::MethodNotFound, err.message
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ class MiqAeServiceMethods

SYNCHRONOUS = Rails.env.test?

def self.send_email(to, from, subject, body, content_type: nil, cc: nil, bcc: nil) # rubocop:disable Naming/MethodParameterName
def self.send_email(to, from, subject, body, options = {}) # rubocop:disable Naming/MethodParameterName
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 method needed to be changed to no longer accept kwargs as the caller in execute_with_user will append the kwargs to the incoming arguments as a hash so no kwargs will be sent. I also need to change miq_send_email to no longer send kwargs as it bypassed execute_with_user.

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 now stale, I figured out a way to treat this method differently and accept kwargs or options hashes so we don't need to break existing callers using kwargs.

ar_method do
meth = SYNCHRONOUS ? :deliver : :deliver_queue
options = {
:to => to,
:from => from,
:cc => cc,
:bcc => bcc,
:cc => options[:cc],
:bcc => options[:bcc],
:subject => subject,
:content_type => content_type,
:content_type => options[:content_type],
:body => body
}
GenericMailer.send(meth, :automation_notification, options)
Expand Down Expand Up @@ -144,7 +144,7 @@ def self.create_provision_request(*args)
# Need to add the username into the array of params
# TODO: This code should pass a real username, similar to how the web-service
# passes the name of the user that logged into the web-service.
args.insert(1, User.lookup_by_userid("admin")) if args.kind_of?(Array)
args << User.lookup_by_userid("admin")
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 fixes two bugs:

  1. args is always an array because it's splatted
  2. it inserted the user as the second element in the array, so, if you had an empty array, it would return [nil, user] on this line.

MiqAeServiceModelBase.wrap_results(MiqProvisionVirtWorkflow.from_ws(*args))
end

Expand Down
246 changes: 200 additions & 46 deletions spec/engine/miq_ae_method_service/miq_ae_service_methods_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ def invoke_ae
end

it "sends mail synchronous" do
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:send_email, #{options[:to].inspect}, #{options[:from].inspect}, #{options[:subject].inspect}, #{options[:body].inspect}, :bcc => #{options[:bcc].inspect}, :cc => #{options[:cc].inspect}, :content_type => #{options[:content_type].inspect})"
@ae_method.update(:data => method)
stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(GenericMailer).to receive(:deliver).with(:automation_notification, options).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end

it "sends mail synchronous with options hash (for backwards compatibility with ruby 2.7) - drop when we drop ruby 2.7" do
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:send_email, #{options[:to].inspect}, #{options[:from].inspect}, #{options[:subject].inspect}, #{options[:body].inspect}, {:bcc => #{options[:bcc].inspect}, :cc => #{options[:cc].inspect}, :content_type => #{options[:content_type].inspect}})"
@ae_method.update(:data => method)
stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
Expand All @@ -46,66 +55,116 @@ def invoke_ae
MiqRegion.seed
miq_server.server_roles << FactoryBot.create(:server_role, :name => 'notifier')

method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:send_email, #{options[:to].inspect}, #{options[:from].inspect}, #{options[:subject].inspect}, #{options[:body].inspect}, {:bcc => #{options[:bcc].inspect}, :cc => #{options[:cc].inspect}, :content_type => #{options[:content_type].inspect}})"
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:send_email, #{options[:to].inspect}, #{options[:from].inspect}, #{options[:subject].inspect}, #{options[:body].inspect}, :bcc => #{options[:bcc].inspect}, :cc => #{options[:cc].inspect}, :content_type => #{options[:content_type].inspect})"
@ae_method.update(:data => method)
stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
expect(MiqQueue).to receive(:put).with({
:class_name => 'GenericMailer',
:method_name => "deliver",
:args => [:automation_notification, options],
:role => "notifier"
).once
}).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end
end

it "#snmp_trap_v1" do
to = "[email protected]"
from = "[email protected]"
inputs = {:to => to, :from => from}
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:snmp_trap_v1, #{inputs.inspect})"
@ae_method.update(:data => method)
context "#snmp_trap" do
it "_v1" do
to = "[email protected]"
from = "[email protected]"
inputs = {:to => to, :from => from}
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:snmp_trap_v1, #{inputs.inspect})"
@ae_method.update(:data => method)

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(MiqSnmp).to receive(:trap_v1).with(inputs).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
:class_name => "MiqSnmp",
:method_name => "trap_v1",
:args => [inputs],
:role => "notifier",
:zone => nil
).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end
stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(MiqSnmp).to receive(:trap_v1).with(inputs).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy

it "#snmp_trap_v2" do
to = "[email protected]"
from = "[email protected]"
inputs = {:to => to, :from => from}
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:snmp_trap_v2, #{inputs.inspect})"
@ae_method.update(:data => method)
stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
:class_name => "MiqSnmp",
:method_name => "trap_v1",
:args => [inputs],
:role => "notifier",
:zone => nil
).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(MiqSnmp).to receive(:trap_v2).with(inputs).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
:class_name => "MiqSnmp",
:method_name => "trap_v2",
:args => [inputs],
:role => "notifier",
:zone => nil
).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
it "_v2" do
to = "[email protected]"
from = "[email protected]"
inputs = {:to => to, :from => from}
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:snmp_trap_v2, #{inputs.inspect})"
@ae_method.update(:data => method)

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(MiqSnmp).to receive(:trap_v2).with(inputs).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
:class_name => "MiqSnmp",
:method_name => "trap_v2",
:args => [inputs],
:role => "notifier",
:zone => nil
).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end

it "_v1 kwargs" do
to = "[email protected]"
from = "[email protected]"
inputs = {:to => to, :from => from}
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:snmp_trap_v1, **#{inputs.inspect})"
@ae_method.update(:data => method)

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(MiqSnmp).to receive(:trap_v1).with(inputs).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
:class_name => "MiqSnmp",
:method_name => "trap_v1",
:args => [inputs],
:role => "notifier",
:zone => nil
).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end

it "_v2 kwargs" do
to = "[email protected]"
from = "[email protected]"
inputs = {:to => to, :from => from}
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:snmp_trap_v2, **#{inputs.inspect})"
@ae_method.update(:data => method)

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', true)
expect(MiqSnmp).to receive(:trap_v2).with(inputs).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy

stub_const('MiqAeMethodService::MiqAeServiceMethods::SYNCHRONOUS', false)
expect(MiqQueue).to receive(:put).with(
:class_name => "MiqSnmp",
:method_name => "trap_v2",
:args => [inputs],
:role => "notifier",
:zone => nil
).once
ae_object = invoke_ae.root(@ae_result_key)
expect(ae_object).to be_truthy
end
end

it "#vm_templates" do
Expand Down Expand Up @@ -147,6 +206,20 @@ def category_create_script
expect(invoke_ae.root(@ae_result_key)).to be_truthy
end

def category_create_script_kwargs
<<-'RUBY'
options = {:name => 'flintstones',
:description => 'testing'}
$evm.root['foo'] = $evm.execute(:category_create, **options)
RUBY
end

it "#category_create kwargs" do
@ae_method.update(:data => category_create_script)

expect(invoke_ae.root(@ae_result_key)).to be_truthy
end

context "#category_delete!" do
let(:ct) { FactoryBot.create(:classification, :name => "test_category") }

Expand Down Expand Up @@ -255,6 +328,16 @@ def category_create_script
expect(ct.entries.collect(&:name).include?('fred')).to be_truthy
end

it "#tag_create kwargs" do
ct = FactoryBot.create(:classification_department_with_tags)
method = "$evm.root['#{@ae_result_key}'] = $evm.execute(:tag_create, #{ct.name.inspect}, :name => 'fred', :description => 'ABC')"
@ae_method.update(:data => method)

expect(invoke_ae.root(@ae_result_key)).to be_truthy
ct.reload
expect(ct.entries.collect(&:name).include?('fred')).to be_truthy
end

context "#tag_delete!" do
let(:ct) { FactoryBot.create(:classification_department_with_tags) }
let(:entry_name) { ct.entries.first.name }
Expand Down Expand Up @@ -329,6 +412,77 @@ def category_create_script
end
end

context "#create_provision_request" do
let(:workspace) do
double("MiqAeEngine::MiqAeWorkspaceRuntime",
:persist_state_hash => MiqAeEngine::StateVarHash.new,
:ae_user => user)
end
let(:user) { double }
let(:miq_ae_service) { MiqAeMethodService::MiqAeService.new(workspace) }
before do
allow(User).to receive(:lookup_by_userid).and_return(user)
end

it "passes arguments correctly" do
expect(MiqProvisionVirtWorkflow).to receive(:from_ws).with('one', 'two', user).and_return(true)
expect(MiqAeMethodService::MiqAeServiceModelBase).to receive(:wrap_results).and_return(true)
allow(workspace).to receive(:disable_rbac)
miq_ae_service.execute(:create_provision_request, 'one', 'two')
end

it "passes array arguments correctly" do
expect(MiqProvisionVirtWorkflow).to receive(:from_ws).with(['one', 'two'], user).and_return(true)
expect(MiqAeMethodService::MiqAeServiceModelBase).to receive(:wrap_results).and_return(true)
allow(workspace).to receive(:disable_rbac)
miq_ae_service.execute(:create_provision_request, %w[one two])
end

it "passes empty array arguments correctly" do
expect(MiqProvisionVirtWorkflow).to receive(:from_ws).with([], user).and_return(true)
expect(MiqAeMethodService::MiqAeServiceModelBase).to receive(:wrap_results).and_return(true)
allow(workspace).to receive(:disable_rbac)
miq_ae_service.execute(:create_provision_request, [])
end

it "passes nil argument correctly" do
expect(MiqProvisionVirtWorkflow).to receive(:from_ws).with(user).and_return(true)
expect(MiqAeMethodService::MiqAeServiceModelBase).to receive(:wrap_results).and_return(true)
allow(workspace).to receive(:disable_rbac)
miq_ae_service.execute(:create_provision_request)
end
end

context "#create_automation_request" do
let(:workspace) do
double("MiqAeEngine::MiqAeWorkspaceRuntime",
:persist_state_hash => MiqAeEngine::StateVarHash.new,
:ae_user => user)
end
let(:user) { double }
let(:admin_user) { double }
let(:miq_ae_service) { MiqAeMethodService::MiqAeService.new(workspace) }
let(:auto_approve) { true }
let(:options) { {:test => 1} }

it "passes arguments correctly" do
expect(AutomationRequest).to receive(:create_request).with(options, user, auto_approve).and_return(true)
expect(MiqAeMethodService::MiqAeServiceModelBase).to receive(:wrap_results).and_return(true)
allow(workspace).to receive(:disable_rbac)
allow(User).to receive(:lookup_by_userid!).and_return(user)
miq_ae_service.execute(:create_automation_request, options, user, auto_approve)
end

it "passes arguments correctly from defaults" do
expect(AutomationRequest).to receive(:create_request).with(options, admin_user, false).and_return(true)
expect(MiqAeMethodService::MiqAeServiceModelBase).to receive(:wrap_results).and_return(true)
allow(workspace).to receive(:disable_rbac)
allow(User).to receive(:lookup_by_userid!).and_return(admin_user)

miq_ae_service.execute(:create_automation_request, options)
end
end

context "#create_service_provision_request" do
let(:options) { {:fred => :flintstone} }
let(:svc_options) { {:dialog_style => "medium"} }
Expand Down
6 changes: 3 additions & 3 deletions spec/miq_ae_engine_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def call_automate(obj_type, obj_id, open_url_task_id = nil)
object_id = cluster.id
automate_attrs = {"#{object_type}::#{object_type.underscore}" => object_id,
"User::user" => user.id}
expect(MiqAeEngine).to receive(:create_automation_object).with(instance_name, automate_attrs, :vmdb_object => cluster).and_return('uri')
expect(MiqAeEngine).to receive(:create_automation_object).with(instance_name, automate_attrs, {:vmdb_object => cluster}).and_return('uri')
expect(call_automate(object_type, object_id)).to be_nil
end

Expand All @@ -90,7 +90,7 @@ def call_automate(obj_type, obj_id, open_url_task_id = nil)
object_id = ems.id
automate_attrs = {"#{base_name}::#{base_name.underscore}" => object_id,
"User::user" => user.id}
expect(MiqAeEngine).to receive(:create_automation_object).with(instance_name, automate_attrs, :vmdb_object => ems).and_return('uri')
expect(MiqAeEngine).to receive(:create_automation_object).with(instance_name, automate_attrs, {:vmdb_object => ems}).and_return('uri')
expect(call_automate(object_type, object_id)).to be_nil
end
end
Expand Down Expand Up @@ -141,7 +141,7 @@ def call_automate(obj_type, obj_id, open_url_task_id = nil)
args[:fqclass_name] = "Factory/StateMachines/ServiceProvision_template"
args[:user_id] = user.id
args[:miq_group_id] = user.current_group.id
expect(MiqAeEngine).to receive(:create_automation_object).with("DEFAULT", attrs, :fqclass => "Factory/StateMachines/ServiceProvision_template").and_return('uri')
expect(MiqAeEngine).to receive(:create_automation_object).with("DEFAULT", attrs, {:fqclass => "Factory/StateMachines/ServiceProvision_template"}).and_return('uri')
expect(MiqAeEngine.deliver(args)).to eq(@ws)
end
end
Expand Down