Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search for the knife.rb like chef does #383

Merged
merged 8 commits into from
Apr 8, 2013
Merged

Search for the knife.rb like chef does #383

merged 8 commits into from
Apr 8, 2013

Conversation

sethvargo
Copy link
Contributor

See: #382

@sethvargo
Copy link
Contributor Author

@reset @schisamo, added specs


# Ascending search for .chef directory
Pathname.new(working_dir).ascend do |file|
possibles << file.expand_path if file.basename == '.chef'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be something like:

candidate_directory = File.join(file.expand_path, '.chef')
if File.exist?(candidate_directory) && File.directory?(candidate_directory)
  possibles << File.join(candidate_directory, 'knife.rb')
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it's fundamentally the same, no?

Search upward and add anything that is .chef. I could add the directory? check, but the actual existence check is done later at line ~45.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the directory check in 0b8005a

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directory check wasn't my concern, I'm confused on how this code:

Pathname.new(working_dir).ascend do |file|
  possibles << file.expand_path if file.basename == '.chef' && file.directory?
end

will ever actually get a .chef directory. For example:

 % irb
irb(main):001:0> require 'pathname'
=> true
irb(main):002:0> p = Pathname.new(Dir.pwd)
=> #<Pathname:/Users/schisamo/dev/code/opscode-cookbooks/opscode-ci>
irb(main):003:0> p.ascend{ |file| puts "Path = #{file}, Basename = #{file.basename}" }
Path = /Users/schisamo/dev/code/opscode-cookbooks/opscode-ci, Basename = opscode-ci
Path = /Users/schisamo/dev/code/opscode-cookbooks, Basename = opscode-cookbooks
Path = /Users/schisamo/dev/code, Basename = code
Path = /Users/schisamo/dev, Basename = dev
Path = /Users/schisamo, Basename = schisamo
Path = /Users, Basename = Users
Path = /, Basename = /
=> nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I just realized this in refactoring my tests. Push coming soon 😄

@sethvargo
Copy link
Contributor Author

@schisamo re-review please when you have a chance?

@ivey
Copy link
Contributor

ivey commented Apr 6, 2013

Are we merging this, or moving to a new gem?

@sethvargo
Copy link
Contributor Author

I'm indifferent. @reset?

@reset
Copy link
Contributor

reset commented Apr 8, 2013

We should merge it in and then refactor it if at anytime a gem exists which replicates the functionality

reset added a commit that referenced this pull request Apr 8, 2013
Search for the knife.rb like chef does
@reset reset merged commit ddb874f into berkshelf:master Apr 8, 2013
@sethvargo sethvargo deleted the 382-find_knife branch April 8, 2013 20:24
@berkshelf berkshelf locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants