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

Add "property" to resource #128

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
@jkeiser
Member

jkeiser commented May 14, 2015

No description provided.

@someara

This comment has been minimized.

Show comment
Hide comment
@someara

someara May 14, 2015

+1.

The overloading of this word is extremely confusing when teaching people how to write resources.

someara commented May 14, 2015

+1.

The overloading of this word is extremely confusing when teaching people how to write resources.

Show outdated Hide outdated new/property.md
This works similarly to `attribute`, except that:
1. Default values are *sticky*: they are dup'd and stored in the class the first time they are retrieved. The reason: if a class has a property and the user wants to start with the default value and change it, we support that. This does *not* apply to lazy defaults, which do not generally have the same problem.

This comment has been minimized.

@jaymzh

jaymzh May 15, 2015

Member

wait, waht?

For all instances of a resource, or only for the same resource wiht the same name? Your explanation doesn't compute for me, but your examples seems to imply that if I do:

package 'thing' do
  version 1
do

package 'another thing'

that 'another thing' implicitly has version 1... which I'm sure you don't mean. If you mean it's only duped if the name is the same, isn't that the same as attributes today (hence all the warnings)?

@jaymzh

jaymzh May 15, 2015

Member

wait, waht?

For all instances of a resource, or only for the same resource wiht the same name? Your explanation doesn't compute for me, but your examples seems to imply that if I do:

package 'thing' do
  version 1
do

package 'another thing'

that 'another thing' implicitly has version 1... which I'm sure you don't mean. If you mean it's only duped if the name is the same, isn't that the same as attributes today (hence all the warnings)?

This comment has been minimized.

@jkeiser

jkeiser May 15, 2015

Member

@jaymzh oops, I said the word "class" when I meant "resource." I'll fix it.

Basically it means that a default is a computed value that is cached. So:

class MyResource < Chef::Resource
  attribute :foo, default: []
end
my_resource 'a' do
  foo << 'x'
end
my_resource 'b' do
  foo << 'y'
  puts foo #=> [ 'y' ]
end

Will fix the word, sorry for the confusion.

@jkeiser

jkeiser May 15, 2015

Member

@jaymzh oops, I said the word "class" when I meant "resource." I'll fix it.

Basically it means that a default is a computed value that is cached. So:

class MyResource < Chef::Resource
  attribute :foo, default: []
end
my_resource 'a' do
  foo << 'x'
end
my_resource 'b' do
  foo << 'y'
  puts foo #=> [ 'y' ]
end

Will fix the word, sorry for the confusion.

This comment has been minimized.

@jaymzh

jaymzh May 15, 2015

Member

I still don't understand... the difference being that you can rely on the default and add to it rather than simploy overwriting it?

@jaymzh

jaymzh May 15, 2015

Member

I still don't understand... the difference being that you can rely on the default and add to it rather than simploy overwriting it?

This comment has been minimized.

@jkeiser

jkeiser May 15, 2015

Member

So, there are a few ways to handle a non-lazy default like []:

class MyResource < Chef::Resource
  attribute :foo, default: []
end
my_resource 'a' do
  foo << 'x'
end
my_resource 'b' do
  foo << 'y'
end
  1. You can just return the default when someone asks for it. The issue is that [] is only ever evaluated once: when you first compile the class. That's just a Ruby thing. So if you return that thing, and someone does something to it like an append, it affects all other instances of that resource class. If you do this, b.foo and a.foo will both be [ 'a', 'b' ] at converge time.
  2. You can dup and sticky it. mixlib-config had the same problem (@danielsdeleo can confirm here :), and our solution there was to dup the value and stick it to the instance (which means a.foo == [ 'a' ] and b.foo == [ 'b' ] at the end).
  3. A solution which is too large to contain in this margin.

It could be we just leave well enough alone; but it's worth thinking through here because we have a clean break in which we can make behavior changes (since property is a new word).

@jkeiser

jkeiser May 15, 2015

Member

So, there are a few ways to handle a non-lazy default like []:

class MyResource < Chef::Resource
  attribute :foo, default: []
end
my_resource 'a' do
  foo << 'x'
end
my_resource 'b' do
  foo << 'y'
end
  1. You can just return the default when someone asks for it. The issue is that [] is only ever evaluated once: when you first compile the class. That's just a Ruby thing. So if you return that thing, and someone does something to it like an append, it affects all other instances of that resource class. If you do this, b.foo and a.foo will both be [ 'a', 'b' ] at converge time.
  2. You can dup and sticky it. mixlib-config had the same problem (@danielsdeleo can confirm here :), and our solution there was to dup the value and stick it to the instance (which means a.foo == [ 'a' ] and b.foo == [ 'b' ] at the end).
  3. A solution which is too large to contain in this margin.

It could be we just leave well enough alone; but it's worth thinking through here because we have a clean break in which we can make behavior changes (since property is a new word).

This comment has been minimized.

@jkeiser

jkeiser May 15, 2015

Member

