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

RFC some changes to default behavior #135

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
7 participants
@jkeiser
Member

jkeiser commented Jun 16, 2015

No description provided.

Show outdated Hide outdated new/property-defaults.md
### `reset_property`
There is already a `property_is_set?`, but no way to reset a property to its default. We suggest using `reset_property :name` to do this.

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

Seems like API creep, this is rare enough that it doesn't seem like a big deal and should basically just work if you do remove_instance_variable(:@name) should do the trick. That is slightly less nice, but given I've never wanted to do this or seen anyone want to do it, it seems like a win to avoid adding another method to the API.

@coderanger

coderanger Jun 16, 2015

Contributor

Seems like API creep, this is rare enough that it doesn't seem like a big deal and should basically just work if you do remove_instance_variable(:@name) should do the trick. That is slightly less nice, but given I've never wanted to do this or seen anyone want to do it, it seems like a win to avoid adding another method to the API.

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

From some further discussion on IRC, I still think this is creep in an already overloaded namespace (Chef::Resource). If we really want it for symmetry I would propose property_reset! as a better name to keep related stuff under a shared method prefix. Not as nice as a real-er API, but fights the creep a bit.

@coderanger

coderanger Jun 16, 2015

Contributor

From some further discussion on IRC, I still think this is creep in an already overloaded namespace (Chef::Resource). If we really want it for symmetry I would propose property_reset! as a better name to keep related stuff under a shared method prefix. Not as nice as a real-er API, but fights the creep a bit.

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

Yeah, just seems like if you have an official way to set a property, and an official way to check if it's set, but no official way to unset it, it seems like a douche move. "We have this toggle that you can look at, but you can't change." This isn't creep, it's completing a missing API.

My thought on API names is we use a naming convention to guard everything we care about--like, the base resource methods always have resource_x / property_x (for nouns) or x_resource / x_property (for verbs). property_reset sounds like you are asking whether a property is reset (though I admit the ! makes it clear that it's a verb--if you think we can couch all our verbs with !, then I'm on board).

@jkeiser

jkeiser Jul 7, 2015

Member

Yeah, just seems like if you have an official way to set a property, and an official way to check if it's set, but no official way to unset it, it seems like a douche move. "We have this toggle that you can look at, but you can't change." This isn't creep, it's completing a missing API.

My thought on API names is we use a naming convention to guard everything we care about--like, the base resource methods always have resource_x / property_x (for nouns) or x_resource / x_property (for verbs). property_reset sounds like you are asking whether a property is reset (though I admit the ! makes it clear that it's a verb--if you think we can couch all our verbs with !, then I'm on board).

This comment has been minimized.

@jkeiser

jkeiser Jul 10, 2015

Member

While I still believe this, I've removed reset_property from the proposal, because I can see the argument of "add it when you need it."

@jkeiser

jkeiser Jul 10, 2015

Member

While I still believe this, I've removed reset_property from the proposal, because I can see the argument of "add it when you need it."

Show outdated Hide outdated new/property-defaults.md
```ruby
class Foo < Chef::Resource
attribute :myhash, default: lazy { {} }

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

As written these are incorrect, should not have lazy {} in there.

@coderanger

coderanger Jun 16, 2015

Contributor

As written these are incorrect, should not have lazy {} in there.

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

Yeah, attribute doesn't work either. I'll modify it to extend from LWRPBase.

@jkeiser

jkeiser Jul 7, 2015

Member

Yeah, attribute doesn't work either. I'll modify it to extend from LWRPBase.

Show outdated Hide outdated new/property-defaults.md
end
```
This will fail to work in the new world, because we are freezing the constants. This is deliberate and a bugfix; the purpose is to avoid users setting values that affect all instances.

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

It isn't a bug fix per se as the bug will still be there, it will just show this to the user more directly rather than silently moving on.

@coderanger

coderanger Jun 16, 2015

Contributor

It isn't a bug fix per se as the bug will still be there, it will just show this to the user more directly rather than silently moving on.

This comment has been minimized.

@jaymzh

