Chef 12.3.0 package provider override not working

[irvingpop]

I'm using chef-server-ingredient and supplying a package source, which should pass provider Chef::Provider::Package::Rpm to the package resource given the following code:

• https://github.com/chef-cookbooks/chef-server-ingredient/blob/v0.3.2/libraries/chef_server_ingredients_provider.rb#L57
• https://github.com/chef-cookbooks/chef-server-ingredient/blob/v0.3.2/libraries/helpers.rb#L44

Despite that, it's using yum_package which is giving me the same issues as #3059

[2015-06-03T16:48:42-04:00] DEBUG: resources for generic package resource enabled on node include: [Chef::Resource::YumPackage]
[2015-06-03T16:48:42-04:00] DEBUG: resources that survived replacement include: [Chef::Resource::YumPackage]
[2015-06-03T16:48:42-04:00] DEBUG: resources for generic package resource enabled on node include: [Chef::Resource::YumPackage]
[2015-06-03T16:48:42-04:00] DEBUG: resources that survived replacement include: [Chef::Resource::YumPackage]

    * packagecloud_repo[chef/stable] action add[2015-06-03T16:48:42-04:00] INFO: Processing packagecloud_repo[chef/stable] action add (/var/chef/cache/cookbooks/chef-server-ingredient/libraries/chef_server_ingredients_provider.rb line 48)
 (skipped due to only_if)
[2015-06-03T16:48:42-04:00] DEBUG: Skipping packagecloud_repo[chef/stable] due to only_if ruby block
    * yum_package[supermarket] action install[2015-06-03T16:48:42-04:00] INFO: Processing yum_package[supermarket] action install (/var/chef/cache/cookbooks/chef-server-ingredient/libraries/chef_server_ingredients_provider.rb line 53)
[2015-06-03T16:48:42-04:00] DEBUG: yum_package[supermarket] checking rpm status
supermarket 1.10.1~alpha.0-1.el5
[2015-06-03T16:48:45-04:00] DEBUG: yum_package[supermarket] checking install state
package supermarket is not installed
[2015-06-03T16:48:45-04:00] DEBUG: yum_package[supermarket] supermarket not installed, installing
[2015-06-03T16:48:45-04:00] DEBUG: yum_package[supermarket] is already installed - nothing to do

I added a ChefSpec test to my cookbook which currently fails because it expects rpm_package to be used, not yum_package: https://github.com/irvingpop/supermarket-omnibus-cookbook/pull/8/files#diff-9f91e0135de8f3ef2e8493be0f54648bR112

[lamont-granquist]

The way its using the provider attribute on a package resource is wrong. You want to refactor that so that it builds an rpm_package or yum_package resource explicitly instead. What you're getting is an rpm_package provider wrapped in a yum_package resource which is displaying 'yum_resource'. This was always bad practice and its become a worse practice with Chef 12

[irvingpop]

cc: @jtimberman

I think have some ideas for fixing this in chef-server-ingredient

[irvingpop]

hey @lamont-granquist I don't know if it's wrong or not, but this is a pattern I've seen in many other cookbooks and it had been working well enough.

I don't think it's fair to simply close this issue saying the usage is wrong. Why would package allow a provider argument if it's wrong? where's the deprecation warning? why does our documentation say this is an OK thing to do?

[irvingpop]

@lamont-granquist I owe you an apology, I shouldn't have gotten testy in my previous comment. I filed a PR against chef-server-ingredient which I believe will get my customer unblocked.

What I should have said is: provider is a documented feature of the package resource and it's widely used in the field by our customers. We should continue to support it until it is deprecated (with generous warning to cookbook authors)

[irvingpop]

what I see now is that it really was using rpm_package all along - but I was thrown off by the yum_package in the messages. We should fix that at, but definitely not a huge priority.

[lamont-granquist]

Using provider isn't totally deprecated, so I wouldn't want to add a deprecation warning, but it remains that this logic:

my_provider = if whatever?
  Chef::Provider::Package::Yum
else
  Chef::Provider::Package::Rpm
end

package "whatever" do
  provider my_provider
end

was ALWAYS inferior to this logic:

if whatever?
  yum_package "whatever"
else
  rpm_package "whatever"
end

In Chef 10+11 you got a Chef::Resource::Package wrapping a Chef::Provider::Package::Yum (or ::Rpm) which meant that any validation and additional properties in Chef::Resource::YumPackage could not be used as well. This was also a problem on the command line where the typical use case of writing:

package "foo"

Always resulted in this case so that on a CentOS box doing yum-specific arguments in package would fail and if you went down the road of trying to pass a Chef::Resource::YumPackage argument then you would also need to know that you needed to change the resource you typed to yum_package to get it to work.

In Chef 12 this common case is fixed so that yum-specific properties just work because you really get a Chef::Resource::YumPackage when you type that on a CentOS box in the DSL. That's better for the much more common case for more users. And this is reflected in the output where you're getting yum_package[whatever] now instead of package[whatever] now.

In the process this made passing the provider argument worse and more confusing, but that was always bad, you were never really getting what you thought you were, and its a leaky abstraction and very tight coupling to the class tree internals of the chef-client (the fact that Chef::Provider::Package::Yum is in so much cookbook code means that class is now fixed in stone and we could never change it if we wanted to).

By going in through the DSL in the second case, you instead correctly specify the real RESOURCE that you want and not just the PROVIDER. You also avoid the tight coupling and you're not embedding internal class names in your own code. You are also going through all the resource resolver and provider resolver logic which converts the typed DSL code 'yum_package' into the correct resources and providers inside of the chef-client.

The use case of hacking up the provider like that is bad code practice. Arguably we should at least put in a foodcritic rule here to start to stop this practice, but I just don't want to fight that battle yet. But the fact that I don't have the energy to start the argument about deprecating this feature doesn't mean its a feature and doesn't mean that there's any bug to get fixed here. It just means that lots of people are writing very bad code with unsupportable edge cases.

So that's the whole story, and the TL;DR is that this really is a WONTFIX. There is no bug to fix here, its working as designed. The use of the provider property is the issue and that really does need to get fixed, because it NEVER worked (and CANNOT work) the way people assumed it did.

[lamont-granquist]

Actually this is the better code in place of using the provider property:

my_resource = rhel? ? :yum_package : :apt_package
declare_resource my_resource, "lsof" do
  action :upgrade
end

That uses symbols instead of classes, and you can keep roughly the same kind of shape as the existing code, but it uses an argument to declare_resource rather than a method on an already built resource of the wrong type.

There should be a FC rule to replace the use of provider with that pattern instead.

[irvingpop]

Thanks @lamont-granquist - that explanation was super helpful! I didn't know about using declare_resource, that is awesome and quite readable. I think it might be worth a blog post to introduce it, I didn't find any documentation or articles mentioning it 😉

[lamont-granquist]

Yeah, dan added that and build_resource back in 11.10.0 and it cleaned up the DSL generation code as well as gave a really useful method to do things like that, but nobody took it over the line in terms of getting the word out.