I went ahead and removed this bit for now ... it presents other difficulties which I think I like even less. Fixing it probably relates to a different proposal anyway.

@jkeiser

jkeiser May 15, 2015

Member

I went ahead and removed this bit for now ... it presents other difficulties which I think I like even less. Fixing it probably relates to a different proposal anyway.

class A < Chef::Resource
property :a, String
end
class B < Chef::Resource

This comment has been minimized.

@jaymzh

jaymzh May 15, 2015

Member

did you mean to make B a subclass of A here?

@jaymzh

jaymzh May 15, 2015

Member

did you mean to make B a subclass of A here?

This comment has been minimized.

@jaymzh

jaymzh May 15, 2015

Member

did you see this note?

@jaymzh

jaymzh May 15, 2015

Member

did you see this note?

This comment has been minimized.

@jkeiser

jkeiser May 15, 2015

Member

Yep, I just didn't have time to make other changes before I had to go :)

@jkeiser

jkeiser May 15, 2015

Member

Yep, I just didn't have time to make other changes before I had to go :)

This comment has been minimized.

@jaymzh

jaymzh Jun 3, 2015

Member

You updated this commit a few times, but this is still not fixed... just FYI

@jaymzh

jaymzh Jun 3, 2015

Member

You updated this commit a few times, but this is still not fixed... just FYI

@jaymzh

This comment has been minimized.

Show comment
Hide comment
@jaymzh

jaymzh May 15, 2015

Member

OMG I'm super excited about this!

Member

jaymzh commented May 15, 2015

OMG I'm super excited about this!

Show outdated Hide outdated new/property.md
### nil
In order to allow for `nil` as a potential explicit value, property setters accept `nil` and set the value of the property to `nil`. This differs from `attribute`, which considers `property(nil)` to be a get.

This comment has been minimized.

@jeremyolliver

jeremyolliver May 19, 2015

Is this supposed to read "considers attribute(nil) to be a get"?

👍 on proposal, and patchy properties sounds pretty useful

@jeremyolliver

jeremyolliver May 19, 2015

Is this supposed to read "considers attribute(nil) to be a get"?

👍 on proposal, and patchy properties sounds pretty useful

This comment has been minimized.

@jkeiser

jkeiser May 20, 2015

Member

Actually, probably my_property(nil) is the most accurate.

@jkeiser

jkeiser May 20, 2015

Member

Actually, probably my_property(nil) is the most accurate.

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

If you implement attributes as properties then you can't do this. It'll be a breaking change, and there's code out there which expects resource.thing nil to not be a setter and to be silently ignored such that resource.thing will later have the default value. I made an attempt to implement nillable attributes and it blew up horribly on code in the application cookbook.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

If you implement attributes as properties then you can't do this. It'll be a breaking change, and there's code out there which expects resource.thing nil to not be a setter and to be silently ignored such that resource.thing will later have the default value. I made an attempt to implement nillable attributes and it blew up horribly on code in the application cookbook.

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

Worth noting that version of the application cookbook is super deprecated and I give no shits if it breaks on newer Chef.

@coderanger

coderanger May 21, 2015

Contributor

Worth noting that version of the application cookbook is super deprecated and I give no shits if it breaks on newer Chef.

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

Also, if you allow properties and attributes to differ slightly, then all the existing core chef attributes must remain attributes and it'll be a breaking change to convert them to properties.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

Also, if you allow properties and attributes to differ slightly, then all the existing core chef attributes must remain attributes and it'll be a breaking change to convert them to properties.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Yes, it will be a breaking change to convert an attribute to a property, because this difference will exist. The statement that attributes are implemented as properties implies that they use the same core, but different default behavior.

The concern about complexity (as I understood it) was around the fact that two completely different systems (and sets of code) would exist, which will definitely not be the case.

@jkeiser

jkeiser May 21, 2015

Member

Yes, it will be a breaking change to convert an attribute to a property, because this difference will exist. The statement that attributes are implemented as properties implies that they use the same core, but different default behavior.

The concern about complexity (as I understood it) was around the fact that two completely different systems (and sets of code) would exist, which will definitely not be the case.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Oh, now I understand what you mean by "core chef attributes"--you mean attributes in core Chef resources? Yes, we'll have to have an RFC to be able to convert existing attributes over, since it will cause file.mode(nil) to emit an error (you can't set mode to nil, it's not a valid value). We can convert them over if we implement this difference as a modifier like nil_is_default: true and add the modifier to the core Chef codebase; but I'm not sure that's worth it when we could just RFC it.

@jkeiser

jkeiser May 21, 2015

Member

Oh, now I understand what you mean by "core chef attributes"--you mean attributes in core Chef resources? Yes, we'll have to have an RFC to be able to convert existing attributes over, since it will cause file.mode(nil) to emit an error (you can't set mode to nil, it's not a valid value). We can convert them over if we implement this difference as a modifier like nil_is_default: true and add the modifier to the core Chef codebase; but I'm not sure that's worth it when we could just RFC it.

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

Anything in this proposal that would result in two parallel systems should be deemed out of scope. It sounds like nil-able attributes should, at a minimum, be a class level opt-in or possibly a per-attribute opt-in.

@coderanger

coderanger May 21, 2015

Contributor

Anything in this proposal that would result in two parallel systems should be deemed out of scope. It sounds like nil-able attributes should, at a minimum, be a class level opt-in or possibly a per-attribute opt-in.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Lamont and I were talking, and I'm planning to update this RFC to say that attributes will do the same thing as well, but that setting an_attribute(nil) will be deprecated in Chef 12 and removed in Chef 13. My plan is to make it so that property :x, must_match: [String, nil] (or whatever that ends up being) will allow you to set nil in Chef 12.

What say you?

(Edit: oh, you already said, because I apparently already pointed this out.)

@jkeiser

jkeiser May 21, 2015

Member

Lamont and I were talking, and I'm planning to update this RFC to say that attributes will do the same thing as well, but that setting an_attribute(nil) will be deprecated in Chef 12 and removed in Chef 13. My plan is to make it so that property :x, must_match: [String, nil] (or whatever that ends up being) will allow you to set nil in Chef 12.

What say you?

(Edit: oh, you already said, because I apparently already pointed this out.)

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

👍 I've lost track of which threads are which on here already.

@coderanger

coderanger May 21, 2015

Contributor

👍 I've lost track of which threads are which on here already.

@jkeiser jkeiser changed the title from DRAFT: Add "property" to resource to Add "property" to resource May 20, 2015

Show outdated Hide outdated new/property.md
# Resource Properties
We add `property` DSL to resources, similar to (and interoperable with) LWRP `attribute`.

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

I thought there was agreement some time ago to use the word parameter, so just curious as to why property and not parameter.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

I thought there was agreement some time ago to use the word parameter, so just curious as to why property and not parameter.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

My reasoning was that property is a thing a noun (resource) has, and parameter is a thing a verb (function or action) has. I'm not crazy attached, but I do have that slight preference: it'll be a common enough word that it will flavor peoples' orientation towards what a resource is--whether it is a noun describing desired state or a verb describing an action that needs to be taken. If you, @someara and @danielsdeleo would like the latter, I'll go there.

@jkeiser

jkeiser May 21, 2015

Member

My reasoning was that property is a thing a noun (resource) has, and parameter is a thing a verb (function or action) has. I'm not crazy attached, but I do have that slight preference: it'll be a common enough word that it will flavor peoples' orientation towards what a resource is--whether it is a noun describing desired state or a verb describing an action that needs to be taken. If you, @someara and @danielsdeleo would like the latter, I'll go there.

This comment has been minimized.

@coderanger

coderanger May 22, 2015

Contributor

I have no strong feelings, but we should pick one and broadcast it so there is some consistency :)

@coderanger

coderanger May 22, 2015

Contributor

I have no strong feelings, but we should pick one and broadcast it so there is some consistency :)

Show outdated Hide outdated new/property.md
```ruby
property :x, must_match: [String, :a, :b, nil]
```

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

This seems like kind of a bad practice to be encouraging. Looks like using an array as a struct of things that should probably be paramaters of their own.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

This seems like kind of a bad practice to be encouraging. Looks like using an array as a struct of things that should probably be paramaters of their own.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

As far as arrays go, all our existing matchers (kind_of: [ String, Symbol ] and equal_to: [ :a, :b ] support them. But, I'm unclear what you mean by parameters of their own?

@jkeiser

jkeiser May 21, 2015

Member

As far as arrays go, all our existing matchers (kind_of: [ String, Symbol ] and equal_to: [ :a, :b ] support them. But, I'm unclear what you mean by parameters of their own?

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

Okay syntax was unclear before the coffee hit. That looks like you're asserting that the property must be an array which starts with a string, then :a, :b and nil. The words "must_match" are probably bad though since it wasn't immediately clear what you were doing.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

Okay syntax was unclear before the coffee hit. That looks like you're asserting that the property must be an array which starts with a string, then :a, :b and nil. The words "must_match" are probably bad though since it wasn't immediately clear what you were doing.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Your suggestion of one_of or matches_one_of might be good. I sort of like one_of.

@jkeiser

jkeiser May 21, 2015

Member

Your suggestion of one_of or matches_one_of might be good. I sort of like one_of.

This comment has been minimized.

@coderanger

coderanger May 22, 2015

Contributor

Maybe just matches, that implies some kind of vague matching behavior. Django uses validation as the key to a similar goal.

@coderanger

coderanger May 22, 2015

Contributor

Maybe just matches, that implies some kind of vague matching behavior. Django uses validation as the key to a similar goal.

Show outdated Hide outdated new/property.md
#### lazy values
Properties may be set to lazy values, which work the same as in attributes: they are treated as computed values, popped open and validated each time you access them.

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

Note to double check if lazy values are cached or not. Also re: lazy default.

@coderanger

coderanger May 21, 2015

Contributor

Note to double check if lazy values are cached or not. Also re: lazy default.

This comment has been minimized.

@jkeiser

jkeiser May 27, 2015

Member

@coderanger it looks like we cache all attribute values, actually: @z is 1 after we retrieve the value of z:

class Xyz < Chef::Resource::LWRPBase
  self.resource_name = "xyz"
  provides :xyz
  attribute :x, default: lazy { [] }
  attribute :y, default: []
  attribute :z, default: 1
end

xyz 'blah' do
  puts self.inspect
  z
  puts self.inspect
end
@jkeiser

jkeiser May 27, 2015

Member

@coderanger it looks like we cache all attribute values, actually: @z is 1 after we retrieve the value of z:

class Xyz < Chef::Resource::LWRPBase
  self.resource_name = "xyz"
  provides :xyz
  attribute :x, default: lazy { [] }
  attribute :y, default: []
  attribute :z, default: 1
end

xyz 'blah' do
  puts self.inspect
  z
  puts self.inspect
end
Show outdated Hide outdated new/property.md
#### default
This works similarly to `attribute`, except that `lazy` values are automatically run in the context of the *instance*:

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

This makes it impossible to implement attribute as an alias to this doesn't it without changing attribute semantics?

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

This makes it impossible to implement attribute as an alias to this doesn't it without changing attribute semantics?

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

It will be implemented as a property, but not strictly aliased. Think subclass with a couple of behavior differences. (Though it may be implemented like def attribute(...); opts[:default] = opts.delete(:default); end.) The reason being, since we're already interested in changing the word, that's a good break point to make any changes we need.

@jkeiser

jkeiser May 21, 2015

Member

It will be implemented as a property, but not strictly aliased. Think subclass with a couple of behavior differences. (Though it may be implemented like def attribute(...); opts[:default] = opts.delete(:default); end.) The reason being, since we're already interested in changing the word, that's a good break point to make any changes we need.

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

That'll be somewhat confusing. I can see your point, but now you have to explain very subtle details to users over the choice of a word which does almost exactly the same thing.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

That'll be somewhat confusing. I can see your point, but now you have to explain very subtle details to users over the choice of a word which does almost exactly the same thing.

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

I think this should be a strict alias. Otherwise we are back to the two systems problem. That means we need to be more careful with compat in the design, but that's okay.

@coderanger

coderanger May 21, 2015

Contributor

I think this should be a strict alias. Otherwise we are back to the two systems problem. That means we need to be more careful with compat in the design, but that's okay.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

@lamont-granquist offered a plan that seems OK to me: property will be an actual alias to attribute (not a subclass or anything), and we will deprecate letting you set something to nil, but keep the old behavior (attr(nil) doesn't change the value) unless the property explicitly allows nil as a valid value (in which case it sets the value to nil).

@jkeiser

jkeiser May 21, 2015

Member

@lamont-granquist offered a plan that seems OK to me: property will be an actual alias to attribute (not a subclass or anything), and we will deprecate letting you set something to nil, but keep the old behavior (attr(nil) doesn't change the value) unless the property explicitly allows nil as a valid value (in which case it sets the value to nil).

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

Sure, in that case the equal_to or similar setting is effectively an opt-in :-)

@coderanger

coderanger May 21, 2015

Contributor

Sure, in that case the equal_to or similar setting is effectively an opt-in :-)

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

Though we should probably print a deprecation warning on the blocked setter calls so we can clean that up in 13 or somesuch. It's usually indicative of bad code anyway.

@coderanger

coderanger May 21, 2015

Contributor

Though we should probably print a deprecation warning on the blocked setter calls so we can clean that up in 13 or somesuch. It's usually indicative of bad code anyway.

Show outdated Hide outdated new/property.md
#### must_match
This new option takes an array of values which use Ruby's universal matching

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

Kind of meh on this as a specific thing, especially if we can use RSpec matchers for this kind of rare case, or a proc.

@coderanger

coderanger May 21, 2015

Contributor

Kind of meh on this as a specific thing, especially if we can use RSpec matchers for this kind of rare case, or a proc.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

The actual reason I personally want it is that I'm often interested in doing things like "this can either be a String or one of the symbols :x, :y or :z. But the equal_to and kind_of operators just can't be combined in that way. Same for regex, etc. I see this as a one-size replacement for all of those operators that can be used everywhere.

@jkeiser

jkeiser May 21, 2015

Member

The actual reason I personally want it is that I'm often interested in doing things like "this can either be a String or one of the symbols :x, :y or :z. But the equal_to and kind_of operators just can't be combined in that way. Same for regex, etc. I see this as a one-size replacement for all of those operators that can be used everywhere.

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

That seems like enough of a special case that is doesn't need its own thing, but I wouldn't argue the point :)

@coderanger

coderanger May 21, 2015

Contributor

That seems like enough of a special case that is doesn't need its own thing, but I wouldn't argue the point :)

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

The name is kind of icky though, maybe just add this as a thing in equal_to? If one of the values in the array is a Class, it acts like kind_of. I don't know if I like that any more though. Hard to make something intuitively obvious when there is so much overlap.

