Skip to content

(PUP-10496) Fix parsing version range for regular version#8138

Merged
Dorin-Pleava merged 1 commit intopuppetlabs:masterfrom
Hexta:fix-parsing-regular-version
May 20, 2020
Merged

(PUP-10496) Fix parsing version range for regular version#8138
Dorin-Pleava merged 1 commit intopuppetlabs:masterfrom
Hexta:fix-parsing-regular-version

Conversation

@Hexta
Copy link
Copy Markdown

@Hexta Hexta commented May 6, 2020

Fix parsing version range for simple version string like '1.2.3'.

Puppet Version: 6.11.0
Puppet Server Version: 6.11.0
OS Name/Version: Debian 9

Steps:
puppet resource package prometheus-client ensure='0.7.1' provider='pip3' --debug

Actual Behavior:
Debug log contains strange string:

Debug: Cannot parse 0.7.1 as a pip version range, falling through.

@Hexta Hexta requested review from a team May 6, 2020 22:42
@puppetlabs-jenkins
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@Hexta Hexta force-pushed the fix-parsing-regular-version branch 6 times, most recently from 358822e to f15ae31 Compare May 6, 2020 23:36
@mihaibuzgau mihaibuzgau requested a review from a team May 7, 2020 05:48
@luchihoratiu luchihoratiu self-assigned this May 7, 2020
@luchihoratiu
Copy link
Copy Markdown
Contributor

Hi @Hexta! Thank you for using Puppet and showing interest in code contribution, but before reviewing your solution, we would really appreciate getting more information regarding the issue you've come across when using version ranges (such as the package provider used, operating system, package name or any other details that could help us reproduce the problem and manually check your fix against).

We are asking this because the case (as currently explained in your pull request's description) should already be covered by letting each package provider handle simple version string like '1.2.3' their own way. This fallback (from version ranges to default implementation) was intentional (for first revision of the feature at least) due to the complexity and refactoring needed for uniformizing all the package providers to have simple versions (or =version) also fully covered in the added version ranges code.

Cheers!

@Hexta Hexta force-pushed the fix-parsing-regular-version branch from f15ae31 to 6f3a584 Compare May 7, 2020 15:11
@Hexta
Copy link
Copy Markdown
Author

Hexta commented May 7, 2020

@luchihoratiu Hello Luchi!
I've added comments from the JIRA ticket: https://tickets.puppetlabs.com/browse/PUP-10496

Copy link
Copy Markdown
Contributor

@luchihoratiu luchihoratiu left a comment

Choose a reason for hiding this comment

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

Besides some very small observations, this looks really good. I initially found the changes a bit inconsistent across providers but this is actually due to the current implementation and specifics each have (also double checked and indeed this wouldn’t make sense for the gem provider). They might get a bit more standardised at some point in a future refactoring.

I’ve manually tested the changes for each provider (pip and pip3 on macOS 10.14, apt on Ubuntu 20.04, yum on RedHat 8 and zypper on SLES 15) and they work as expected with previous functionality being unaffected. Thank you again @Hexta for your contribution. Great job!

Comment thread lib/puppet/util/package/version/range.rb Outdated
Comment thread spec/unit/provider/package/yum_spec.rb
Comment thread spec/unit/util/package/version/range_spec.rb
@luchihoratiu luchihoratiu requested a review from a team May 14, 2020 08:53
@Hexta Hexta force-pushed the fix-parsing-regular-version branch from 6f3a584 to b3723af Compare May 14, 2020 14:32
Fix parsing version range for simple version string like '1.2.3'.
@Hexta Hexta force-pushed the fix-parsing-regular-version branch from b3723af to 8a47b42 Compare May 14, 2020 14:47
@Hexta
Copy link
Copy Markdown
Author

Hexta commented May 14, 2020

@luchihoratiu Thank you for you comments!
I have fixed all issues.

Copy link
Copy Markdown
Contributor

@Dorin-Pleava Dorin-Pleava left a comment

Choose a reason for hiding this comment

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

LGTM, and works as expected on Fedora.
One small thing before merging would be to add the JIRA ticket number in the PR title and in the PR commit:
(PUP-10496) Fix parsing version range for regular version

@Hexta Hexta changed the title Fix parsing version range for regular version (PUP-10496) Fix parsing version range for regular version May 20, 2020
@Hexta
Copy link
Copy Markdown
Author

Hexta commented May 20, 2020

@Dorin-Pleava I've changed PR title.
The commit already has the ticket number in the title.

@Dorin-Pleava Dorin-Pleava merged commit 04d9a97 into puppetlabs:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants