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

Update version validation #472

Merged
merged 5 commits into from
Apr 10, 2019
Merged

Conversation

bodgit
Copy link
Contributor

@bodgit bodgit commented Apr 3, 2019

Pull Request (PR) description

Update the version parameter validation to accept SCL-style Python versions, i.e. rh-python36

This Pull Request (PR) fixes the following issues

Fixes #471
Fixes #475

@bastelfreak
Copy link
Member

Hi @bodgit, thanks for the PR. Can you add some tests so we know that the issue is actually fixed?

@bastelfreak bastelfreak added bug Something isn't working needs-tests labels Apr 4, 2019
Move SCL checks to the EL6/7 contexts as EL5 is neither tested against
nor does it support SCL repositories.
@bodgit
Copy link
Contributor Author

bodgit commented Apr 8, 2019

@bastelfreak Fixed. I noticed a few problems with the tests:

  1. It's keyed off facts[:os]['name'] and only checks for Red Hat so CentOS is ignored completely. There are no Red Hat sample facts so basically none of these tests ever fire.
  2. Most of the checks for Red Hat are within the EL5 context, and as only EL6+ is supported they're still not run.
  3. SCL doesn't work on EL5 anyway so I've moved those tests specifically to the EL6 and EL7 contexts.

The tests now fail for on master and are fixed on this branch, so they're in a better state than they were.

@@ -29,7 +29,7 @@
# @example install python3 from scl repo
# class { 'python' :
# ensure => 'present',
# version => 'rh-python36-python',
# version => 'rh-python36',
Copy link
Member

Choose a reason for hiding this comment

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

mhm, is -python not valid anymore? That would be a backwards-incompatible change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either work to install the runtime as both packages basically require each other, but as I mentioned in #471 if you specify the version as rh-python36-python and elect to install the dev package, it will try and install rh-python36-python-scldevel which doesn't exist, yet rh-python36-scldevel does exist.

The original code has clearly been written with using rh-python36 as the version as there is a commented out block that would have installed ${version}-python-virtualenv which would also error if you tried to use rh-python36-python for the version, (too many -python-'s). It's been commented out as the rh-python36 package will pull this in as a dependency, but rh-python36-python will not.

I'll update the regex so it doesn't break existing users but personally I think it needs a bit of work in the future; arguably the version should actually be rh-python36-python and the dev option should install rh-python36-python-devel rather than rh-python36-scldevel, and rh-python36-python-virtualenv should be explicitly installed rather than be commented out like it is currently.

For reference, these are the "core" packages that are available for the 3.6 software collection:

rh-python36.x86_64 : Package that installs rh-python36
rh-python36-runtime.x86_64 : Package that handles rh-python36 Software Collection.
rh-python36-scldevel.x86_64 : Package shipping development files for rh-python36
rh-python36-python.x86_64 : Version 3 of the Python programming language aka Python 3000
rh-python36-python-debug.x86_64 : Debug version of the Python 3 runtime
rh-python36-python-devel.x86_64 : Libraries and header files needed for Python 3 development
rh-python36-python-setuptools.noarch : Easily build and distribute Python packages
rh-python36-python-virtualenv.noarch : Tool to create isolated Python environments
rh-python36-python-wheel.noarch : A built-package format for Python

@bastelfreak bastelfreak added needs-feedback Further information is requested and removed needs-tests labels Apr 8, 2019
Allows the python3_version fact to be bypassed fully.
@bastelfreak bastelfreak removed the needs-feedback Further information is requested label Apr 10, 2019
@bastelfreak
Copy link
Member

Thanks @bodgit !

@bastelfreak bastelfreak merged commit 5fc5567 into voxpupuli:master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python3_version fact doesn't work on SCL Unable to use SCL version
2 participants