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

ensure not detecting a pip version #617

Closed
maxadamo opened this issue Jul 31, 2021 · 4 comments · Fixed by #703
Closed

ensure not detecting a pip version #617

maxadamo opened this issue Jul 31, 2021 · 4 comments · Fixed by #703

Comments

@maxadamo
Copy link
Contributor

maxadamo commented Jul 31, 2021

Affected Puppet, Ruby, OS and module versions/distributions

this is happening with the latest version of the module: 6.1.0

I am wondering if this is really the intended, and if we need to have a parameter to disable strict version checking:

if $_ensure =~ /^((19|20)[0-9][0-9]-(0[1-9]|1[1-2])-([0-2][1-9]|3[0-1])|[0-9]+\.\w+\+?\w*(\.\w+)*)$/ {

for instance, I do have different software versions in our environments, as follows:

indico_version: '2.1.11+20210406.1454.2b0d0b9'    # KO
indico_version: '2.3.5+20210720.1912.e31eb12'     # KO
indico_version: '2.0+202106300809.4eec583ae7'     # OK

and only the latter is detected. It would accept 2.1.11, but not 2.1.11+xxxxx.

But rather than trying to fix this convoluted regex, would it make sense to have a parameter called strict_version_checking that can be set to true/false?

@maxadamo
Copy link
Contributor Author

if this idea makes sense to you, I can work on a PR.

@smortex
Copy link
Member

smortex commented Jul 31, 2021

This regexp is barely readable 😒. PRs are always welcome, however, workaround for a problematic case should be avoided (e.g. setting strict_version_checking=false because otherwise compilation fail).

In any case, the regexp looks wrong: ((19|20)[0-9][0-9]-(0[1-9]|1[1-2])-([0-2][1-9]|3[0-1])) seems to be matching a date… unless it's in october or the 10th or the 20th of the month… and as far as I can see, this is not the problem you see. But maybe it is not supposed to match a date, but it have not been modified for a long time.

Maybe we can find a way which which avoid completely this regexp, a quick look at the source makes me think we want to distinguish things like present or absent from an explicit version, so maybe we can replace this piece of code with a selector?

$grep_regex = $_ensure ? {
  'present'  => '...',
  'absent'   => '...',
  'whatever' => '...',
  default    => '...',
}

Opening a PR that add a unit test that demonstrate the problem is a very good first step!

@maxadamo
Copy link
Contributor Author

I'm trying to get the story because I'm not clear yet.
If a version number is wrong and it can't be installed, the package manager will error.
If the package manager accepts that version, why do we need to bother?
And why do we need to implement the same logic which is already present in the package manager (I assume this module wont' do better than PIP).
I'd make it really simpler, and let the package manager decide whether to throw an error or not.
You could send a warning if a regex is not matched, but do we really need to chase this regex for the purpose of warning the user?
What do you think? If in your opinion, it's a non-sense you can just say it 😃

@smortex
Copy link
Member

smortex commented Jul 31, 2021

I trend to agree: validating version numbers is a PITA and unless this is utterly needed, should be avoided IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants