Add resource current value loading to actions, and converge helper

[jkeiser]

No description provided.

[jkeiser]

@coderanger had some good feedback which I haven't had time to update this proposal with. The three biggies were:

1. The word converge is probably wrong, especially since this is only really needed by people who write raw resources (resources that use Ruby APIs rather than resources to do the converge work).
2. Resource output (turning resources into a read API) could exacerbate users' confusion with the compile/converge model by making it too easy to read data during the compile phase. His suggestion to move that bit of the proposal to "actions only" might be correct, but I'd like to mull over ways to get output to users who know what they are doing, without jumping through hoops.
3. Finding out if an attribute was set by the user or not can be hard in an API that returns values even if you didn't set them; you can't just check if the response is nil. This problem already exists for values with defaults, but it would happen more often when you add in read values. I have added is_set? since then (which helps with defaults as well and may be useful regardless of this proposal).

[lamont-granquist]

There's really 4 different states of resource parameters and we only capture two:

1. the default state where no parameters have been changed
2. the existing state on the box (current_resource)
3. the requested state by the user (new_resource)
4. the final state on the box

We diff 2 and 3 in order to make run reports, but we really should be diffing 2 and 4 and hacking up #3 for reporting causes all kinds of issues if we later wind up re-using the new_resource and we've lost he original meaning of what the user requested. There's also most likely plenty of existing bugs where providers mutate the new_resource and corrupt the original meaning.

[jkeiser]

@lamont-granquist What is reporting doing to new_resource?

[lamont-granquist]

reporting doesn't do it directly. see chef/chef#3295 for more details.

[jkeiser]

@coderanger @lamont-granquist I have modified the proposal to remove current-value loading from the actual front-facing resource, so that we can have that discussion separately; and renamed load to load_actual_value. This RFC is now ready for review to the best of my knowledge.

One thing that is worth looking at: it might be worth renaming load_actual_value to load_current_value or load_current, to be consistent with other things; I'm resisting it at the moment because "actual" is clearer to me, but there is prior art to consider.

[coderanger]

I don't think this is actually possible.

[jkeiser]

Oh! I'll fix that one. I was using def load_actual_value! at first.

[coderanger]

I'm still really not very enthused about these new APIs overall. Given the target is a very narrow niche of people that write resources from scratch (i.e. not based on existing resources for the most part) I would rather see them done in a helper library and iterated outside of core for a bit.

[coderanger]

This seems kind of unfortunate, but I agree I don't know of a better option. As an API it seems really silly though.

[jkeiser]

Another possibility is actual_value_exists? { File.exist?(path) }. Not as much a fan of that, but it can make sense.

[jkeiser]

@coderanger some form of load is necessary--there is presently no way to create a load_current_value for the actions to use, and that's a blocker for turning old resources into action resources.

As for putting it in an outside library so it can be used and iterated upon, load and converge have been in the resource cookbook for a while and used with great success :)

[coderanger]

@jkeiser The changes you just made to use a block to define methods on the action class should work for #load_current_resource I would think?

[jkeiser]

@coderanger super() is how you would do it with the block-based API, yeah.

[coderanger]

@jkeiser I don't feel like the resource cookbook is a great example of being proven out in the community given relatively low uptake, but that's a catch 22 for you so :-/ This feels like adding an API no one other than you has asked for and it does add considerable complexity. I feel like this is a small step in your vision for Resources 2.0 but we still have no actual overall view of that vision, let alone agreement that it is what we should be moving towards.

[jkeiser]

@coderanger Like I said, there's no way to avoid having load_actual_value or something like it--there isn't another way to attach methods to the action provider currently, and we've shied away from adding that in another PR (which I agree with you on).

converge is surely an optional helper, but reduces risk and complexity for the resource writer to a huge degree, removing a lot of boilerplate and doing several things right that are easy to forget. You want to ask people about things that frighten them off when trying to write a resource, have them look at this:

if current_resource
  # We're updating; look for properties that the user wants to change (do the "test" part of test-and-set)
  differences = []
  if (new_resource.property_is_set?(:foo) && new_resource.foo != current_resource.foo)
    differences << "foo = #{new_resource.foo}"
  end
  if (new_resource.property_is_set?(:bar) && new_resource.bar != current_resource.bar)
    differences << "bar = #{new_resource.bar}"
  end
  if (new_resource.property_is_set?(:baz) && new_resource.baz != current_resource.baz)
    differences << "baz = #{new_resource.baz}"
  end

  if !differences.empty?
    converge_by "updating FooBarBaz #{new_resource.id}, setting #{differences.join(", ")}" do
      FooBarBaz.create(new_resource.id, new_resource.foo, new_resource.bar, new_resource.baz)
    end
  end

else
  # If the current resource doesn't exist, we're definitely creating it
  converge_by "creating FooBarBaz #{new_resource.id} with foo = #{new_resource.foo}, bar = #{new_resource.bar}, baz = #{new_resource.baz}" do
    FooBarBaz.update(new_resource.id, new_resource.foo, new_resource.bar, new_resource.baz)
  end
end

Instead of this:

converge do
  if current_resource
    FooBarBaz.update(new_resource.id, new_resource.foo, new_resource.bar, new_resource.baz)
  else
    FooBarBaz.create(new_resource.id, new_resource.foo, new_resource.bar, new_resource.baz)
  end
end

In one case, there is a huge amount of ceremony you have to go through if you want to do things right; in the other, you concentrate only on your own code rather than handholding Chef.

[jkeiser]

And the above assumes that on update, unspecified values for foo, bar and baz are set to something sane (which the proposal addresses).

I'll put the above example into the RFC for comparison purposes.

[coderanger]

@jkeiser Maybe I'm confused, I thought to goal of chef/chef#3660 was to be able to define methods on the fused provider class.

[jkeiser]

@coderanger you rightly pointed out that you really shouldn't need to put (much of?) your crap in that class, so I'm backing off on that.

[coderanger]

Okay, then yes back on board with load_actual_value, though I would reeeeally rather call it load_current_value to keep it clear those two APIs are related. We can talk about renaming both of them some other time methinks, but better to keep the correlation even if we slightly dislike the old name.

[jkeiser]

Yeah, I can do that.

[coderanger]

With that I'm back 👍. I feel like we could go two ways with #converge, either leave the name generic and slowly add different convergence-related features to it or make the name something more specific and add other helpers later. I think the former is fine if we mark is experimental (a la audit mode, policyfiles) to allow for API changes before Chef 13 as we figure out use cases :-)

[jkeiser]

I'm seriously all ears for different names :) I haven't thought of one better yet, but it does gnaw at me.

[coderanger]

Random ideas without regard for actual quality: auto_converge, converge_properties, converge_values (or some combo of those words).

[jkeiser]

Maybe test_and_set or converge_if_changed ...

[coderanger]

Naming is hard, let's just call it #74853800-7fad-48b8-905e-cafead22c6f0 and go get lunch :-)

[jkeiser]

Deal!

[jkeiser]

@coderanger renamed converge to converge_if_changed and load to load_current_value per comments here (no other changes).

[coderanger]

Should be

`super()` in `load_current_value` will call the superclass's `load_current_value` method.

[adamhjk]

Okay - this seems to have universal good times. 👍 /deciderated

[nathenharvey]

@chef/rfc-editors this appears ready for merge...

[coderanger]

Doneburger.