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

Enable use of vagrant-omnibus plugin in generated vagrant files #799

Merged
merged 1 commit into from
Aug 20, 2013

Conversation

pghalliday
Copy link
Contributor

Opscode no longer provide base boxes with chef preinstalled and instead recommend the use of the vagrant-omnibus plugin to specify the chef version to install.

These changes allow a chef version to be specified in the berkshelf config so that it can be set by default in generated cookbooks.

As a bonus I have added a cookbook and vagrant file to the project in a separate commit so that vagrant can be used for development of berkshelf.

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@pghalliday appreciate the contribution! This might be a bit too much over engineering, though. I actually think the right course of action is to just flat out set these defaults instead of what we had before:

  config.vm.box      = "opscode_ubuntu-12.04_provisionerless"
  config.vm.box_url  = "https://opscode-vm-bento.s3.amazonaws.com/vagrant/opscode_ubuntu-12.04_provisionerless.box"
  config.omnibus.chef_version = :latest

We should also then make Vagrant-Omnibus a dependency of Vagrant-Berkshelf to ensure that the user has the Vagrant-Omnibus plugin installed.

@ivey @sethvargo what do you think?

@ivey
Copy link
Contributor

ivey commented Aug 14, 2013

I agree that Berkshelf should set things up properly, so that includes vagrant-omnibus as a vagrant-berkshelf dependency.

I do like having a Vagrantfile for berkshelf itself, though.

@tknerr
Copy link
Contributor

tknerr commented Aug 14, 2013

@reset @ivey check out fgrehm/bindler - instead of introducing vagrant-omnibus as a gem dependency you can use a 'plugins.json' file and then just run 'vagrant plugin bundle'. The plugins.json would then contain both the vagrant-berkshelf and vagrant-omnibus plugins (with pinned versions if you prefer).

This makes it more transparent for the user and allows to manage the vagrant dependencies on a per project level.

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@ivey I don't see the value in having the Vagrantfile in the project. It's just going to pull the version of Berkshelf that was installed by the Vagrant-Berkshelf gem.

I do think that we should begin releasing the Berkshelf gem and the Vagrant-Berkshelf gem in lock step similar to how Chef, Merb, Rails is/was packaged.

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@tknerr I don't think that tool applies for this situation. We would have to generate a plugins.json and then instruct the user to run vagrant plugin bundle instead of just getting Vagrant-Omnibus by running vagrant plugin install vagrant-berkshelf.

@tknerr
Copy link
Contributor

tknerr commented Aug 14, 2013

@reset or to put it the other way around: please don't introduce a gem dependency between vagrant-berkshelf and vagrant-omnibus, otherwise you are taking people the freedom to update/manage their omnibus plugin independent from vagrant-berkshelf or at least you are introducing a potential conflict here.

Vagrant-berkshelf does not need vagrant-omnibus -- it's the Vagrantfile you choose to generate that needs both plugins!

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@tknerr no, we would not be restricting somebody from managing their omnibus plugin independently. This would be a greedy match of >= 0.0.0.

@tknerr
Copy link
Contributor

tknerr commented Aug 14, 2013

@reset still, I don't like the idea of adding this as a gem dependency, because it's really not a dependency of vagrant-berkshelf. I'm using a lot of vagrant plugins, they are all adding isolated features and are independent of each other. None of them depends on another vagrant plugin. It's always the Vagrantfile which depends on one or more plugins being present, but never the plugins depending on each other. I'm just saying this is likely to confuse vagrant users.

If you add it as a gem dependency:

  • What will 'vagrant plugin list' report? Will vagrant-omnibus show up there as a separate plugin?
  • What if I have another version of vagrant-omnibus already installed? Will it "upgrade" my existing version? Is that what I would expect? Would it replace the existing plugin or introduce conflicts?
  • What if I decide to use another basebox which has Chef installed already? Is it sufficient to remove the omnibus config from the Vagrantfile?
  • Can I uninstall the vagrant-omnibus part of vagrant-berkshelf if I wanted to?

I'd rather avoid that complexity and find another solution, e.g.:

  • let the user choose to generate the Vagrantfile with either provionerless or provisionerful baseboxes via cmdline switch. If provisionerless is used show a message that he must take care of installing vagrant-omnibus himself (in addition to vagrant-berkshelf)
  • detect if vagrant-omnibus is already installed and generate the Vagrantfile accordingly
  • add a postinstall message to vagrant-berkshelf that users might want to install vagrant-omnibus as well

@pghalliday @reset what do you think?

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@tknerr

  • Yes, it will show up as a separate plugin
  • No, it won't upgrade the plugin or conflict with your other constraints
  • Yes, you can just remove the omnibus plugin config from the Vagrantfile
  • No, you can't uninstall it. It wouldn't matter, though. Disk space is cheap and Vagrant-Omnibus has no gem dependencies of it's own so there isn't even a possibility of a gem activation conflict.

I do not agree that more burden should be placed on new cookbook authors. Why should they also need to know what Vagrant-Omnibus is? Berkshelf should be generating projects that work out of the box the way that we intend people to work.

@pghalliday
Copy link
Contributor Author

I'm glad this has generated so much interest :)

Perhaps, first I should explain why I did it the way I did.

  • I wanted to make sure that whatever change I made would not impact anybody's existing workflow
  • but I wanted to be able to configure Berkshelf for myself to have a default vagrant configuration that used omnibus
  • Naturally I wanted to minimize the size of the change to focus on the value I wanted to achieve

That said, here's what I think with regard to making this default behaviour and how.

  • It may (will?) break people's existing workflows
    • so will possibly need a major version bump to reflect this
  • Bindler may be a good solution to managing plugins that I have also been looking at but honestly I have no experience with it
    • my first impression was that it might result in plugin conflicts between different projects from different sources but I think the vagrant plugin model may suffer from that anyway. Unless I haven't looked into it enough and actually it maintains plugins local to projects - which would be awesome!
  • Making vagrant-omnibus a gem dependency could work given @reset's comment
    • It also makes sense if you want to make the use of omnibus the default
    • my only concern was controlling the version of omnibus installed
      • what if a breaking change is introduced in vagrant-omnibus?
      • do you fix the version of vagrant-omnibus installed?
        • if you do then don't you tie yourself to their release schedule?
          • I have this problem in a grunt plugin I maintain for another tool and it's a bit of a pain

In the interest of having a non breaking change for now, I still like the way I actually implemented it.

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@pghalliday making vagrant-omnibus a dependency of vagrant-berkshelf with the constraint of >= 0.0.0 will simply ensure that it is installed. If the user already has it installed, nothing will happen. It is still up to the end user to manage the vagrant-omnibus plugin if they want a newer or older version.

If a release of vagrant-omnibus goes out the door which is dead on arrival, it should be yanked from Rubygems. Since we are not actually integrating with the plugin in anyway and are simply listing it as a dependency to ensure that it's installed on the system, we will not run into any compatibility issues.

My proposed change would not actually break anyone's workflow. It would only affect the Vagrantfile of newly generated cookbooks. If somebody doesn't want to use vagrant-omnibus they can always uncomment that line out of their generated Vagrantfile. Since most people either: A) stick with the default box we ship, or B) change it to a box they want or to use vagrant-omnibus, this is a no-risk change.

If you'd like to make the more simplified proposed changes and rebase or re-issue your PR that would be awesome. I don't think the current pull request will be accepted. It was a super valuable conversation starter though! :)

@pghalliday
Copy link
Contributor Author

@reset, I'm thinking about the workflow of creating cookbooks.

If the default behaviour changes to one that will install the latest version of chef on the vagrant test platform for any new cookbook, this will be a breaking change for anyone who only wants to create cookbooks for a particular version of chef and uses a base box with that installed. It will add an additional step for those people to remove the omnibus configuration after using the berks cookbook command

I'm not saying that such a breaking change is not acceptable though - it would suit me ;)

@pghalliday
Copy link
Contributor Author

@reset, also the change you propose is to both berkshelf and vagrant-berkshelf so people will have to update both to prevent a breakage (and pull requests will need to be submitted to both).

Currently vagrant-berkshelf is not included in berkshelf as far as I know and even if it were it would not help in my workflow at least.

I run the berks cookbook command from a vagrant box with berkshelf installed and then go the host to run vagrant with the cookbook I have under development (I don't have berkshelf installed on the host, only vagrant, vagrant-berkshelf and vagrant-omnibus).

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@pghalliday the specific version of Chef can be provided instead of the symbol :latest to the vagrant-omnibus plugin. This is quite an improvement ontop of what we currently provide the end user which is a basebox that contains Chef 10.4.

@tknerr
Copy link
Contributor

tknerr commented Aug 14, 2013

@reset not being able to uninstall the vagrant-omnibus plugin would be an issue for me. Sure I can comment out the omnibus config from the Vagrantfile, but I'd also like to be under control of my vagrant plugins :-)

Just saying there are happy users of vagrant-berkshelf out there which don't use Berkshelf for generating the Vagrantfile.

@pghalliday bindler in fact installs the vagrant pluins in the a project-local .vagrant directory -- thus it solves the problem of inter-project vagrant plugin conflicts quite nicely!

@pghalliday
Copy link
Contributor Author

@reset, I'm happy with that as long as you're ok that existing users will need to change their configuration to specify the chef version on upgrade if they want to stay on 10.4

Also I still think there should be an option to disable omnibus in the berkshelf configuration if people have their own base box that they don't want touched (maybe they have a personal fork of chef or something)

This would mean changing the defaults to

config.vm.box      = "opscode_ubuntu-12.04_provisionerless"
config.vm.box_url  = "https://opscode-vm-bento.s3.amazonaws.com/vagrant/opscode_ubuntu-12.04_provisionerless.box"
config.omnibus.enabled= true
config.omnibus.chef_version = :latest

EDIT: sorry that must look a bit garbled, obviously I won't put the enabled option in the Vagrantfile but I hope you get what I mean

@pghalliday
Copy link
Contributor Author

@tknerr, I'll definitely look at bindler some more and maybe I'll submit a subsequent patch to add bindler support in the future :) Right now though it smells of scope creep ;)

@pghalliday
Copy link
Contributor Author

@reset, are you sure you want to switch from CentOS to Ubuntu too? Again this suits me but maybe not others.

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@pghalliday since the default value of the box url is configurable I don't think it matters much. I linked the Ubuntu one because that was historically the most common OS of the Chef community. That might not be true anymore.

Here's the equivalent CentOS link: https://opscode-vm-bento.s3.amazonaws.com/vagrant/opscode_centos-6.4_provisionerless.box

@pghalliday
Copy link
Contributor Author

@reset, I changed the last commit to default to using omnibus and the ubuntu image (this is the one I use anyway).

I left the enable parameter in though for the reasons stated above as I think it should at least be possible to set it to not use omnibus. If you really think this is unnecessary then i can remove it.

@sethvargo
Copy link
Contributor

I'm 👎 on including a Berksfile/Vagrantfile in Berkshelf. I think this would be better suited in a new repo - berkshelf-dev-vm or something. To clarify - I like it, just not here 😄. I'd like to merge 7dede74 and then CP 7450151 as the start of a new GitHub project for a dev vm.

I'm 👍 on the omnibus changes though.

@pghalliday
Copy link
Contributor Author

@sethvargo that would be great!

In retrospect I think there is still some work to do getting all the tests to pass in vagrant - I'm having trouble with Mercurial in particular.

I'll start a new repo for that and happily transfer it over to you guys if you want. In the mean time I'll see if I can't drop that commit from this pull request

@pghalliday
Copy link
Contributor Author

I have split the vagrant stuff into this project for now

https://github.com/pghalliday/berkshelf-dev-vm

@reset
Copy link
Contributor

reset commented Aug 14, 2013

@pghalliday I think the appropriate place for the Berkshelf client recipe is the Berkshelf cookbook https://github.com/berkshelf/berkshelf-cookbook

@pghalliday
Copy link
Contributor Author

@reset yeah, i think your right but i'm not sure it's very stable right now and could probably do with some work to make it more generic.

I should confess that actually a lot of tests fail when i run them in vagrant (particularly now that I have discovered the cucumber scenarios) and i'm still investigating that.

@pghalliday
Copy link
Contributor Author

Can someone give Travis a nudge on this. It seems the build passed on 1.9.3 but the 2.0.0 build stalled for some reason. I suspect if you restart it, it will pass

@ivey
Copy link
Contributor

ivey commented Aug 15, 2013

restarted

@pghalliday
Copy link
Contributor Author

thanks, but gah it failed again - i'm pretty sure it's not related to the changes in this pull request though. I've been getting the same errors when I run tests on master (I thought they were to do with my vagrant config though)

reset added a commit that referenced this pull request Aug 20, 2013
Enable use of vagrant-omnibus plugin in generated vagrant files
@reset reset merged commit 434b8f2 into berkshelf:master Aug 20, 2013
@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.

5 participants