Modified Resources and Providers creation

[afiune]

This commit modified the way we create the Resources and Providers inside Cheffish.

Instead of creating them as:

class Chef::Resource::Blah

We now use:

class Chef
class Resource
class Blah

The same for the Providers.

The main reason for this is to open Chef class and consume what is in there. In this case the NOT_PASSED constant! We also check that the NOT_PASSED constant is not defined before declaring it.

This was failing on chefdk with chef version chef-12.5.0

Here the snippet of the error:

[1] pry(#<Chef::Recipe>)> my_role = chef_role 'salim'
=> #<Chef::Resource::ChefRole:0x3fe2ed1f4b44>
[2] pry(#<Chef::Recipe>)> my_role.run_action(:create)
Chef::Exceptions::ValidationFailed: Option name's value #<Object:0x007fc5da8cfdc8> does not match regular expression /^[.\-[:alnum:]_]+$/
from /opt/chefdk/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.0.current.0/lib/chef/mixin/params_validate.rb:299:in `_pv_regex'

[danielsdeleo]

This is okay, but it seems like this is only needed if you're requiring the same file twice, which shouldn't be happening.

[afiune]

We are just writing some tests .. :smile:

[tyler-ball]

@danielsdeleo what if we instance_exec or class_exec that file again? Or used that class to class_exec something else? Could that possibly be re-defining that constant?

@afiune you could also add

if defined?(NOT_PASSED)
  require pry; binding.pry
  NOT_PASSED = Object.new
end

And inspect when/how it is getting re-defined. I would be curious to know how its getting redefined if you have a repro.

[jkeiser]

The problem: if you declared a chef_role in 12.5, it would fail when trying to access the ChefRole.name.

The affecting Chef 12 change: This happened because the "name" property in 12.5 uses a new NOT_PASSED constant to determine whether the user passed a value or not. In other words, if you call name "hi" then value == "hi", if you call name with no arguments, value == NOT_PASSED.

The cause: Chef::Resource::Role defines its own NOT_PASSED, and the Ruby lookup meant that the base class (where name is defined) was sometimes picking up Chef::Resource::Role::NOT_PASSED and sometimes picking up Chef::NOT_PASSED. This meant comparisons would sometimes fail and weird things would happen.

To fix it, we:

1. Opened up the Chef class using class Chef; class Resource; class Role < Chef::Resource::LWRPBase instead of class Chef::Resource::Role < Chef::Resource::LWRPBase. This gave us access to the Chef::NOT_PASSED constant.
2. Did not redefine the constant if it was already available (this is backcompat for Chef 12.4 and below).

[lamont-granquist]

You need to not be using flat indenting, sooner or later someone like me will come along and do something significant enough that I want the autoformatting in my editor to work correctly and I'll remove all the flat indenting.

[afiune]

I would love to fix that ... It's up to John and Tyler to give the green light! 😁

On Wednesday, September 2, 2015, Lamont Granquist notifications@github.com wrote:

You need to not be using flat indenting, sooner or later someone like me will come along and do something significant enough that I want the autoformatting in my editor to work correctly and I'll remove all the flat indenting.

— Reply to this email directly or view it on GitHub https://github.com/chef/cheffish/pull/70#issuecomment-137243541.

Salim Afiune — Solutions Engineer

917.716.0448 – afiune@chef.io – my: Linkedin http://www.linkedin.com/in/afiune Twitter http://www.twitter.com/afiune

CHEF

  CHEF.IO <http://www.chef.io/>

TM

chef.io http://www.chef.io/ Blog http://www.chef.io/blog/ Facebook https://www.facebook.com/getchefdotcom Twitter https://twitter.com/chef Youtube https://www.youtube.com/getchef

[randomcamel]

@lamont-granquist Fair enough, but it seems reasonable to have it as a separate change, so it doesn't obscure everything else that's going on by marking every line of a file as changed.

[tyler-ball]

:+1:

[lamont-granquist]

adding ?w=0 to the url will remove the whitespace from the diffs

[afiune]

Oh that is true Lamont!!

[jkeiser]

I'll indent them before merge.

[lamont-granquist]

I'm :+1: on the code changes, though, it is the correct way to do all of this. Opening up classes with the colon notation tends to end badly...

[afiune]

hold on .. we are working on the indentation!

[randomcamel]

:+1:

[afiune]

almost done.. please hold :smile:

[afiune]

Done! :awyeah:

[lamont-granquist]

:+1:

[danielsdeleo]

There are two changes like NOT_PASSED = Object.new unless defined?(NOT_PASSED) still in there, I'd hope we don't need that now?

[jkeiser]

@danielsdeleo we still need that for 12.4 compatibility. 12.5 is when NOT_PASSED was introduced.

[jkeiser]

@danielsdeleo https://github.com/chef/cheffish/pull/70#issuecomment-137201782 has the full explanation.

[afiune]

Alright this is getting super exciting!!!

[danielsdeleo]

Ruby lookup meant that the base class (where name is defined) was sometimes picking up Chef::Resource::Role::NOT_PASSED and sometimes picking up Chef::NOT_PASSED

What are the different circumstances? I would think that you'd look things up in the lexical scope of the method definition, which got fixed here by doing class A; class B instead of class A::B

[jkeiser]

@danielsdeleo the issue was that the base class Chef::Resource (whose lexical scope and inheritance hierarchy has not changed) was picking up Chef::Resource::Role::NOT_PASSED when you called name with no arguments (since name is a property, it uses the NOT_PASSED thingy in a method that looks like def name(value=NOT_PASSED)). However, the code that actually compared it to NOT_PASSED (in params_validate, I think) picked up Chef::NOT_PASSED.

[danielsdeleo]

Ugh, yeah. Seems like we should either do the comparison in the class that defines that constant or just re-use the same one. I might actually advocate for back porting Chef::NOT_PASSED by opening up class Chef and adding NOT_PASSED = Object.new unless defined?(NOT_PASSED) since you need the compatibility.

[jkeiser]

Yeah, I'd be cool with that too.