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

Open
wants to merge 38 commits into
from

Conversation

Projects
None yet
2 participants
@ssnodgra

ssnodgra commented Feb 9, 2012

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!

Steve Snodgrass added some commits Feb 3, 2012

Steve Snodgrass
Merge remote branch 'upstream/master'
Conflicts:
	manifests/system.pp
@blt04

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 9, 2012

Owner

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.

Owner

blt04 commented Feb 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 9, 2012

Owner

Oh and thanks a ton for this!!!!

Owner

blt04 commented Feb 9, 2012

Oh and thanks a ton for this!!!!

@ssnodgra

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 9, 2012

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 Feb 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 9, 2012

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. :)

ssnodgra commented Feb 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 9, 2012

Owner

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.

Owner

blt04 commented Feb 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 9, 2012

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 Feb 9, 2012

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

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 15, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 16, 2012

Owner

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?

Owner

blt04 commented Feb 16, 2012

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 added a commit that referenced this pull request Feb 16, 2012

Don't use Facter rvm_binary in providers
Facter facts are not re-evaluated for each install stage.  This commit
uses a different mechanism to locate the rvm binary.

Refs #34.
@blt04

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 16, 2012

Owner

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.

Owner

blt04 commented Feb 16, 2012

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

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 16, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 16, 2012

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. :)

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

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 16, 2012

Owner

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.

Owner

blt04 commented Feb 16, 2012

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

This comment has been minimized.

Show comment Hide comment
@ssnodgra

ssnodgra Feb 16, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@blt04

blt04 Feb 16, 2012

Owner

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.

Owner

blt04 commented Feb 16, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment