fix cookbook loading in select_items

[ohlol]

I couldn't get the plugin to load my cookbooks to chef-zero without this.

[andrewgross]

Hey @ohlol. Can you give me a brief example of what your directory structure looks like? I would like to set up a test for it as the logic in this section is a bit hairy, so having some defined use cases would be really helpful.

Thanks, Andrew

[jmickle]

I ran into this same problem on https://github.com/andrewgross/vagrant-chef-zero/blob/master/lib/vagrant-chef-zero/action/upload.rb#L37 and https://github.com/andrewgross/vagrant-chef-zero/blob/master/lib/vagrant-chef-zero/action/upload.rb#L144

The select function gets passed a string and is always a directory when using the config.chef_zero.chef_repo_path param. WHich means the path is always /cookbooks/ and typically inside of here will be more directories for each cookbook. As a result the glob on #{path} for .json or .rb wont work as the files are typically nested in the cookbook folders themselves

[andrewgross]

Thanks, I'll check this out

[aaronbbrown]

To clarify, a normal chef cookbook directory looks like this

cookbooks/foo
cookbooks/foo/metadata.rb
cookbooks/foo/recipes
cookbooks/foo/recipes/default.rb
cookbooks/foo/attributes
cookbooks/foo/attributes/default.rb

So L144 above doesn't find any cookbooks.

[jmickle]

Correct, because l144 is looking for .json and .rb files in cookbooks/ and not any sub directories. Also You probably dont want to go based on .rb as it will pick up metadata.rb etc as well.

I patched my local copy with this

def select_cookbooks(path)
  if path.nil?
    path = []
  elsif path.is_a?(String) && File.directory?(path)
    path = Dir.glob("#{path}/*/")
  elsif path.is_a?(String) && File.exists?(path)
    path = [path]
  else
    @env[:chef_zero].ui.warn("Warning: unable to normalize cookbooks #{path}, skipping")
    path = []
  end
  path
end

which gets all names of all cookbook directories and allows the uploader to smart upload each of these items. The only problem with this which works in my case and why i didnt submit this as a PR is this assumes a user wants to always upload ALL cookbooks instead of testing just one cookbook or recipe. So the challenge here is to patch it to respect both respective portions. Although it sounds to me that by using the "intelligent" select variable you have eliminated to logic for single cookbook testing. Is that the original design?

[jmickle]

also i dont think this PR from @ohlol is a fix either, I think it needs to take into account the logic of how the cookbook path is being defined. Wether its being defined as a single cookbook/recipe or if its being defined as a "repo" each case will have different outcomes of how to test right. I may not always want to upload every single cookbook if i have a large repository it may take 15-20 min of upload time of cookbooks. Sometimes I only want to test a single recipe, while other times I want to test the whole thing

[jmickle]

any update on this bug?

[andrewgross]

Hey @jmickle, sorry for the delay. Working on getting my Vagrant/Ruby environments to behave so I can test some of these. Unfortunately having a fair amount of trouble with them at the moment.

[jstarcher]

@ohlol's patch worked great for me, but @jmickle has a fair point. In my case I'm testing several cookbooks so it worked great, but I could see the need for single cookbook/recipe support as well.

[petere]

How is one supposed to use this plugin if the cookbook loading doesn't work? Is there another way to load cookbooks?

[spheromak]

@petere you can use vagrant-berkshelf to upload cooks, but i would rather just have vagrant-chef-zero loaded. @andrewgross any thoughts on how you would like to see this implemented / fixed.. Right now this feature is broken as-is.

[spheromak]

@jmickle For my cases I would assume you want to include all cooks defined by the config value for cookbooks from the Vagrantfile. Weather its 1 or many. I implemented your patch as well locally here with good results. @andrewgross I am happy to submit a PR with that solution.

[jmickle]

@spheromak Thats correct, though the problem persists that there is a major logic flaw in the design of this feature. For example, in Repo mode it probably should assume that ALL cookbooks/roles/environments would want to be uploaded to the fake chef server. Though in singular mode you probably may want to break it down. Take for example I am developing a specific cookbook with no dependencies and I do not want to wait for 100+ other cookbooks to upload.

[spheromak]

@jmickle so you would want it to handle a dir where all you had was the metadata.rb and other "cookbook" files as well as a directory of cookbooks. Should be doable on that select_cookbooks method by looking for metadata.rb

[jmickle]

@spheromak yeah i think that should work, i also think that the expanding of other paths as in #27 might actually be fixed through the means of this fix as well. The root of the problem is path expansion/file selection

[andrewgross]

Hey all, sorry for the delay in getting around to this. I appreciate you all taking the time to discuss this. I have a tentative solution pushed in 0406fa9cae7066b9f457a003352ab29f51026434 with some tests that should hit the edge cases. It currently keeps the existing select_items and adds a new select_cookbooks method that works properly for the various cookbook cases. Please take a look @spheromak and let me know if you see any glaring inconsistencies with the logic.

I will investigate how to resolve some of the more meta concerns raised by @jmickle regarding loading all listed cookbooks. When I designed this I assumed that each cookbook you were working on would have it's own Vagrant file/block to allow you have relatively isolated control over what your Chef server environment would be.

Could you please elaborate a bit more on how you structure your Vagrantfile / cookbook setup when working on a 'single' cookbook vs 'repo' mode?