New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for default resource name and DSL #136

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
@jkeiser
Member

jkeiser commented Jun 17, 2015

No description provided.

When a resource class is declared with an explicit name, its resource name is set automatically:
```ruby
class SuperAwesome < Chef::Resource

This comment has been minimized.

@coderanger

coderanger Jun 17, 2015

Contributor

Huge 👎 on this, classes outside Chef::Resource should not get names automatically. This would be a new feature on something we want to eventually deprecate anyway.

@coderanger

coderanger Jun 17, 2015

Contributor

Huge 👎 on this, classes outside Chef::Resource should not get names automatically. This would be a new feature on something we want to eventually deprecate anyway.

This comment has been minimized.

@coderanger

coderanger Jun 17, 2015

Contributor

To more specific, using the class name makes way more assumptions about module layout than I think is good. It encourages dumping things in a shared namespace because the resource and provider are thought to have to have the same class name. For most developers that aren't core Chef, this is a terrible way to organize code.

@coderanger

coderanger Jun 17, 2015

Contributor

To more specific, using the class name makes way more assumptions about module layout than I think is good. It encourages dumping things in a shared namespace because the resource and provider are thought to have to have the same class name. For most developers that aren't core Chef, this is a terrible way to organize code.

This comment has been minimized.

@jkeiser

jkeiser Jun 17, 2015

Member

People that want to organize code a different way can still use resource_name to do it literally any way they want to (the default name isn't set in stone).

But many people (I would argue most) will be perfectly fine naming their resources the same as the DSL. I don't think we'd ever want to deprecate this, and it makes the process of writing a new resource that much more delightful (one less bit of ceremony by default).

@jkeiser

jkeiser Jun 17, 2015

Member

People that want to organize code a different way can still use resource_name to do it literally any way they want to (the default name isn't set in stone).

But many people (I would argue most) will be perfectly fine naming their resources the same as the DSL. I don't think we'd ever want to deprecate this, and it makes the process of writing a new resource that much more delightful (one less bit of ceremony by default).

This comment has been minimized.

@coderanger

coderanger Jun 18, 2015

Contributor

So to expand on this a bit from a discussion on IRC (this is going to be a wall of text, bear with me). My concern is that I would like to see The System encourage better layout of the resource and provider classes. Right now we generally have Chef::Resource::Name and Chef::Provider::Name which is gross because the two classes are totally separated. This is a lost cause for those namespaces themselves, but we've started to see some better (IMO) layouts in third-party code. Two notables are my layout (Name::Resource and Name::Provider) and @jkeiser's layout (Name and Name::Provider). I think we should encourage the use of layouts like that by building the name extraction code to support them. It is true you can always set the name manually so every layout is always "possible", but the default behavior sets developer expectations and we have a chance to possibly reshape those here so I think we should :-)

From the two examples I gave, possibly just taking the class name and chopping off a few known words could go a long way. I don't think auto-naming outside Chef::Resource is overall a bad idea, just that I don't think it should use the current behavior.

@coderanger

coderanger Jun 18, 2015

Contributor

So to expand on this a bit from a discussion on IRC (this is going to be a wall of text, bear with me). My concern is that I would like to see The System encourage better layout of the resource and provider classes. Right now we generally have Chef::Resource::Name and Chef::Provider::Name which is gross because the two classes are totally separated. This is a lost cause for those namespaces themselves, but we've started to see some better (IMO) layouts in third-party code. Two notables are my layout (Name::Resource and Name::Provider) and @jkeiser's layout (Name and Name::Provider). I think we should encourage the use of layouts like that by building the name extraction code to support them. It is true you can always set the name manually so every layout is always "possible", but the default behavior sets developer expectations and we have a chance to possibly reshape those here so I think we should :-)

From the two examples I gave, possibly just taking the class name and chopping off a few known words could go a long way. I don't think auto-naming outside Chef::Resource is overall a bad idea, just that I don't think it should use the current behavior.

This comment has been minimized.

@jkeiser

jkeiser Jul 10, 2015

Member

@coderanger I'm in full concurrence with what you are talking about, I think. I could expand this proposal to say something like:

Chef::Resource classes named X::Y::Z or X::Y::Z::Resource get automatic resource name z. Additionally, if no provider is automatically found for the resource, we will look for a Chef::Provider class named X::Y::Z::Provider, and use that.

Chef::Provider::X and Chef::Resource::X will not be considered pairs in this hierarchy, and Chef::Provider::X will not be given an automatic provides line. In fact, no provider will ever be given an automatic provides line; pairing is the only way it will be automatically chosen.

Core Chef providers in Chef::Provider will move to Chef::Resource::X::Provider as well, and alised into the Chef::Provider namespace for backwards compatibility.

The intent here is that Providers should be (by default) rather tightly coupled with Resources (see @lamont-granquist's dreams of 1:1 mapping). In general, unless you actually create multiple providers which match up with the exact same resource class, you should never ever call provides on a provider.

@jkeiser

jkeiser Jul 10, 2015

Member

@coderanger I'm in full concurrence with what you are talking about, I think. I could expand this proposal to say something like:

Chef::Resource classes named X::Y::Z or X::Y::Z::Resource get automatic resource name z. Additionally, if no provider is automatically found for the resource, we will look for a Chef::Provider class named X::Y::Z::Provider, and use that.

Chef::Provider::X and Chef::Resource::X will not be considered pairs in this hierarchy, and Chef::Provider::X will not be given an automatic provides line. In fact, no provider will ever be given an automatic provides line; pairing is the only way it will be automatically chosen.

Core Chef providers in Chef::Provider will move to Chef::Resource::X::Provider as well, and alised into the Chef::Provider namespace for backwards compatibility.

The intent here is that Providers should be (by default) rather tightly coupled with Resources (see @lamont-granquist's dreams of 1:1 mapping). In general, unless you actually create multiple providers which match up with the exact same resource class, you should never ever call provides on a provider.

This comment has been minimized.

@nathwill

nathwill Jul 10, 2015

In general, unless you actually create multiple providers which match up with the exact same resource class, you should never ever call provides on a provider.

How would one provider capable of handling multiple similar resources work in this case? just aliases? curious as i've been doing this a fair amount lately as a way to reduce the amount of effort required (e.g. Chef::Provider::SystemdUnit, which works nicely as a provider for Chef::Resource::{SystemdUnit,SystemdTarget,SystemdTimer,SystemdService...})

@nathwill

nathwill Jul 10, 2015

In general, unless you actually create multiple providers which match up with the exact same resource class, you should never ever call provides on a provider.

How would one provider capable of handling multiple similar resources work in this case? just aliases? curious as i've been doing this a fair amount lately as a way to reduce the amount of effort required (e.g. Chef::Provider::SystemdUnit, which works nicely as a provider for Chef::Resource::{SystemdUnit,SystemdTarget,SystemdTimer,SystemdService...})

This comment has been minimized.

@jkeiser

jkeiser Jul 10, 2015

Member

I would expect provides to be used for any case that isn't 1 to 1.

It might be possible to modify this definition to say it will look at the superclass resource to see if it has a corresponding provider. I can see some pros and cons offhand, but I'm not sure where the balance lies on that.

@jkeiser

jkeiser Jul 10, 2015

Member

I would expect provides to be used for any case that isn't 1 to 1.

It might be possible to modify this definition to say it will look at the superclass resource to see if it has a corresponding provider. I can see some pros and cons offhand, but I'm not sure where the balance lies on that.

This comment has been minimized.

@nathwill

nathwill Jul 10, 2015

ok, cool. thanks for the clarification.

@nathwill

nathwill Jul 10, 2015

ok, cool. thanks for the clarification.

#### Lexical order
Previously, resource matches at the same precedence level involved sorting the classes by name. We will replicate this (though priority arrays with multiple classes will not obey this rule and will be entirely last-declared).

This comment has been minimized.

@coderanger

coderanger Jun 17, 2015

Contributor

For clarity, while this is new in the RFC, the code is already merged to master so this is a retroactive description.

@coderanger

coderanger Jun 17, 2015

Contributor

For clarity, while this is new in the RFC, the code is already merged to master so this is a retroactive description.

This comment has been minimized.

@jkeiser

jkeiser Jul 10, 2015

Member

Yep. We have the opportunity to change it in a future release if we want, but it was necessary for backwards compatibility.

@jkeiser

jkeiser Jul 10, 2015

Member

Yep. We have the opportunity to change it in a future release if we want, but it was necessary for backwards compatibility.

Show outdated Hide outdated new/default-resource-name.md
#### resource_name nil unsets resource name
class.resource_name nil unsets a resource name (and prevents that name from appearing in DSL).

This comment has been minimized.

@coderanger

coderanger Jun 17, 2015

Contributor

This is class.resource_name(nil).

@coderanger

coderanger Jun 17, 2015

Contributor

This is class.resource_name(nil).

This comment has been minimized.

@tyler-ball

tyler-ball Jul 23, 2015

What about using false instead of nil for unsetting? I ask because we have run across a lot of provisioning use cases where we want to unset something (like an attribute/property) and nil communicates "don't change this" because it is the default value.

@tyler-ball

tyler-ball Jul 23, 2015

What about using false instead of nil for unsetting? I ask because we have run across a lot of provisioning use cases where we want to unset something (like an attribute/property) and nil communicates "don't change this" because it is the default value.

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Jun 17, 2015

Contributor

You might want to put some mention of the explicit motivating case in here:

In 12.0.0 the provider resolver dealt with ambiguous provider resolution by sorting the classes and then picking the first one which survived various conditions which mean it was lexicographical by class name. In 12.3.0 a warning was added that this was ambiguous and that the user need to set the priority array explicitly. In 12.4.0.rc.2 due to refactoring this changed so that it was last-parsed-class wins which meant that it was (after cookbook loading order) the last lexicographically sorted library file that was parsed, which in the common case of class names and file names matching and the providers in the same cookbook meant that the sort order was reversed.

This directly affects the git cookbook which declares both source and binary providers that provides the :git_client resource. In 12.0.0 through 12.3.0 this would select GitClient::Binary over GitClient::Source. If we press the trigger today on 12.4.0 this would reverse and users would get the source compiled git installed into /usr/local/bin/git at the version pinned by the default attribute in the cookbook which is 1.9.5 instead of the version in the O/S packages being installed into /usr/bin/git.

This is a cookbook bug, but its also likely a breaking change since the behavior on upgrade is undelightful. We want to preserve the 12.0.0 sort order behavior in this case and preserve a warning similar to 12.3.0

Contributor

lamont-granquist commented Jun 17, 2015

You might want to put some mention of the explicit motivating case in here:

In 12.0.0 the provider resolver dealt with ambiguous provider resolution by sorting the classes and then picking the first one which survived various conditions which mean it was lexicographical by class name. In 12.3.0 a warning was added that this was ambiguous and that the user need to set the priority array explicitly. In 12.4.0.rc.2 due to refactoring this changed so that it was last-parsed-class wins which meant that it was (after cookbook loading order) the last lexicographically sorted library file that was parsed, which in the common case of class names and file names matching and the providers in the same cookbook meant that the sort order was reversed.

This directly affects the git cookbook which declares both source and binary providers that provides the :git_client resource. In 12.0.0 through 12.3.0 this would select GitClient::Binary over GitClient::Source. If we press the trigger today on 12.4.0 this would reverse and users would get the source compiled git installed into /usr/local/bin/git at the version pinned by the default attribute in the cookbook which is 1.9.5 instead of the version in the O/S packages being installed into /usr/bin/git.

This is a cookbook bug, but its also likely a breaking change since the behavior on upgrade is undelightful. We want to preserve the 12.0.0 sort order behavior in this case and preserve a warning similar to 12.3.0

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Jun 17, 2015

Contributor

Relevant git cookbook bug: chef-cookbooks/git#64

Contributor

lamont-granquist commented Jun 17, 2015

Relevant git cookbook bug: chef-cookbooks/git#64

@coderanger

This comment has been minimized.

Show comment
Hide comment
@coderanger

coderanger Jun 17, 2015

Contributor

Yeah, big +1 on having a stable, documented order for the resolution system. One thing that should also probably be address specifically is cookbooks with the same resources as core too, since that has gone pear shaped a few times now.

Contributor

coderanger commented Jun 17, 2015

Yeah, big +1 on having a stable, documented order for the resolution system. One thing that should also probably be address specifically is cookbooks with the same resources as core too, since that has gone pear shaped a few times now.

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser Jul 10, 2015

Member

@coderanger regarding your last comment, can you elaborate on the case of cookbooks with the same resources as core? I don't mind folding it in if it makes sense in context of the RFC.

Member

jkeiser commented Jul 10, 2015

@coderanger regarding your last comment, can you elaborate on the case of cookbooks with the same resources as core? I don't mind folding it in if it makes sense in context of the RFC.

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser Jul 23, 2015

Member

@coderanger added MyThing::Resource -> my_thing

Member

jkeiser commented Jul 23, 2015

@coderanger added MyThing::Resource -> my_thing

@martinb3

This comment has been minimized.

Show comment
Hide comment
@martinb3

martinb3 Jul 30, 2015

Contributor

I feel like anything outside Chef::Resource probably shouldn't get any automagic behavior. I almost always prefer explicit over implicit, especially when someone is already down at the level of writing pure Ruby classes for resources and providers.

Contributor

martinb3 commented Jul 30, 2015

I feel like anything outside Chef::Resource probably shouldn't get any automagic behavior. I almost always prefer explicit over implicit, especially when someone is already down at the level of writing pure Ruby classes for resources and providers.

@coderanger

This comment has been minimized.

Show comment
Hide comment
@coderanger

coderanger Jul 30, 2015

Contributor

Summary from rambling on IRC: -1 on this as I would rather see explicit naming. More concretely this also doesn't apply to providers at all, so in all cases those would still need to be explicit when outside the Chef::Provider namespace, so even when using an implicit name you need to do the mental work to know what the name would be so that you can write the provides() line in that class. With that in mind, the savings is literally just typing the provides() line in the resource. If we see people using the fused mode Resource.action() stuff in lieu of writing provider classes, we should revisit this again as that will change the calculus.

Contributor

coderanger commented Jul 30, 2015

Summary from rambling on IRC: -1 on this as I would rather see explicit naming. More concretely this also doesn't apply to providers at all, so in all cases those would still need to be explicit when outside the Chef::Provider namespace, so even when using an implicit name you need to do the mental work to know what the name would be so that you can write the provides() line in that class. With that in mind, the savings is literally just typing the provides() line in the resource. If we see people using the fused mode Resource.action() stuff in lieu of writing provider classes, we should revisit this again as that will change the calculus.

@danielsdeleo

This comment has been minimized.

Show comment
Hide comment
@danielsdeleo

danielsdeleo Jul 30, 2015

Member

My preference is to not have a convention that includes branching logic, because the user ends up having to understand and internalize that complexity anyway, but it's also "out of sight, out of mind." If MyNamespace::Resource -> my_namespace is the one true way, that's fine, and the other way is also fine, but I really don't want both.

Member

danielsdeleo commented Jul 30, 2015

My preference is to not have a convention that includes branching logic, because the user ends up having to understand and internalize that complexity anyway, but it's also "out of sight, out of mind." If MyNamespace::Resource -> my_namespace is the one true way, that's fine, and the other way is also fine, but I really don't want both.

@jonlives

This comment has been minimized.

Show comment
Hide comment
@jonlives

jonlives Aug 6, 2015

Contributor

Essentially my thoughts are I don't like the idea of giving resource magical autonaming that providers don't get, and would rather keep it explicit via the provides statement for anything outside of Chef::Resource.

Contributor

jonlives commented Aug 6, 2015

Essentially my thoughts are I don't like the idea of giving resource magical autonaming that providers don't get, and would rather keep it explicit via the provides statement for anything outside of Chef::Resource.

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Aug 6, 2015

Contributor

I think we should just force it to be explicit:

class Foo < Chef::Resource
  resource_name :foo
  provides :foo
  provides :bar, os: :linux
end

Since this is all about reducing the typing of Foo once and :foo twice. I'd also think something like this would work as a compromise:

class Foo < Chef::Resource
  provides :foo, name: true
  provides :bar, os: :linux
end

Borrows from the "name attribute" concept to name the resource after one of the provides lines. Still got to type 'Foo' once and ':foo' once, but I think there's a tradeoff between magic and typing that you hit when you try to reduce duplicated typing to zero -- and magic is bad because you have to hold that magic in your head, and there's already enough stuff to learn about provider and resource resolution.

I also think that even inside of Chef::Resource it should work this. We have some backcompat stuff we have to support with magic based on the class, but that should be confined to Chef::Resource and deprecated.

Contributor

lamont-granquist commented Aug 6, 2015

I think we should just force it to be explicit:

class Foo < Chef::Resource
  resource_name :foo
  provides :foo
  provides :bar, os: :linux
end

Since this is all about reducing the typing of Foo once and :foo twice. I'd also think something like this would work as a compromise:

class Foo < Chef::Resource
  provides :foo, name: true
  provides :bar, os: :linux
end

Borrows from the "name attribute" concept to name the resource after one of the provides lines. Still got to type 'Foo' once and ':foo' once, but I think there's a tradeoff between magic and typing that you hit when you try to reduce duplicated typing to zero -- and magic is bad because you have to hold that magic in your head, and there's already enough stuff to learn about provider and resource resolution.

I also think that even inside of Chef::Resource it should work this. We have some backcompat stuff we have to support with magic based on the class, but that should be confined to Chef::Resource and deprecated.

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser Aug 13, 2015

Member

Don't think I have time to make sense of the alternative suggestions and come up with something happy; I can live with what I have now with use_automatic_resource_name, just not thrilled with it.

Member

jkeiser commented Aug 13, 2015

Don't think I have time to make sense of the alternative suggestions and come up with something happy; I can live with what I have now with use_automatic_resource_name, just not thrilled with it.

@jkeiser jkeiser closed this Aug 13, 2015

@jkeiser jkeiser deleted the jk/default-resource-name branch Aug 13, 2015

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Aug 13, 2015

Contributor

That really seems like the worst of both worlds. Its convention that you have to type to avoid typing convention.

Contributor

lamont-granquist commented Aug 13, 2015

That really seems like the worst of both worlds. Its convention that you have to type to avoid typing convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment