-
-
Notifications
You must be signed in to change notification settings - Fork 374
Improve support for pip on CentOS7/EPEL #347
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
Conversation
Due to a recent change in EPEL the python-pip package was renamed to python2-pip. This currently breaks this module on CentOS 7. This commit makes sure that it defaults to python2-pip if the system is running Python 2 on CentOS 7 when using EPEL.
@@ -213,6 +213,9 @@ | |||
if $::python::version =~ /^3/ { | |||
$pip_category = undef | |||
$pip_package = 'python3-pip' | |||
} elsif ($::osfamily == 'RedHat') and (versioncmp($::operatingsystemmajrelease, '7') >= 0) and ($python::use_epel == true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled use_epel since I have manage it elsewhere, but still would like this case to apply, which is why I like #349 better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably misinterpreted $use_epel
when writing this. I'll remove it.
As far as #349 is concerned: it doesn't do any spec tests and uses a regex on a fact that can be done cheaper and more effectively with versioncmp()
and $::operatingsystemmajrelease
.
@@ -217,6 +217,22 @@ | |||
end | |||
end | |||
|
|||
context "on a Redhat 7 OS" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work, thank you!
Would you mind adding a case for RedHat 6, that shows the same thing as this, but with the correct pip package name and update the existing RedHat 5 part with the name attribute. That would give us much better testing for all use cases with EL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a change that adds tests for 6 and tests the name on 5 too.
I would suggest rewriting the spec tests at some point so there isn't that much divergence between the different operating systems. E.g.
require 'spec_helper'
describe 'python', :type => :class do
context 'supported operating systems' do
on_supported_os.each do |os, facts|
context "on #{os}" do
Something for the future.
@aschaber1 We need a third party to validate that this code actually works on EL7. Would you be able to do that and report back? |
@ghoneycutt, just tried it: works.
RPM Output:
|
Thank you! |
Released in 1.18.2 |
I don't see python3-xxx (including python3-pip) available on CentOS7, even with epel repo enabled, though I do see python34-pip via our EPEL mirror. It's also not clear to me that this would help, even with python2-pip absent and python34-pip present, seems to me like the module is still hard-coded to use "pip". Some additional comments in #303. |
This pull request only solved an issue where packages were renamed by EPEL. What you're describing is a different issue. I think it would be better to discuss this in #303. |
Now uses the correct package name 'python2-pip'. Due to a recent change in EPEL the python-pip package was renamed to python2-pip. This currently breaks this module on CentOS 7. This commit makes sure that it defaults to python2-pip if the system is running Python 2 on CentOS 7 when using EPEL. * Remove the unneeded dependency on EPEL * Added pip package tests on RHEL 5 and 6
Due to a recent change in EPEL the python-pip package was renamed to python2-pip. This currently breaks this module on CentOS 7. This commit makes sure that it defaults to python2-pip if the system is running Python 2 on CentOS 7 when using EPEL.