FIX data_bag_item.rb:161: warning: circular argument reference - data_bag

[haberbyte]

This fixes a warning in Ruby 2.2 about circular argument references in data_bag_item.rb

See this ruby issue for more details: https://bugs.ruby-lang.org/issues/10314

[tyler-ball]

I would like to see a test added to https://github.com/opscode/chef/blob/master/spec/unit/data_bag_item_spec.rb that ensures this does not regress in the future. There are no specs for the destroy test yet, so would you mind adding them? Reach out to us if you need help with that.

Otherwise, this LGTM. Although I am worried that we could be bitten by this Ruby change elsewhere.

[haberbyte]

Hmm. I'm not quite sure how a proper test would look like.
Testing for ruby warnings seems kind of odd to me.

I got a spec working like this, should i add it?
It is somewhat of a hack, so i don't like it at all. I think ruby warning should not be explicitly tested against. Maybe there is a rspec setting that makes the tests fail if the ruby interpreter throws a warning??

@orig_stderr = $stderr
$stderr = StringIO.new

require 'spec_helper'
require 'chef/data_bag_item'

stderr = $stderr
stderr.rewind
$stderr = @orig_stderr

describe Chef::DataBagItem do
  before(:each) do
    @data_bag_item = Chef::DataBagItem.new
  end

  describe "initialize" do
    it "should be a Chef::DataBagItem" do
      expect(@data_bag_item).to be_a_kind_of(Chef::DataBagItem)
    end

    it "does not warn on initialization" do
      expect(stderr.string.chomp).to be_empty
    end
  end

....

Although i think this kind of spec should be added somewhere else...or maybe not at all.

[tyler-ball]

I'm thinking you should add this to data_bag_item_spec.rb:

  describe "destroy" do
    let(:server) { instance_double(Chef::REST) }
    # TODO we should convert this whole file to `let` syntax
    let(:data_bag_item) {
      data_bag_item = Chef::DataBagItem.new
      data_bag_item.data_bag('a_baggy_bag')
      data_bag_item.raw_data = { "id" => "some_id" }
      data_bag_item
    }
    it "should set default parameters" do
      expect(Chef::REST).to receive(:new).and_return(server)
      expect(server).to receive(:delete_rest).with("data/a_baggy_bag/data_bag_item_a_baggy_bag_some_id")

      data_bag_item.destroy
    end
  end

When I run that locally with Ruby 2.2.0-preview1 I get the following rspec failure:

Failures:

  1) Chef::DataBagItem destroy should set default parameters
     Failure/Error: data_bag_item.destroy
       Double "Chef::REST (instance)" received :delete_rest with unexpected arguments
         expected: ("data/a_baggy_bag/data_bag_item_a_baggy_bag_some_id")
              got: ("data//data_bag_item_a_baggy_bag_some_id")
     # ./lib/chef/data_bag_item.rb:162:in `destroy'
     # ./spec/unit/data_bag_item_spec.rb:221:in `block (3 levels) in <top (required)>'

Which is what I expect. So the purpose of this test is not to test that we miss a Ruby warning - it is to test that we are upholding our internal contract. Namely, that we will construct that REST URL correctly.

Let me know if this doesn't make sense. Extra credit if you want to convert this whole file to let syntax!

[haberbyte]

Thanks, that makes sense.
I will add your spec and then try to convert the file to let syntax.

[lamont-granquist]

👍

[tyler-ball]

That let conversion looks sweeeeeeeeet @habermann24 - thanks for all your work on this! 💯 This will be merged in our next merge-a-thon

[adamdecaf]

Any update on when this could be merged and released?

[tonytonyjan]

+1, I'm looking forward the merging.

[gretel]

merge, pleeaaaase 💋

[gretel]

thanks! ❤️

[chef-supermarket]

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

GitHub Users Who Are Not Authorized To Contribute

The following GitHub users do not appear to have signed a CLA:

• @haberbyte

Please sign the CLA here.