jaymzh Jul 7, 2015

Member

In this example does:

foo 'x' do
 myhash {:a => 10}
 mylist 'hi'
end

still work?

@jaymzh

jaymzh Jul 7, 2015

Member

In this example does:

foo 'x' do
 myhash {:a => 10}
 mylist 'hi'
end

still work?

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

Yes, both currently and after spec implementation. No change.

@jkeiser

jkeiser Jul 7, 2015

Member

Yes, both currently and after spec implementation. No change.

### Non-Sticky Defaults
Defaults are presently *sticky*, in that the first time a property is accessed, it is stored in the instance variable:

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

To be clear, this section is descriptive of what we have now and is not proposing any changes.

@coderanger

coderanger Jun 16, 2015

Contributor

To be clear, this section is descriptive of what we have now and is not proposing any changes.

Show outdated Hide outdated new/property-defaults.md
#### Computed defaults
```ruby

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

Kind of meh on this idea. Adding a new keyword to the API for properties isn't terrible, but it is already a big API so just noting that we should proceed cautiously. The added functionality isn't really something I've ever seen the need for, generally I would use a method when I want something not cached and based on other properties. I can see some argument in very complex cases though so I wouldn't fight it specifically. Docs should make it clear this is a rare case and probably not what a new user wants (they want lazy).

@coderanger

coderanger Jun 16, 2015

Contributor

Kind of meh on this idea. Adding a new keyword to the API for properties isn't terrible, but it is already a big API so just noting that we should proceed cautiously. The added functionality isn't really something I've ever seen the need for, generally I would use a method when I want something not cached and based on other properties. I can see some argument in very complex cases though so I wouldn't fight it specifically. Docs should make it clear this is a rare case and probably not what a new user wants (they want lazy).

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

I'll leave this one out for now.

@jkeiser

jkeiser Jul 7, 2015

Member

I'll leave this one out for now.

Show outdated Hide outdated new/property-defaults.md
#### Frozen constants
Sticky defaults create a serious issue for constants: *every instance* of a resource gets the same value.

This comment has been minimized.

@coderanger

coderanger Jun 16, 2015

Contributor

👍 on freezing, I've lost way too much time to this issue over the years.

@coderanger

coderanger Jun 16, 2015

Contributor

👍 on freezing, I've lost way too much time to this issue over the years.

Show outdated Hide outdated new/property-defaults.md
To fix this, we propose freezing constant default values, and not assigning them to the instance.
TODO an alternative is to dup them when assigning to an instance the way Chef::Config does.

This comment has been minimized.

@lamont-granquist

lamont-granquist Jun 17, 2015

Contributor

Should really go the other way and freeze the user supplied values as well. Mutating the new_resource always causes bugs.

@lamont-granquist

lamont-granquist Jun 17, 2015

Contributor

Should really go the other way and freeze the user supplied values as well. Mutating the new_resource always causes bugs.

This comment has been minimized.

@coderanger

coderanger Jun 17, 2015

Contributor

Eh, I've found it useful in a few cases where provider mixins mutate the resource in place. That's super niche though and I'm sure there are other ways to do it, but still not quite "always".

@coderanger

coderanger Jun 17, 2015

Contributor

Eh, I've found it useful in a few cases where provider mixins mutate the resource in place. That's super niche though and I'm sure there are other ways to do it, but still not quite "always".

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

@lamont-granquist By which you mean, we should make new_resource readonly before we run actions?

@jkeiser

jkeiser Jul 7, 2015

Member

@lamont-granquist By which you mean, we should make new_resource readonly before we run actions?

@pburkholder

This comment has been minimized.

Show comment
Hide comment
@pburkholder

pburkholder Jun 18, 2015

As a someone who helps our guide our customers through our docs and apis, I find that extending the API with more keywords tends to be more of a hinderance than a help. So +1 to @coderanger comments re api bloat.

As a someone who helps our guide our customers through our docs and apis, I find that extending the API with more keywords tends to be more of a hinderance than a help. So +1 to @coderanger comments re api bloat.

Show outdated Hide outdated new/property-defaults.md
#### Backcompat break: writing to constant defaults
Users may rely on this behavior to *write* to `myprop`, like this:

This comment has been minimized.

@miketheman

miketheman Jun 25, 2015

Member

Any evidence of this in the wild that can be pointed to as examples?

@miketheman

miketheman Jun 25, 2015

Member

Any evidence of this in the wild that can be pointed to as examples?

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

There was a bug recently around this where someone was appending a newline to a property . I'll see if I can find it later.

@jkeiser

jkeiser Jul 7, 2015

Member

There was a bug recently around this where someone was appending a newline to a property . I'll see if I can find it later.

Show outdated Hide outdated new/property-defaults.md
```ruby
class X < Chef::Resource
attribute :foo, default: lazy { Random.rand }

