(PDK-840) Add PDK::Util::PuppetVersion.from_module_metadata #461
Conversation
lib/pdk/util/ruby_version.rb
Outdated
| end | ||
|
|
||
| def scan_for_packaged_rubies | ||
| { '2.4.3' => '2.4.0' } |
There was a problem hiding this comment.
The contents do not match the name of the function. What gives?
There was a problem hiding this comment.
Yeah, that was method was just moved around in the file, I was going to implement it in the next PR.
| @@ -24,9 +24,22 @@ | |||
| shared_context 'is a package install' do | |||
| before(:each) do | |||
| allow(PDK::Util).to receive(:package_install?).and_return(true) | |||
| allow(PDK::Util::RubyVersion).to receive(:versions).and_return('2.1.9' => '2.1.0', '2.4.3' => '2.4.0') | |||
There was a problem hiding this comment.
This does not match actual behaviour of this method
There was a problem hiding this comment.
It does now (that method was just a stub that was going to be implemented in the next PR).
| PDK::Util::RubyVersion.use('2.1.9') | ||
| instance219 = PDK::Util::RubyVersion.instance | ||
| PDK::Util::RubyVersion.use('2.4.3') | ||
| instance243 = PDK::Util::RubyVersion.instance |
There was a problem hiding this comment.
This feels like it could benefit from a .instance(version), instead of requiring two calls?
| def result(version) | ||
| { | ||
| gem_version: Gem::Version.new(version), | ||
| ruby_version: PDK::Util::RubyVersion.default_ruby_version, |
There was a problem hiding this comment.
Is this a test of the default_ruby_version, too, or are you using it here only as a shortcut?
There was a problem hiding this comment.
PDK::Util::RubyVersion.default_ruby_version will return the details of the running ruby version for non-packaged installs (gem or git clone), so I guess shortcut?
| @@ -59,6 +59,36 @@ def from_pe_version(version_str) | |||
| find_gem_for(gem_version[:gem_version]) | |||
| end | |||
|
|
|||
| def from_module_metadata(metadata) | |||
There was a problem hiding this comment.
shouldn't this parsing be located in the PDK::Module::Metadata class?
There was a problem hiding this comment.
I think it make sense to have it in util, because it validates the metadata object itself.
There was a problem hiding this comment.
It could go in either really, but I opted to contain it with all the other puppet version finding logic for clarity (imho). I can move it to be a method of PDK::Module::Metadata if that's the group consensus though.
There was a problem hiding this comment.
I've moved the validation logic into PDK::Module::Metadata and left the version finding logic in PDK::Util::PuppetVersion
For use when selecting the default Puppet & Ruby version if the user doesn't specify one