Remove method_missing and make Chef::Resource unspecial

[jkeiser]

Implements chef-boneyard/chef-rfc#117

[jkeiser]

@coderanger it looks like this passes all desired specs except Poise; but it doesn't appear to have made Poise any I can't find a set of Poise tests that pass against master of chef. Would you mind looking into that when you get a chance? Or tell me what git sha / tag / branch I should run? Thanks :)

[jkeiser]

@lamont-granquist @danielsdeleo I think this is ready for review. Seems to be consensus so I wouldn't mind being ready to check it in when the RFC is approved next Thursday :)

[lamont-granquist]

Chef::Log.deprecation

[jkeiser]

All comments handled except @lamont-granquist's one about LWRP class creation (we can chat on Monday, I am likely missing something).

[jkeiser]

I also added in the deprecation warning for Chef::Provider and made sure all providers and resources in Chef have an appropriate provides.

[lamont-granquist]

Okay, not sure what code I was reading Friday, but I missed this. Might be why I don't like returns out of the middle of functions....

I think this might work, but it seems a little roundabout going through the ResourceResolver to look up the resource name. I'm actually a bit surprised that we don't have a bit of inception since the ResourceResolver is depending on all the provides lines getting setup but we don't setup the default provides and slurp in the class_from_file until a few lines later....

[jkeiser]

Hmm, I can find a way to work out the middle-of-function return, and at the least make it clearer why we're skipping so early in the function. The reason we don't set anything up before skipping is that we want to check if it's already there before we waste time setting it up (and chewing through memory). The first time through, nobody has called provides :x_y, so we don't skip.

[jkeiser]

And, if there is a better way to find out whether we are redefining a resource than to use the ResourceResolver, I'm not sure what it is. If we just looked at LWRPs that happened to be defined in the past (which we don't have a list of anyway), we could still redefine resources created in other ways, and at least as written in the description, that is not intended behavior.

[lamont-granquist]

So, is this going to break the windows_package and windows_service cookbook resources which expect to be able to fire up and overwrite the built in classes? And more broadly we support (and have to support) multiple classes handling the same resource in the dsl, and i think whichever one is parsed first will win based on this logic?

[lamont-granquist]

(and i don't really care about the return in the middle that much, i was trolling a completely different PR with that comment...)

[jkeiser]

windows_package and windows_service should be fine, because they will add provides and the package and service classes do not (they rely on the provider mapping). Classes who use provides presently have priority over classes in the provider mapping.

The current logic is, if there are multiple provides the winner is the first in alphabetical order (unless there is an entry in the priority array).

Here's the summary of how it works right now (before this patch). We try all these in order:

1. Resource.provider: If resource.provider is non-nil, we return that.
2. Priority array: If there is a priority array for this platform, we pick the first Provider that responds. Only the last-loaded priority array for this platform is used (so that cookbooks can override the system).
3. Provides?: If any descendant of Provider responds favorably to provides?, we return the first match in alphabetical order by class name.
4. Platform mapping: If the old platform mapping has an entry, we return that. If there are multiple matches, we take the most specific (os first, then platform, then platform version).
5. Chef::Provider::Blah: If we find a Chef::Provider in Chef::Provider::Blah, we return that.

After the patch, behavior does not change except deprecation warnings. However, after Chef N+1, provides will become required for Chef::Resource classes to participate in DSL. When you add provides to a class, it gets an implicit priority bump because it is picked by step 3 instead of step 5. Thus, it now becomes possible (depending on alphabet) for the superclass to be picked where the subclass used to be picked, if you were relying on that implicit ordering.

The smallest suggestion I have to fix that is to modify step 3 so that any Resource or Provider with provides :blah, <anything> is preferred over provides :blah with no arguments or block. I think there are further steps we can take to improve the selection overall, but I think we should probably take those steps separately.

[jkeiser]

Until we're ready to change the priority system, though, I think we should have documentation for people that are encountering this. The blessed way to do it is to use a priority array:

class Chef::Resource::Blarghle
  # Added provides!
  provides :blarghle
end
class Linux::A
  provides :blarghle, os: 'linux'
  # Since I was relying on the real blarghle's priority before, now I need to *specify* it:
  Chef.set_resource_priority_array :blarghle, [Linux::A, Chef::Resource::Blarghle]
end

[jkeiser]

I'm going to add a note about this in the RFC for explicitness.

[coderanger]

A thing to check is what happens if all the providers in the priority array returns false from provides?. Currently it just falls forward to the next phase I think. I use this in a few places for plugins where only the core ones have a priority order.

[jkeiser]

@coderanger yep, we do the same thing as before--it proceeds to the next phase.

[lamont-granquist]

So by adding all the provides lines I'm a bit concerned that you're going to bypass the platform_map for some providers that still require that and they'll fail. The platform_map really needs to be properly emptied out in order to get this done.

[jkeiser]

@lamont-granquist what I actually did was avoid adding the provides lines for things in the provider mapping. I think it can be done as a followon and doesn't need to be in the patch.

The big reason I didn't do it was I wanted to add individual provides to the actual classes, and to do that we would need to add an implicit priority to our mapping similar to the one the provider mapping uses: more specific providers are preferred over more general. So, if A specifies a platform version, and B specifies a platform family, and both match, we prefer A. os > platform_family > platform > platform_version, I would think. But, it's also possible we could do it by adding explicitly to the priority map. I'll tinker when I get a moment.

[lamont-granquist]