This comment has been minimized.

@miketheman

miketheman Jun 25, 2015

Member

I don't think I see the attribute anywhere in https://github.com/chef/chef/blob/12-stable/lib/chef/resource.rb - does this exist currently in this class? I thought it had to be subclassed from LWRPBase.

@miketheman

miketheman Jun 25, 2015

Member

I don't think I see the attribute anywhere in https://github.com/chef/chef/blob/12-stable/lib/chef/resource.rb - does this exist currently in this class? I thought it had to be subclassed from LWRPBase.

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

Yep. I'll update the example.

@jkeiser

jkeiser Jul 7, 2015

Member

Yep. I'll update the example.

@miketheman

This comment has been minimized.

Show comment
Hide comment
@miketheman

miketheman Jun 25, 2015

Member

Adding more options and toggles always seems a little fiddly to me, and as @pburkholder said, increases the amount of overhead.
I think, in general, we want the Thing of Least Surprise. I might be in the minority here, but I hope this is where everyone else wants to be.

It was surprising to me to read @jkeiser's example of current behavior (albiet it's not very clear how to run them, as they don't appear to work Chef 12 shell) and see the second case of x 'b' containing the values that had been set in x 'a' - something I don't believe I've ever encountered personally before, and didn't really expect.

However, this pattern is not one we've typically shown in recipes before - where the attribute in a resource is passed as a value to a method, not set or changed. Unless I'm totally reading it wrong.

Consider:

# what we typically do
my_resource 'a' do
  foo 1
end
# what this example shows
my_resource 'a' do
  foo << 1
end

So are we saying that we should be setting attribute values like foo = [1]? This differs from how we've been writing recipes with resources in them.

Member

miketheman commented Jun 25, 2015

Adding more options and toggles always seems a little fiddly to me, and as @pburkholder said, increases the amount of overhead.
I think, in general, we want the Thing of Least Surprise. I might be in the minority here, but I hope this is where everyone else wants to be.

It was surprising to me to read @jkeiser's example of current behavior (albiet it's not very clear how to run them, as they don't appear to work Chef 12 shell) and see the second case of x 'b' containing the values that had been set in x 'a' - something I don't believe I've ever encountered personally before, and didn't really expect.

However, this pattern is not one we've typically shown in recipes before - where the attribute in a resource is passed as a value to a method, not set or changed. Unless I'm totally reading it wrong.

Consider:

# what we typically do
my_resource 'a' do
  foo 1
end
# what this example shows
my_resource 'a' do
  foo << 1
end

So are we saying that we should be setting attribute values like foo = [1]? This differs from how we've been writing recipes with resources in them.

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser Jul 7, 2015

Member

@miketheman people don't presently use = to set, because it doesn't work. But I've seen (rare) cases where people use << and .push so that they can just append their stuff to the default; and it is one of those things that works great when you start (when there is only one resource) and fails in ways you might not even notice (and can't debug if you do) as you expand. (Exactly the sort of subtle bug we should help the user avoid, IMO.)

Member

jkeiser commented Jul 7, 2015

