-
Notifications
You must be signed in to change notification settings - Fork 23
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
allows patch levels (ie 9.5.14) #26
Conversation
@ojongerius Thank you for highlighting the issue and your first time contribution. 💯 You are right that we should support all patch versions. I propose the following regex instead |
Good catch! Should be addressed in 5947fa4. Let me know if you prefer me to rebase to one commit. |
Please check this too: inspec/inspec#3349 I believe that neither regex will match postgresql 10 output on ubuntu:
I think that using a look behind is necessary here. E.g.
|
Thank you for pointing this out @pmav99 I think it is right to fix the detection upstream in InSpec. Could we also use a simpler regex for the detection? |
@chris-rock I think that the regex you propose is not correct because it does match the text too. I don't have much experience with ruby but according to this ruby regexes do not support non-capturing groups. This is why I proposed a look behind. Although it seems that there is indeed a simpler solution: "psql (PostgreSQL) 9.6.10".split[2]
"psql (PostgreSQL) 10.5 (Ubuntu 10.5-1.pgdg16.04+1)".split[2] |
I'm not too familiar with Inspec, what does the Does it test if something matched at all? ie are we trying to make something like Generally simpler is better, both easier to read and maintain, how about |
The inspec matcher is just running the regular expression and checks if it matches. Maybe we should write a simple pg_version resource that parses the output and returns a version struct. This allows us to focus on version numbers only for the comparison. It also is easier to read and understand. |
@chris-rock ta. Would that resource do more than test if If not, I'd say let's go with |
@chris-rock ping! |
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.
Thank you @ojongerius for addressing that issue and for your contribution. This looks good to me. Lets see if we need to improve this further later.
This broke after #25 went in
Using this PR:
See http://rubular.com/r/2aiU6kIEG6