Fixes #37103 - Ruby 3.0 support #743
Conversation
ofedoren
force-pushed
the
ruby3-min
branch
2 times, most recently
from
92dee44
to
868cf48
Compare
| def plan(action_class, targets, *args, concurrency_limit: nil, **kwargs) | ||
| check_targets!(targets) | ||
| limit_concurrency_level!(concurrency_limit) if concurrency_limit | ||
| extracted_concurrency_limit = extract_concurrency_limit(args, concurrency_limit) | ||
| limit_concurrency_level!(extracted_concurrency_limit) if extracted_concurrency_limit |
There was a problem hiding this comment.
I think the concurrency_limit keyword is not used anywhere yet, so we might be able to change the interface if that would help
There was a problem hiding this comment.
Yeah, it would be a bit cleaner. I didn't remove it from the signature with kwargs just in case I missed use case and it is being used somewhere.
Although, from what I found and if I understand the logic right, concurrency_limit: nil, **kwargs is simply ignored due to dynflow doesn't support passing keywords into plan method. Neither foreman-tasks in some places.
There was a problem hiding this comment.
Now I'm confused. It must have worked somehow otherwise the test that is now failing on 2.7 would never pass have passed, no?
|
Jeez.. 2.7 failure is just... such random. I'll rebase the branch just in case :/ @adamruzicka, any idea why it fails?.. |
app/lib/actions/bulk_action.rb
Outdated
| private | ||
|
|
||
| def extract_concurrency_limit(args = [], limit = nil) | ||
| args.find { |arg| arg.is_a?(Hash) && arg.key?(:concurrency_limit) }&.fetch(:concurrency_limit, limit) |
There was a problem hiding this comment.
My Ruby is bad, but doesn't this mean:
- take
args - find the first element in it that is a Hash and has a
concurrency_limitkey - if found, fetch the value from that Hash. if not found, don't do anything as
findreturnedniland the safe operator skips the.fetch
so instead, how about:
| args.find { |arg| arg.is_a?(Hash) && arg.key?(:concurrency_limit) }&.fetch(:concurrency_limit, limit) | |
| args.find { |arg| arg.is_a?(Hash) && arg.key?(:concurrency_limit) }&.fetch(:concurrency_limit) || limit |
There was a problem hiding this comment.
and depending on the Ruby version, calling with (…, concurrency_limit: 25) places that in either args or concurrency_limit and produces 2.7 failures
|
Also, no pressure, but this PR is the only thing needed to be able to deploy (not pass tests, alas) a Katello on EL9 :) |
|
The tests are passing, and Katello seems to work fine with these changes (even bats is passing once the Ruby3 patch for Katello is applied) \o/ |
No description provided.