Skip to content
This repository has been archived by the owner on Mar 19, 2022. It is now read-only.

Remove hard dependency on librarian #211

Merged
merged 7 commits into from
Mar 25, 2013

Conversation

tmatilai
Copy link
Collaborator

@tmatilai tmatilai commented Mar 9, 2013

We should not require Librarian (nor Berkshelf) to be installed, but instead check their presence at run time.

Only try to use the librarian gem if the Cheffile is found (and
`--no-librarian` is not specified). Complain to the user with
instructions if the gem is not found.

Also add a debug message is the Cheffile is not found.
@tmatilai
Copy link
Collaborator Author

tmatilai commented Mar 9, 2013

This removes the hard dependency but there is still a problem in default value of config[:librarian] when called indirectly with for example knife bootstrap --solo. I think I'll add a fix for it in this same PR. But some other day. =)

@tmatilai
Copy link
Collaborator Author

@matschaffer could you review (or even merge, yikes!) this PR? I'll fix the mentioned bug in #221.

@tmatilai
Copy link
Collaborator Author

Next Librarian version will extract librarian-chef to a separate gem. So we need to change the warning message at that time.

librarian v0.1.0 will not include librarian-chef any more,
as it will be extracted to own gem.
@tknerr
Copy link

tknerr commented Mar 25, 2013

I was just about to submit a PR for loosening the librarian dependency. I have now librarian-chef 0.0.1.beta.1 installed which depends on librarian 0.1.0. This now conflicts with knife-solo 0.3.0.pre2 (depends librarian ~> 0.0.20) in my bundle.

+1 for merging :-)

@tmatilai
Copy link
Collaborator Author

Btw, I tested knife-solo yesterday also with librarian(-chef) betas and everything worked fine. So only things we need to change after their release is the warning message and development dependency requirement.

@tmatilai
Copy link
Collaborator Author

@tknerr oh, and we could not just loosen the "librarian" gem dependency as 0.1.0 does not include all the classes we need. With librarian 0.1.0 the librarian-chef gem is needed. It is a good thing the previous releases locked the librarian requirement to 0.0.x. But obviously that can cause conflicts with other software. Bundler helps in many cases.

@tknerr
Copy link

tknerr commented Mar 25, 2013

@tmatilai fully agree!

@matschaffer any chance of a .pre3 release once this is merged in?

@matschaffer
Copy link
Owner

Hey guys, sorry just coming up for air after a massive b-day party for my son and mom. Should have some this week though. Any rough priority on the activity since yesterday would be appreciated. Either find me in the IRC channel or maybe just order them on http://huboard.com/matschaffer/knife-solo/board

if !File.exist? 'Cheffile'
Chef::Log.debug "Cheffile not found"
elsif !load_librarian
ui.warn [
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a big fan of heredoc + margin removal for this sort of thing (e.g., https://github.com/matschaffer/knife-solo/blob/master/lib/knife-solo/info.rb#L7) Might be time just just pull in https://github.com/rubyworks/facets/blob/master/lib/core/facets/string/trim.rb and standardize on that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack. But I don't want newlines inside the strings that go to ui.warn, so I'll split the sentences to separare ui.warn calls.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh.... It didn't occur to me that that's what you were going for here. I
wonder if there's a cleaner approach. I'll ponder it for a bit.

-Mat

On Mar 25, 2013, at 17:15, Teemu Matilainen notifications@github.com
wrote:

In lib/chef/knife/solo_cook.rb:

@@ -133,10 +131,30 @@ def time(msg)
end

   def librarian_install
  •    return unless File.exist? 'Cheffile'
    
  •    ui.msg "Installing Librarian cookbooks..."
    
  •    Librarian::Action::Resolve.new(librarian_env).run
    
  •    Librarian::Action::Install.new(librarian_env).run
    
  •    if !File.exist? 'Cheffile'
    
  •      Chef::Log.debug "Cheffile not found"
    
  •    elsif !load_librarian
    
  •      ui.warn [
    

Ack. But I don't want newlines inside the strings that go to ui.warn, so
I'll split the sentences to separare ui.warn calls.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/211/files#r3521250
.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The message construction was copied from another project doing a similar thing, so I had not spent too many seconds thinking how to do it. =)

The error message could also be improved further. Maybe something like:

Cheffile exists but Librarian-Chef could not be loaded.
Please add the librarian gem to your Gemfile or install it manually with gem install librarian.
Or you can skip Librarian and this warning by using --no-librarian option or by putting knife[:librarian] = false to knife.rb.

But now off to bed...

@matschaffer
Copy link
Owner

Just some stylistic points. Looks good otherwise.

@tmatilai
Copy link
Collaborator Author

Thanks a lot for reviewing! Really appreciate.

I added the modifications as new commits. I'll merge this later if there are no new comments.

tmatilai added a commit that referenced this pull request Mar 25, 2013
Remove hard dependency on librarian
@tmatilai tmatilai merged commit 8fdfb7b into matschaffer:master Mar 25, 2013
@tmatilai tmatilai deleted the librarian-dependency branch March 26, 2013 01:46
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.

3 participants