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

Fix version-check. #489

Merged
merged 3 commits into from
May 29, 2019
Merged

Fix version-check. #489

merged 3 commits into from
May 29, 2019

Conversation

pillarsdotnet
Copy link
Contributor

Fixes #303
Fixes #354
Fixes #420
Fixes #429
Fixes #444
Fixes #476

Allows Python3 installation on Centos-7 thusly:

class { 'python':
  version: 'python36'
}

Copy link
Member

@rnelson0 rnelson0 left a comment

Choose a reason for hiding this comment

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

The code looks functional, a few comments on layout/syntax. This would benefit from an additional test or two leveraging the new version string allowed, though.

manifests/init.pp Outdated Show resolved Hide resolved
manifests/install.pp Outdated Show resolved Hide resolved
manifests/install.pp Outdated Show resolved Hide resolved
@pillarsdotnet
Copy link
Contributor Author

The code looks functional, a few comments on layout/syntax. This would benefit from an additional test or two leveraging the new version string allowed, though.

Made requested changes; added tests; waiting for them to pass.

@pillarsdotnet
Copy link
Contributor Author

@rnelson0 -- Woo-hoo! Tests passed.

Copy link
Member

@rnelson0 rnelson0 left a comment

Choose a reason for hiding this comment

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

As mentioned in Slack, so many ::s :) But it works just fine! Looks like tests are passing so it should be good to go. Can you just add a note about whether you’ve tried using this locally, in a more complex environment, before merging?

manifests/install.pp Outdated Show resolved Hide resolved
@pillarsdotnet
Copy link
Contributor Author

Removed :: prefixes as requested.

Yes, I'm using this locally to install requisites for pre-commit.

@alexjfisher alexjfisher merged commit 617cf5d into voxpupuli:master May 29, 2019
@bastelfreak bastelfreak added the bug Something isn't working label May 29, 2019
@jozsi
Copy link

jozsi commented Aug 30, 2019

This is actually breaking. If I specify version 3.7 or python3.7 (because the Ubuntu package is called python3.7), it matches the /\A(python)?([0-9]+)/ regular expression and the 2nd group matches only 3, so the .7 gets cut off in the package version.

@bastelfreak
Copy link
Member

@jozsi thanks for pointing this out. Are you able to provide a PR with a test that matches this?

@jozsi
Copy link

jozsi commented Aug 31, 2019 via email

baurmatt added a commit to syseleven/puppet-python that referenced this pull request Dec 5, 2019
baurmatt added a commit to syseleven/puppet-python that referenced this pull request Jan 13, 2020
krissik pushed a commit to krissik/puppet-python that referenced this pull request Jul 30, 2020
russellshackleford pushed a commit to russellshackleford/puppet-python that referenced this pull request Sep 28, 2020
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
5 participants