ensure not detecting a pip version

[maxadamo]

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]

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

[smortex]

This regexp is barely readable :unamused:. 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]

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 :smiley:

[smortex]

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