@miketheman people don't presently use = to set, because it doesn't work. But I've seen (rare) cases where people use << and .push so that they can just append their stuff to the default; and it is one of those things that works great when you start (when there is only one resource) and fails in ways you might not even notice (and can't debug if you do) as you expand. (Exactly the sort of subtle bug we should help the user avoid, IMO.)

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser Jul 7, 2015

Member

I'll update this PR (heavily) after vacation, just wanted to respond to people now that I had a minute.

Member

jkeiser commented Jul 7, 2015

I'll update this PR (heavily) after vacation, just wanted to respond to people now that I had a minute.

Show outdated Hide outdated new/property-defaults.md
### Move Core resources to `default` [compatbreak w/deprecation]
`default` is generally a better thing for memory pressure, and is useful for detecting whether a user has set a value or not. Currently, core Resources tend to assign property default values to the proper instance variable during `initialize`, and it is possible that subclasses may rely on this behavior. For Chef 12, we will grab the default values of any lazy arrays or hashes on initialize, and we will issue a deprecation warning for subclasses that directly use the initialized instance variable.

This comment has been minimized.

@jaymzh

jaymzh Jul 7, 2015

Member

so how should a subclass build upon defualts?

@jaymzh

jaymzh Jul 7, 2015

Member

so how should a subclass build upon defualts?

This comment has been minimized.

@jkeiser

jkeiser Jul 7, 2015

Member

Can you be more specific? In general if you want to change the default, you override the property and specify a different one. But not sure if that's what you mean.

@jkeiser

jkeiser Jul 7, 2015

Member

Can you be more specific? In general if you want to change the default, you override the property and specify a different one. But not sure if that's what you mean.

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser Jul 10, 2015

Member

@coderanger @miketheman @jaymzh @lamont-granquist this is updated in response to all current comments.

Member

jkeiser commented Jul 10, 2015

@coderanger @miketheman @jaymzh @lamont-granquist this is updated in response to all current comments.

Show outdated Hide outdated new/property-defaults.md
# Property Default Value Improvements
This RFC addresses some longstanding issues with `default` values on properties (attributes), adds `reset_property` to Resource, and addresses how we will move core Chef resources over to use defaults and properties.

This comment has been minimized.

@coderanger

coderanger Jul 23, 2015

Contributor

Reset got removed from below.

@coderanger

coderanger Jul 23, 2015

Contributor

Reset got removed from below.

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Jul 30, 2015

Contributor

👍

Contributor

lamont-granquist commented Jul 30, 2015

👍

@thommay

This comment has been minimized.

Show comment
Hide comment
@thommay

thommay Jul 30, 2015

Collaborator

I am Thom May, and I approve this RFC.

Collaborator

thommay commented Jul 30, 2015

I am Thom May, and I approve this RFC.

@jaymzh

This comment has been minimized.

Show comment
Hide comment
@jaymzh

jaymzh Jul 30, 2015

Member

I'm a huge fan of this, if for no reason other than validation working. :)

But are we providing a way for people who want to append to the default - or at least grab the default and then assign something based on that in? I don't personally do it, but clearly some people are doing it, or we wouldn't be here - what's the "right" way for those people?

Member

jaymzh commented Jul 30, 2015

I'm a huge fan of this, if for no reason other than validation working. :)

But are we providing a way for people who want to append to the default - or at least grab the default and then assign something based on that in? I don't personally do it, but clearly some people are doing it, or we wouldn't be here - what's the "right" way for those people?

@thommay

This comment has been minimized.

Show comment
Hide comment
@thommay

thommay Aug 5, 2015

Collaborator

Accepted as RFC 55, thanks

Collaborator

thommay commented Aug 5, 2015

Accepted as RFC 55, thanks

@thommay thommay closed this Aug 5, 2015

@thommay thommay deleted the jk/property-defaults branch Aug 5, 2015

martinb3 added a commit to martinb3/chef that referenced this pull request Aug 6, 2015

Add additional helpful section for frozen objects
Augument runtime errors with additional information that explains why an object might be frozen, and how frozen resource properties are a good thing.

Fixes #3734, for [accepted RFC 55](chef/chef-rfc#135).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment