blt04 / puppet-rvm

A puppet module for installing and using RVM (Ruby Version Manager)
Other
242 stars 280 forks source link

Updates for rvm-rpm usage, selinux and other fixes #34

Open ssnodgra opened 12 years ago

ssnodgra commented 12 years ago

As promised, here are the updates for compatibility with RPM installs as well as adding selinux support for Redhat-ish systems. My apologies for the number of commits, I was experimenting with the best way to do things along the way.

The first requirement for RPM support was to make the whole thing independent of the rvm install location. In order to accomplish this, I created a new fact called rvm_binary which points to the rvm executable. Both the manifests and providers make use of this fact. Also, the variable $rvm::system::rvmpath contains the rvm installation path, normally /usr/local/rvm for a script install or /usr/lib/rvm for the RPM install. To get the packaged install, you set the use_pkg class parameter to true.

The other major bit is selinux support. This is conditional on the selinux fact, which will be true on selinux-enabled systems. The main part of this is accomplished by adding an after_install hook to rvm, which runs every time a ruby version is installed. It will adjust the selinux file contexts on the ruby directories. Also, there's an extra exec when building passenger, since the Apache-related executables need some special contexts. Note that I found a bug in rvm hooks along the way, see issues 744-745 on rvm for info.

This has all been tested on RHEL6. I have no Ubuntu systems so I would greatly appreciate it if you make sure I didn't break Ubuntu, and I'd more than welcome any other feedback/discussion on the changes. Thanks for providing this very helpful module!

blt04 commented 12 years ago

Steve, this looks great at first glance. I will test it out ASAP.

Would you consider rebasing the pull request and possibly squashing some of the commits to make it a little easier to see what exactly has changed? Basically squashing some of the "try this..., fix that..." commits that fix stuff added in one of your earlier commits. http://help.github.com/rebase/ I'll pull it regardless, but it would make it easier to go through each commit.

blt04 commented 12 years ago

Oh and thanks a ton for this!!!!

ssnodgra commented 12 years ago

Hmm, I never even knew rebase existed, let me take a look at this. I'm kind of confused about how pushing the rebasing will work. I will have to experiment. I can see how things could be made a lot clearer though.

ssnodgra commented 12 years ago

I'm still confused about how this works when I've already pushed commits (especially given the "It is considered bad practice to rebase commits which you have already pushed to a remote repo." warning). Do I need to destroy my puppet-rvm fork and then re-fork so I can push out rebased changes? I'm afraid I still have much to learn about git. :)

blt04 commented 12 years ago

Steve. Don't worry about it, I can pull as is.

But.... If you want to worry about it ;), here's how I would proceed:

Add my repo as a remote (if you haven't already):

git remote add upstream git://github.com/blt04/puppet-rvm.git
git fetch upstream

Create a new branch to work on (since you've already pushed to master):

git branch rpm master
git checkout rpm

Rebase the branch against upstream master:

git rebase upstream/master

There may be conflicts in the above. If so, resolve them, or "don't worry about it" and I'll pull as is.

After rebasing, rearrange and/or squash some of your commits to make it a little easier to read through the history:

git rebase -i HEAD~38

You may end up doing this over and over while you clean stuff up.

Finally, when it all looks good locally, push it to github and either update this pull request, or open a new one.

ssnodgra commented 12 years ago

Brandon, thanks for the explanation, it makes perfect sense. I did mess around with squashing some commits on a branch, but wow, it opened my eyes to how chaotic my commit sequence was on this one. Go ahead and pull this one as is, and I'll take this as a lesson on how to be a better gitizen in the future. Anyway, I highly recommend that you examine the final diffs rather than the individual commits on this one. :P

ssnodgra commented 12 years ago

I'm working on another patch to support rvm installing rubies and gems from local mirrors. I'll submit this as another pull request when it's ready (a better one, I promise!). One thing that is bugging me a little is that every time I add another parameter, I have to add it to the rvm class then pass it along to rvm::system. I'm starting to wonder if it isn't better just to eliminate the builtin run stage support and let the user specify a run stage if needed, which would do away with the need for the intermediate class. That's totally your call though, the code will work either way.

blt04 commented 12 years ago

Steve. Can we move the discussion on rvm parameters / stage support to a different github issue, just to keep this thread relating to your rpm pull request?

I finally started looking at your pull request. It appears to break one-pass multi-stage installs. Apparently facter only gets run once, regardless of how many stages there are. So when puppet tries to run the main stage after installing RVM, it fails to find the RVM binary (because the fact isn't loaded).

So either we need to do away with stage support, or assign the binary path via a variable (that would default to /usr/loca/rvm/bin/rvm). Neither of these are great solutions. I really like the idea of being able to do a complete install in one-pass. It helps with stuff like Vagrant and other server-less puppet set ups. Thoughts?

blt04 commented 12 years ago

Hey Steve, can you test the commit I just added to this pull request. It fixes the multi-stage problem for me.

The rest of the pull request looks great. It works on Ubuntu. I didn't test it on Redhat, but I know you are all over that. I'll pull it as soon as you verify I didn't screw everything up with d6e9527.

ssnodgra commented 12 years ago

I will test this more thoroughly, but looking at the commit, I see a problem with the reference to $rvmpath/bin/rvm. The RPM package bases the rvm install in /usr/lib, but the binaries just go into /usr/bin. So there is no /usr/lib/rvm/bin/rvm to execute.

One problem I had when developing this - my ruby and provider-writing skills are not great. If there is a way to access puppet DSL variables from the provider, the best solution might be to set an $rvm_binary variable inside puppet then access it from the provider, which would get rid of the fact. But I didn't know how to do that.

ssnodgra commented 12 years ago

Hold that thought - it just occurred to me that maybe scope.lookupvar that works for templates will also work inside a provider? I will try that out today if I get a chance.

Also, I meant that the RPM install is based in /usr/lib/rvm, not /usr/lib. :)

blt04 commented 12 years ago

I don't think you can access variables in a provider (unless they were passed to the provider via attributes (like name, ruby_version, etc). I could be wrong.

The big problem is that the rvm_binary is used to determine if the provider is suitable, and I think that all happens before any data is passed to the provider. Again, I could be wrong.

ssnodgra commented 12 years ago

Well, there's nothing really wrong with the path-search method you have implemented in the providers. I was just trying to create a single canonical source for the location of the rvm command. But if that won't work, we can just make an rvm_binary variable in the DSL and use that, and leave the providers with your latest change. Or, we could just add a path argument to any rvm execs that would catch rvm no matter where it is rather than making a DSL variable.

blt04 commented 12 years ago

I tried the second approach (add a path argument to rvm execs) and passenger failed to install. I think that may be the best compromise (at least the expected behavior is identical). I'll see if I can figure out why passenger wouldn't install.