Rebuild HDM for Webpack 5

[evgeni]

No description provided.

[ekohl]

I wonder if this has to do with our recent changes around host STI:

ArgumentError: wrong number of arguments (given 1, expected 2)
/builddir/build/BUILD/foreman_hdm-0.1.4/usr/share/foreman/app/models/concerns/belongs_to_proxies.rb:17:in `register_smart_proxy'
/builddir/build/BUILD/foreman_hdm-0.1.4/usr/share/foreman/app/models/host.rb:43:in `method_missing'
/builddir/build/BUILD/foreman_hdm-0.1.4/usr/share/foreman/app/registries/foreman/plugin.rb:509:in `smart_proxy_for'
/builddir/build/BUILDROOT/rubygem-foreman_hdm-0.1.4-2.fm3_10.el8.x86_64/usr/share/gems/gems/foreman_hdm-0.1.4/lib/foreman_hdm/engine.rb:34:in `block (2 levels) in <class:Engine>'

It's this part: https://github.com/betadots/foreman_hdm/blob/01454d0149dd9ec1b0153ff36ef14ac519d9a9a2/lib/foreman_hdm/engine.rb#L34-L38

Looking at https://github.com/theforeman/foreman/blob/262b5caceab9c3f71d0b80f189cc724389593fb0/app/registries/foreman/plugin.rb#L506 I think that needs to be **options instead.

But really, the difference I see is that other classes use the concrete class Host::Managed instead of the abstract Host.

cc @ofedoren

[ofedoren]

Yeap, it's related. Since method_missing in app/models/host tries to understand what is being called with which arguments by looking at the signature of the "callee". The original definition is def smart_proxy_for(klass, name, options), but that plugin passes keyword arguments instead of a hash, thus it's failing :/

Again, new changes in method_missing are focused on restriction of kw <-> hash argument abuse. But looking again at Ruby 3.2 it seems that it's still "valid" to pass keyword arguments even if a method doesn't explicitly accept them. They are just being converted to a hash or ignored...

[ekohl]

The original definition is def smart_proxy_for(klass, name, options), but that plugin passes keyword arguments instead of a hash, thus it's failing :/

Looks like both are done in practice: foreman_monitoring passes a hash because it's used for both Host::Managed and Hostgroup but others (like foreman_hdm, but also foreman_discovery) pass it as keyword arguments.

[oneiros]

The original definition is def smart_proxy_for(klass, name, options), but that plugin passes keyword arguments instead of a hash, thus it's failing :/

I do not believe this assessment is correct. If our call to smart_proxy_for was faulty, the exception would be raised earlier.

Also, this works even in the latest ruby 3.3:

3.3.0 :001 > def smart_proxy_for(klass, name, options)
3.3.0 :002 >   p options
3.3.0 :003 >   p options.class
3.3.0 :004 > end
 => :smart_proxy_for 
3.3.0 :005 > smart_proxy_for nil, nil, test: 23
{:test=>23}
Hash
 => Hash

As you can see, the keyword arguments still get converted to a Hash. So when smart_proxy_for triggers method_missing in Host it does pass a Hash, not keyword arguments.

[oneiros]

I dug a little deeper and indeed, if I change our call to smart_proxy_for to use a Hash explicitly, I still get the exception.

The problem seems to be, that if you have the following signature for method_missing:

def self.method_missing(method, *args, **kwargs, &block)

and then call a method with a Hash as the last argument, then it lands in kwargs instead of args ☹️

[oneiros]

FWIW, ekohl was right above, if we use Host::Managed in place of just Host, the problem goes away.

If it is safe to do so, I am happy to make that change.

[ekohl]

If it is safe to do so, I am happy to make that change.

It means you can no longer support discovered hosts (from https://github.com/theforeman/foreman_discovery), but I think that's actually correct. You don't run Puppet on those, since they're not provisioned yet.

[evgeni]

You probably could use Host::Base in that case… but I agree, you shouldn't need to support those

[oneiros]

Thank you all for getting back to me so quickly. I created a PR in our repository that should fix the issue.

[evgeni]

#10577 had an updated version that built fine