Remove method_missing and make Chef::Resource unspecial

[jkeiser]

• Use explicit methods for Chef DSL instead of method_missing
• Remove special treatment of classes that happen to be under the Chef::Resource namespace

cc @coderanger @lamont-granquist

[jkeiser]

Note: most of the deprecation feedback is based on edges discovered during the implementation at https://github.com/chef/chef/tree/jk/missing_method_missing . That branch still needs to handle Chef::Provider and add a number of deprecation (and stack trace) tests.

[coderanger]

I would rather never create them for descendants per se, just should be any class that calls Chef::Resource.provides, no matter what that class is. We already have enough issues with descendant tracking for providers as is :-( I think that's basically what you mean though.

[jkeiser]

I'm actually just using def self.inherited on Chef::Resource, which seems to do the job. I'd like descendant tracking to go away, but we have to do this in chess moves, or we get RFCs like the last one :)

[jkeiser]

As far as the everyone vs. no one decision, here are my thoughts:

1. I agree with you from a cleanliness standpoint: everyone should call requires
2. From a practical standpoint this would make a lot of resources stop working
3. I think that, as you have proposed in the past, the real Resource base should be a mixin that we will create later. That is the point at which we get to do things that really cut backcompat deeply, because it won't affect existing Chef::Resource derived stuff.

[coderanger]

Recording the name on .inherited is fine, as long as the system recognizes calling provides manually later and removes the very-briefly set incorrect name. As it stands, the NodeMap code doesn't do that.

[coderanger]

Also because I have to say it, "This won't be thread safe" but I don't think anyone cares about multi-threading the initial cookbook loading process yet, so as long as we all remember it should be fine :-)

[jkeiser]

Believe me when I say I am keenly aware :) I am hoping we can get to the point where separate chef runs can go in separate threads, among other things. I am incredibly uncomfortable with our use of globals to store things that users do in a cookbook in a single chef run. But, baby steps ...

[jkeiser]

@coderanger I modified the provides / does_not_provide sections, and provided an overview of what would be affected in Chef 12 (couple of extra features, and deprecation warnings).

[lamont-granquist]

Either you should use a cleanroom style class instead or Chef::DSL::Recipe should be converted to a cleanroom style class. Probably the latter.

[jkeiser]

The actual solution I implemented was a module named Chef::DSL::Resources, which I include into Chef::DSL::Recipe. But I'm unclear what the purpose of a cleanroom style class is here, and whether that solution addresses what you are talking about? Can you clarify?

[coderanger]

So I think we have hit a fork in the road. If we want to remove method_missing that is doable but will require the eventual removal of the provides? scanning feature for resources. If we are not okay with that feature loss, then we can drastically simplify this proposal to basically just make provides define a method in the DSL module and nothing more since it is an opt-in feature to get slightly nicer tracebacks.

[jonlives]

I'm broadly in favour of this RFC, and my initial thinking was that removing method_missing would provide a neater eventual solution, but after reading @lamont-granquist's points I need to grok a little better how this breaks the overall resource / provider symmetry.

[jkeiser]

It's not necessary to remove provides? or break resource / provider symmetry around that; it may be necessary to modify the rules around it, however.

[jkeiser]

Specifically, the rule could be: (Edit: it turns out this rule is basically in there, in an even more relaxed version; it's just implicit and supports provides? on anything as long as DSL is declared anywhere.)

provides required in order for provides? to be called

In order to be considered a potential provider or resource for the 'my_resource' DSL, a user must call provides 'dsl_name'. This means that anyone overriding provides? to provide a finite set of DSL can still do so.

It means, however, that someone who overrides provides? to provide an infinite set of DSL cannot. For example, if you say "I provide ALL DSL on the Solaris platform, you can't. If you say "I provide sql_<database_name>_<table_name>", you cannot.

This has the advantage that, after scanning all resource classes, you know with certainty the full set of DSL, the full interface for a cookbook.

[jkeiser]

Actually, the RFC is even more relaxed than that, without breaking anything: provides? scanning still happens on both resources and providers. However, we don't try to do provides? scanning unless someone has declared that the DSL method exists with provides. While I think we should eventually restrict it I don't think we should change the RFC on that point in particular; going any further is unnecessary to achieve the aims.

If we are OK with the rule "you must declare the name of a DSL in a provides somewhere," but you may use custom rules in any resource and provider class to implement it," then we're OK for the moment.

[jkeiser]

The final thing we could do--which would still be a big net gain in my opinion--is not deprecate method_missing at all, but still implement explicit methods everywhere. All of the benefits we get here, we would continue to get. The system might be hard to understand in cases where you do a hidden provides? with a name that was never declared as DSL anywhere, but eh, you kinda asked for that complexity when you did something like that. It wouldn't break anything (since we still do provides? scanning in any case, even with explicit DSL, at the moment).

I'll admit I'm not a fan of this approach, and would rather not change to it, but I'll definitely take it if we feel that being able to say essentially provides * and provides /sql_.*_.*/ in an implicit method is important. I'm much more a fan of moving towards a world where the set of potential resources and providers for given DSL is known. (This RFC does not implement that world! But it does move us towards it in a big way, in the sense that it could be known if we collected it.)

[jkeiser]

OK, here's the full description of the issue we're discussing, for those following.

Right now, the Provides system has a provides? method on every single resource and provider. When you call a DSL method, it goes through all Resources and Providers (descendants of Chef::Resource and Chef::Provider) anywhere, and calls provides? on each one. So you can declare a super_file resource by just overriding that method and having it return true if resource_name is "super_file":

class MySuperFileResource < Chef::Resource
  def provides?(node, resource_name)
    resource_name == 'super_file'
  end
end
class MySuperFileProvider < Chef::Resource
  def provides?(node, resource_name)
    resource_name == 'super_file'
  end
end

In Chef 13, with this proposal, typing super_file in a recipe would no longer work. You would need to do this instead:

class MySuperFileResource < Chef::Resource
  provides 'super_file'
end
class MySuperFileProvider < Chef::Resource
  provides 'super_file'
end

This isn't too onerous--provides even takes a block that passes in the node, so you can do any arbitrary logic you want to do to decide whether you handle the resource. Where it gets tricky is this:

class MySuperFileProvider < Chef::Resource
  def provides?
    true # provides any and all resources, because it is THAT awesome
  end
end

Or other arbitrary criteria that allow for arbitrary names. provides 'name' doesn't support that. We would be causing those cases to be impossible. I'm personally OK with it. I'm uncomfortable with the way Rails does this with ActiveRecord, and at any rate, I don't think we intended it, it just happened as a side effect and no one is actually using it this way. If we add the feature in later, we should do so intentionally.

cc @jonlives

[jaymzh]

I'm with you @jkeiser - providing a regex of DSL entries seems crazytown to me.

[jkeiser]

@coderanger @lamont-granquist one more question: once we deprecate the implicit magic entirely and only support provides, Resource.dsl_name and LWRPBase.resource_name are in a ... weird place. It feels like we should deprecate them entirely, and rely on build_resource to tell the class what its name is. What say you?

[jkeiser]

@coderanger @lamont-granquist just wanted to check whether we're in good shape on this or if there's anything else we need to hash out before tomorrow. I've got a working patch I'd like to get in soonish, so that it's cleared out and we can move on to next things.

As to the last question, I modified the RFC to deprecate implicit provides. I did not modify dsl_name and resource_name, but I'll take a shot at all that in a future RFC (when we start getting super explicit about our public API / possibly a mixin).

[coderanger]

Might want to be clearer that this will be removed in Chef 13.

[coderanger]

@jkeiser I think we should probably just get rid of provides_nothing at this point, it is extra stuff to maintain and that isn't a use case that is currently supported (defining a class under Chef::Resource that should not be part of the DSL). If you want to make an abstract base class just put it in your own namespace and it works as expected (no call to provides means no DSL)?

[jkeiser]

Yeah. I'm fine with removing provides_nothing now that we're not doing anything automatic. I'll do that. And yes, if you make an abstract base class outside of Chef::Resource, and don't specify provides, you get no DSL.

[lamont-granquist]

Yeah, provides? can be less lazy and can be moved somewhere right after we first build the node. The right solution to this probably does away with descendent tracking completely and requires every provider that uses dynamic resolution to have a provides? line. We should also be able to refactor into using a single node map at the same time -- although we might need to be careful with handling the case of having multiple providers that can be returned by dynamic resolution with the single node_map (I'll really need to sit down with the code to figure out that, not sure it works in my head).

I'd like to see all of that internal cleanup get done. @jkeiser if your branch doesn't cover all of that, you should probably complete what you can get done so that I can patch on top of that.

[jkeiser]

Yeah, I spiked on getting everything into a single node_map, but it wasn't necessary to get this done so I didn't finish. We can definitely chat about that.

[lamont-granquist]

Yeah, in the shower today I became not sure that works the way that I want. Generally would be good to push your code and start to more collaboratively hack on stuff.

[jkeiser]

@lamont-granquist @coderanger note: I wrote up a new implication of forcing people to use provides that wasn't clear before: Chef::Resource::FooBar has a lower priority than Chef::Resource::FooBar with provides :foo_bar. Adding explicit provides bumps it up, putting it on even footing with any other class that declares provides (and we resolve conflicts the way I pick restaurants: alphabetically). the user is supposed to make anyway: set a priority array.

I think there are ways we could simplify the resolvers by modifying the rules, but I think that merits a whole separate topic and RFC.

@lamont-granquist I think I have enough data now to write out the thinking about node maps and such, in a way that's intelligible enough to detect problems before we spike. I'll write it out as soon as I can manage, and we should find time to talk.

[jonlives]

@chef/rfc-editors this one's decided and ready to rock too!

[nathenharvey]

ping @chef/rfc-editors can we get this merged before tomorrow's meeting, please?

[lamont-granquist]

yeah code is merged and this is gonna ship soonish.