`knife cookbook site install` with specific version fails

No PEBKAC

I've put in a one-character pull request (https://github.com/opscode/chef/pull/128) that I believe fixes the bug.

No PEBKAC, great.

Did some slight adjustments so that the user still gets notified that the installation of multiple cookbook is not yet possible. Can you take a look at https://github.com/opscode/chef/pull/131 and let me know what you think?

By the way, don't know if it's the best workflow to open up another pull request. Let me know if this can be done otherwise in a more precise way and I'll stick to that next time.

Thanks,
Mike

Un-triaging so the team can evaluate both at the next triage meeting.

The regex in the second pull request doesn't match dots, so it wouldn't match a version like '0.1.x'

If some tests were written for cookbook site install, we might be able to catch this.

Linking to a git branch is easier than a pull request in the future.

http://wiki.opscode.com/display/chef/How+to+Contribute

Thanks guys!

We're still looking for a couple tests written around this regex. Can anyone help here?

New branch: https://github.com/stevendanna/chef/tree/CHEF-2512-w-tests

The regex assumes valid versions look like x.y or x.y.z, which is what we assume in chef/lib/chef/version_class.

The tests are pretty basic and stub out pretty much everything, but they should at least cover the argument parsing.

there are some pull requests for this that we should close when we merge.

I have three failing tests. Steven can you take another look here?

Failures:

  1) Chef::Knife::CookbookSiteInstall run should install the specified version if second argument is a three-digit version
     Failure/Error: @knife.run
     Errno::ENOENT:
       No such file or directory - /var/tmp/chef/getting-started.tar.gz
     # ./lib/chef/knife/cookbook_site_install.rb:87:in `unlink'
     # ./lib/chef/knife/cookbook_site_install.rb:87:in `run'
     # ./spec/unit/knife/cookbook_site_install_spec.rb:101:in `block (3 levels) in <top (required)>'

  2) Chef::Knife::CookbookSiteInstall run should install the specified version if second argument is a two-digit version
     Failure/Error: @knife.run
     Errno::ENOENT:
       No such file or directory - /var/tmp/chef/getting-started.tar.gz
     # ./lib/chef/knife/cookbook_site_install.rb:87:in `unlink'
     # ./lib/chef/knife/cookbook_site_install.rb:87:in `run'
     # ./spec/unit/knife/cookbook_site_install_spec.rb:112:in `block (3 levels) in <top (required)>'

  3) Chef::Knife::CookbookSiteInstall run should install the latest version if only a cookbook name is given
     Failure/Error: @knife.run
     Errno::ENOENT:
       No such file or directory - /var/tmp/chef/getting-started.tar.gz
     # ./lib/chef/knife/cookbook_site_install.rb:87:in `unlink'
     # ./lib/chef/knife/cookbook_site_install.rb:87:in `run'
     # ./spec/unit/knife/cookbook_site_install_spec.rb:123:in `block (3 levels) in <top (required)>'

Thanks Bryan. The good news is that I think the problem is with the tests rather than the real code. My original patch did this (in the tests):

+    #Assume all external commands would have succeed. :(
+    @knife.stub!(:shell_out!).and_return(true)

A subsequent commit by John Keiser against master removed all the ugly shelling out to use File methods, which my tests don't stub. If I get some time today I will try to make these tests a bit more resilient.

I pushed a small commit to the CHEF-2512-w-tests branch that should still easily rebase against master and pass the test. All I did was add some stubs to the relevant File methods and then clean up some other stubs.

Merged to master [yesterday].