@coderanger

coderanger May 21, 2015

Contributor

The name is kind of icky though, maybe just add this as a thing in equal_to? If one of the values in the array is a Class, it acts like kind_of. I don't know if I like that any more though. Hard to make something intuitively obvious when there is so much overlap.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Main thing is, if we do this, then equal_to, kind_of, regexes and (to an extent) validate all become sub-cases of this one thing, for nearly everything the user types (except special cases like equal_to: [ Class1, Class2, Class3 ]. It has the potential to be the one-stop shop for most peoples' validation needs.

I'm starting to like how this looks, though:

property :x, [ String, :a, :b, nil ]

I could see is being the property representing this, so this would be valid as well:

property :x, is: [ String, :a, :b, nil ]

What say you all?

@jkeiser

jkeiser May 21, 2015

Member

Main thing is, if we do this, then equal_to, kind_of, regexes and (to an extent) validate all become sub-cases of this one thing, for nearly everything the user types (except special cases like equal_to: [ Class1, Class2, Class3 ]. It has the potential to be the one-stop shop for most peoples' validation needs.

I'm starting to like how this looks, though:

property :x, [ String, :a, :b, nil ]

I could see is being the property representing this, so this would be valid as well:

property :x, is: [ String, :a, :b, nil ]

What say you all?

This comment has been minimized.

@coderanger

coderanger May 21, 2015

Contributor

I can dig it. Can still support procs for the weird case where you want to actually pass a literal class as a property, but even I've never wanted to do that so it seems unlikely.

@coderanger

coderanger May 21, 2015

Contributor

I can dig it. Can still support procs for the weird case where you want to actually pass a literal class as a property, but even I've never wanted to do that so it seems unlikely.

@smurawski

This comment has been minimized.

Show comment
Hide comment
@smurawski

smurawski May 21, 2015

Contributor

👍

Contributor

smurawski commented May 21, 2015

👍

Show outdated Hide outdated new/property.md
end
```
The reason being, a patchy property is one that *leaves the value alone* unless the user actually says they want to change it. Cloning values from other resources violates that.

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

It'd probably be better to simply kill resource cloning in Chef-13 rather than doubling down on figuring out how to support it.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

It'd probably be better to simply kill resource cloning in Chef-13 rather than doubling down on figuring out how to support it.

This comment has been minimized.

@ranjib

ranjib May 21, 2015

Member

👍 we have to bite this bullet, resource cloning is likely to cause more trouble.

@ranjib

ranjib May 21, 2015

Member

👍 we have to bite this bullet, resource cloning is likely to cause more trouble.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

I completely agree. To be clear though, this attribute is all about not supporting resource cloning for properties while leaving it around for attributes. When we remove resource cloning, we'll just have fewer things to deprecate. (Note the default.)

@jkeiser

jkeiser May 21, 2015

Member

I completely agree. To be clear though, this attribute is all about not supporting resource cloning for properties while leaving it around for attributes. When we remove resource cloning, we'll just have fewer things to deprecate. (Note the default.)

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

When we drop resource cloning this just becomes a no-op though. Is there a huge pressing use case for this? because I haven't seen any bugs about it. We're going to force people to think about it, people will misuse it, then we'll drop it and carry around this no-op keyword in recipes going forwards, and inevitably write a 'consider dropping useless patchy modifier' foodcritic rule. It really smells like something we should not implement only to take away later.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

When we drop resource cloning this just becomes a no-op though. Is there a huge pressing use case for this? because I haven't seen any bugs about it. We're going to force people to think about it, people will misuse it, then we'll drop it and carry around this no-op keyword in recipes going forwards, and inevitably write a 'consider dropping useless patchy modifier' foodcritic rule. It really smells like something we should not implement only to take away later.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Ah! Sure. I'll drop it from the proposal. I just hate resource cloning, and didn't realize we intended to take it away.

@jkeiser

jkeiser May 21, 2015

Member

Ah! Sure. I'll drop it from the proposal. I just hate resource cloning, and didn't realize we intended to take it away.

- If the type is a PropertyType instance, it is dup'd and any <option>s are set on the new type.
- If the type is a Class, a new PropertyType instance is created with `kind_of` set to [Class] and any options are set on the new type.
- If type is not passed, a new PropertyType instance is created, and any options are set on the new type.

This comment has been minimized.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

Having some way of determining the default value of a property and if the user had set the property (and distinguishing if they had set it to the default value) would be useful introspection to be able to do on the resource.

@lamont-granquist

lamont-granquist May 21, 2015

Contributor

Having some way of determining the default value of a property and if the user had set the property (and distinguishing if they had set it to the default value) would be useful introspection to be able to do on the resource.

This comment has been minimized.

@jkeiser

jkeiser May 21, 2015

Member

Yeah, I was thinking maybe a properties on the class. As for whether the property was set, I'd like an is_set?(:attr_name), which I'm quite sure will make it into one of these RFCs (though I don't think this RFC made the need for it any worse than it is now).

@jkeiser

jkeiser May 21, 2015

Member

Yeah, I was thinking maybe a properties on the class. As for whether the property was set, I'd like an is_set?(:attr_name), which I'm quite sure will make it into one of these RFCs (though I don't think this RFC made the need for it any worse than it is now).

@coderanger

This comment has been minimized.

Show comment
Hide comment
@coderanger

coderanger May 21, 2015

Contributor

Summary of feedback from IRC: huge +1 on all the functional improvements to the attributes system. -1 on having two systems in parallel (stated for the record, this RFC will not result in two systems in parallel), and would probably be good to pivot the message a bit to reflect that this is 90% about new features for resource attributes and 10% about adding a new name for the resource attributes subsystem because the current name is ass-tastic and we have to start migrating at some point.

Contributor

coderanger commented May 21, 2015

Summary of feedback from IRC: huge +1 on all the functional improvements to the attributes system. -1 on having two systems in parallel (stated for the record, this RFC will not result in two systems in parallel), and would probably be good to pivot the message a bit to reflect that this is 90% about new features for resource attributes and 10% about adding a new name for the resource attributes subsystem because the current name is ass-tastic and we have to start migrating at some point.

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist May 21, 2015

Contributor

👍 generally on renaming 'attributes' and i'm okay with 'property' if everyone else is.

Got some concerns about attributes-vs-properties being semantically different and the confusion that will create since it will cement "resource attributes" as a real thing and make this more than just changing words and extending an API. That will mean that internal core chef resources will have to have attributes and not properties without a breaking change. That doesn't seem like success, I'd like to be able to property-ize all the core chef resources fairly immediately.

Contributor

lamont-granquist commented May 21, 2015

👍 generally on renaming 'attributes' and i'm okay with 'property' if everyone else is.

Got some concerns about attributes-vs-properties being semantically different and the confusion that will create since it will cement "resource attributes" as a real thing and make this more than just changing words and extending an API. That will mean that internal core chef resources will have to have attributes and not properties without a breaking change. That doesn't seem like success, I'd like to be able to property-ize all the core chef resources fairly immediately.

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser May 21, 2015

Member

@coderanger @lamont-granquist there are enough changes that we'll put this back up for vote next meeting. I'm responding to comments and will make whatever changes need making.

Member

jkeiser commented May 21, 2015

@coderanger @lamont-granquist there are enough changes that we'll put this back up for vote next meeting. I'm responding to comments and will make whatever changes need making.

Show outdated Hide outdated new/property.md
```ruby
class MyResource < Chef::Resource
property :path, name_attribute: true
end

This comment has been minimized.

@zuazo

zuazo May 22, 2015

Perhaps I have misunderstood the whole thing. Shouldn't this be property :path, name_property: true? or maybe something like :default_from_name. I find it confusing to have the two nomenclatures there.

@zuazo

zuazo May 22, 2015

Perhaps I have misunderstood the whole thing. Shouldn't this be property :path, name_property: true? or maybe something like :default_from_name. I find it confusing to have the two nomenclatures there.

This comment has been minimized.

@coderanger

coderanger May 22, 2015

Contributor

Good point, we should support both name_attribute and name_property during the transition, with name_attribute showing a deprecation warning?

@coderanger

coderanger May 22, 2015

Contributor

Good point, we should support both name_attribute and name_property during the transition, with name_attribute showing a deprecation warning?

Show outdated Hide outdated new/property.md
- `MyResource.properties` will contain the property type.
- The setter and getter manipulate the class variable `@<name>`.
#### Use `property` instead of `attribute` in documentation

This comment has been minimized.

@pburkholder

pburkholder May 22, 2015

Given that most Chef users get guidance on using Chef from blogs, archived mail lists, and books it will be utterly confounding to most users what is the deal with attributes vs. properties.

@pburkholder

pburkholder May 22, 2015

Given that most Chef users get guidance on using Chef from blogs, archived mail lists, and books it will be utterly confounding to most users what is the deal with attributes vs. properties.

This comment has been minimized.

@pburkholder

pburkholder May 22, 2015

@jamescott might have an opinion on the feasibility of updating all documentation to correctly distinguish between resource 'property' and node 'attribute'

@burtlo might want to weigh in on updating all training materials and video. Also, whether this will provide more clarity to new users or be a net loss in usability.

@pburkholder

pburkholder May 22, 2015

@jamescott might have an opinion on the feasibility of updating all documentation to correctly distinguish between resource 'property' and node 'attribute'

@burtlo might want to weigh in on updating all training materials and video. Also, whether this will provide more clarity to new users or be a net loss in usability.

This comment has been minimized.

@jkeiser

jkeiser May 26, 2015

Member

Getting some feedback from our documentation and training folk will be awesome. I'll ping them directly if they don't respond here.

@jkeiser

jkeiser May 26, 2015

Member

Getting some feedback from our documentation and training folk will be awesome. I'll ping them directly if they don't respond here.

@pburkholder

This comment has been minimized.

Show comment
Hide comment
@pburkholder

pburkholder May 22, 2015

I think many users will find this change in naming from attribute to property more confusing than the current state of overload of the term.

pburkholder commented May 22, 2015

I think many users will find this change in naming from attribute to property more confusing than the current state of overload of the term.

Show outdated Hide outdated new/property.md
```ruby
class File < Chef::Resource
attribute :mode, coerce: proc { |v| mode.is_a?(String) ? mode.to_s(8) : mode }

This comment has been minimized.

@ryancragun

ryancragun May 26, 2015

Member

s/v/mode/

@ryancragun

ryancragun May 26, 2015

Member

s/v/mode/

@jkeiser

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser May 27, 2015

Member

@coderanger @lamont-granquist @danielsdeleo I have updated the proposal based on all comments thus far. There's just enough change to make it worth rereading.

@pburkholder @jamescott seems on board, I still need to get a hold of @burtlo.

Member

jkeiser commented May 27, 2015

@coderanger @lamont-granquist @danielsdeleo I have updated the proposal based on all comments thus far. There's just enough change to make it worth rereading.

@pburkholder @jamescott seems on board, I still need to get a hold of @burtlo.

@pburkholder

This comment has been minimized.

Show comment
Hide comment
@pburkholder

pburkholder May 27, 2015

Cool -- I'm good with this, was just trying on customer lenses to see how it looked from there.

pburkholder commented May 27, 2015

Cool -- I'm good with this, was just trying on customer lenses to see how it looked from there.

@coderanger

This comment has been minimized.

Show comment
Hide comment
@coderanger

coderanger May 27, 2015

Contributor

Isn't the thing we are deprecating the reverse, not doing the set?

Contributor

coderanger commented on new/property.md in 326ec8d May 27, 2015

Isn't the thing we are deprecating the reverse, not doing the set?

This comment has been minimized.

Show comment
Hide comment
@jkeiser

jkeiser May 28, 2015

Member

Yes :)

Member

jkeiser replied May 28, 2015

Yes :)

@coderanger

This comment has been minimized.

Show comment
Hide comment
@coderanger

coderanger May 27, 2015

Contributor

Which move this to the top since you use it in examples above this section.

Contributor

coderanger commented on new/property.md in 326ec8d May 27, 2015

Which move this to the top since you use it in examples above this section.

Show outdated Hide outdated new/property.md
end
```
This is a breaking change, and in Chef 12 will only affect properties (not attributes).

This comment has been minimized.

@coderanger

coderanger May 27, 2015

Contributor

How is this a breaking change? In 12.3 this syntax wouldn't be supported at all AFAIK.

@coderanger

coderanger May 27, 2015

Contributor

How is this a breaking change? In 12.3 this syntax wouldn't be supported at all AFAIK.

This comment has been minimized.

@jkeiser

jkeiser May 28, 2015

Member

@coderanger consider this recipe:

class X < Chef::Resource::LWRPBase
  self.resource_name = 'x'
  provides :x
  attribute :foo, default: 'bar'
  def self.foo
    'foo'
  end

  attribute :y, default: lazy { foo }
end

x 'blah' do
  puts y
end

In Chef 12 it will print "foo", and in Chef 13 it will print "bar."

It probably isn't a big break, and it's a clear win IMO; but it's a break.

@jkeiser

jkeiser May 28, 2015

Member

@coderanger consider this recipe:

class X < Chef::Resource::LWRPBase
  self.resource_name = 'x'
  provides :x
  attribute :foo, default: 'bar'
  def self.foo
    'foo'
  end

  attribute :y, default: lazy { foo }
end

x 'blah' do
  puts y
end

In Chef 12 it will print "foo", and in Chef 13 it will print "bar."

It probably isn't a big break, and it's a clear win IMO; but it's a break.

This comment has been minimized.

@coderanger

coderanger May 28, 2015

Contributor

I don't think that's valid in Chef 12 to start with. #lazy is only defined as an instance method, not a class method.

@coderanger

coderanger May 28, 2015

Contributor

I don't think that's valid in Chef 12 to start with. #lazy is only defined as an instance method, not a class method.

This comment has been minimized.

@coderanger

coderanger May 28, 2015

Contributor

So this might be a breaking change if the user did attribute :y, default: Chef::DelayedEvaluator.new { foo }, but I'm comfortable saying no one has ever done that :-)

@coderanger

coderanger May 28, 2015

Contributor

So this might be a breaking change if the user did attribute :y, default: Chef::DelayedEvaluator.new { foo }, but I'm comfortable saying no one has ever done that :-)

This comment has been minimized.

@jkeiser

jkeiser May 28, 2015

Member

Fascinating, I wonder what I did that made that recipe work for me ...

At any rate, I am definitely willing to say that isn't a breaking change.

@jkeiser

jkeiser May 28, 2015

Member

Fascinating, I wonder what I did that made that recipe work for me ...

At any rate, I am definitely willing to say that isn't a breaking change.

This comment has been minimized.

@coderanger

coderanger May 28, 2015

Contributor

Ahh, I went and looked, Chef::Resource::LWRPBase has its own implementation of lazy as a classmethod >_< So that does technically make it a compat change.

@coderanger

coderanger May 28, 2015

Contributor

Ahh, I went and looked, Chef::Resource::LWRPBase has its own implementation of lazy as a classmethod >_< So that does technically make it a compat change.

This comment has been minimized.

@jkeiser

jkeiser May 28, 2015

Member

Darn, I was hoping I had accidentally introduced lazy myself. I doubt anyone is going to see a real difference, but it's a compat change. Not hard to differentiate, though--you can always have a parameter to DelayedEvaluator to say whether you want to instance_eval or not, and in Chef::Resource lazy turns it on.

@jkeiser

jkeiser May 28, 2015

Member

Darn, I was hoping I had accidentally introduced lazy myself. I doubt anyone is going to see a real difference, but it's a compat change. Not hard to differentiate, though--you can always have a parameter to DelayedEvaluator to say whether you want to instance_eval or not, and in Chef::Resource lazy turns it on.

Show outdated Hide outdated new/property.md
attribute :path, regex: /^\//
attribute :path, respond_to: :merge
attribute :path, cannot_be: :empty
property :path, is: String

This comment has been minimized.

@coderanger

coderanger May 28, 2015

Contributor

I think you meant to have something like # Are equivalent to these:, overall this example is hard to read though.

@coderanger

coderanger May 28, 2015

Contributor

I think you meant to have something like # Are equivalent to these:, overall this example is hard to read though.

This comment has been minimized.

@jkeiser

jkeiser May 28, 2015

Member

I'll make it into a table.

@jkeiser

jkeiser May 28, 2015

Member

I'll make it into a table.

Show outdated Hide outdated new/property.md
```ruby
include RSpec::Matchers
property :path, a_string_starting_with('/')

This comment has been minimized.

@coderanger

coderanger May 28, 2015

Contributor

Kind of having second thoughts about this syntax because of how many methods will be dumped in to the class-level methods. It would be really easy to accidentally mask typos, and this would add a method_missing for be_*. Maybe for rspec matchers we could restrict them to being in a proc? That would allow only adding the RSpec DSL to that context.

@coderanger

coderanger May 28, 2015

Contributor

Kind of having second thoughts about this syntax because of how many methods will be dumped in to the class-level methods. It would be really easy to accidentally mask typos, and this would add a method_missing for be_*. Maybe for rspec matchers we could restrict them to being in a proc? That would allow only adding the RSpec DSL to that context.

This comment has been minimized.

@jkeiser

jkeiser May 28, 2015

Member

I actually wasn't planning on including them natively actually--just for people who wanted, it becomes an immediate option (since RSpec Matchers implement ===).

@jkeiser

jkeiser May 28, 2015

Member

I actually wasn't planning on including them natively actually--just for people who wanted, it becomes an immediate option (since RSpec Matchers implement ===).

This comment has been minimized.

@coderanger

coderanger May 28, 2015

Contributor

Gotcha 👍

@coderanger

coderanger May 28, 2015

Contributor

Gotcha 👍

@ranjib

This comment has been minimized.

Show comment
Hide comment
@ranjib

ranjib May 28, 2015

Member

👍 🏆
i concur with @coderanger , its not a breaking change.

a common requirement has been setting recipe specific resource defaults (like all file resource in a recipe can have a default owner/group). It will be awesome if we make the default value calculation recipe aware

Member

ranjib commented May 28, 2015

👍 🏆
i concur with @coderanger , its not a breaking change.

a common requirement has been setting recipe specific resource defaults (like all file resource in a recipe can have a default owner/group). It will be awesome if we make the default value calculation recipe aware

```ruby
class MyResource < Chef::Resource
property :path, Path, name_attribute: true

This comment has been minimized.

@jaymzh

jaymzh Jun 3, 2015

Member

Path is a type?

@jaymzh

jaymzh Jun 3, 2015

Member

Path is a type?

@adamhjk

This comment has been minimized.

Show comment
Hide comment
@adamhjk

adamhjk Jun 11, 2015

Contributor

This RFC is approved. It cleans up a clearly confusing overload of the term "attributes", provides clear back-compat, and useful new functionality.

Make back-compat a top priority when you implement, please. :)

/cc @chef/rfc-editors

Contributor

adamhjk commented Jun 11, 2015

This RFC is approved. It cleans up a clearly confusing overload of the term "attributes", provides clear back-compat, and useful new functionality.

Make back-compat a top priority when you implement, please. :)

/cc @chef/rfc-editors

@thommay

This comment has been minimized.

Show comment
Hide comment
@thommay

thommay Jun 25, 2015

Collaborator

Accepted as RFC 54, thank you all very much.

Collaborator

thommay commented Jun 25, 2015

Accepted as RFC 54, thank you all very much.

@thommay thommay closed this Jun 25, 2015

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