-
Notifications
You must be signed in to change notification settings - Fork 73
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
Support ruby 3 #509
Conversation
cc @Fryguy do you have opinions about using the |
@@ -201,11 +201,11 @@ def datastore | |||
def ldap | |||
end | |||
|
|||
def execute(method_name, *args) | |||
ruby2_keywords def execute(method_name, *args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: try removing ruby2_keywords and see if I can change options hash methods in MiqAeServiceMethods
to use kwargs and use **
in caller delegates such as this.
(method_name, *args, **kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've dropped ruby2_keywords and found a temporary solution I'm fine with. We basically accept either kwargs/options hashes coming into the execute methods but call the underlying method with only args appended with a hash of kwargs if kwargs exist.
I had to change one method to do no longer accept kwargs and instead pass an options hash.
All kwargs coming into execute_with_user are appended to the args and sent along so all methods in MiqAeServiceMethods can know that there are no kwargs coming in. Converted an existing method to expect an options hash. In the future, we can drop this and go back to kwargs when we drop 2.x. Ensure kwargs are explicit as hashes are no longer automatically converted to kwargs.
@@ -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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes two bugs:
- args is always an array because it's splatted
- 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.
# 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) |
There was a problem hiding this comment.
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.
@@ -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"]}) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -7,18 +7,29 @@ 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, *args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method now accepts being called from execute_with_user
, which will take the kwargs and append them to the args and send them along here. Additionally, it accepts kwargs as it previously did by using any kwargs that are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this changes the interface of this method but doesn't break callers which used the original kwargs interface.
Change
execute_with_user
to capture any kwargs passed as a hash and append to args passed in and call theMiqAeServiceMethods
method with these args but never send over the kwargs. Therefore allMiqAeServiceMethods
methods can be made to only accept options hashes and never kwargs. This works with ruby 2.6-3.0+. We can change this when we drop ruby 2.