(FM 8425) - Replace Library Code #134
Conversation
Codecov Report
@@ Coverage Diff @@
## master #134 +/- ##
===========================================
- Coverage 73.17% 35.71% -37.46%
===========================================
Files 9 7 -2
Lines 492 434 -58
===========================================
- Hits 360 155 -205
- Misses 132 279 +147
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Love all the code removals. Two minor comments on improvements possible. Final call for @michaeltlombardi .
PS: also checked the coverage report and there is no significant change (better/worse) in the code touched, so 👍 from me.
.sync.yml
Outdated
| @@ -20,6 +20,9 @@ Gemfile: | |||
| - gem: beaker-testmode_switcher | |||
| version: "~> 0.4" | |||
| - gem: master_manipulator | |||
| optional: | |||
| ':development': | |||
| - gem: 'ruby-pwsh' | |||
There was a problem hiding this comment.
no project should need the gem AND the module dependency. As a module I'd prefer the module dependency (and fixture). If that leads to issues, we need to fix them further upstream.
There was a problem hiding this comment.
I think this is only for the development purposes, not for the shipped project, but I'm happy to double check whether this is necessary!
There was a problem hiding this comment.
Tested this tonight and the fixtures will help with pdk bundle exec puppet ... calls but not pdk test unit - it will blow up on the requires statement in the provider.
There was a problem hiding this comment.
that is weiiiird. seems like something in the rspec-puppet chain is not setting up load paths correctly.
| @@ -1,8 +1,7 @@ | |||
| require 'pathname' | |||
| require 'json' | |||
| if Puppet::Util::Platform.windows? | |||
| require_relative '../../../puppet_x/puppetlabs/dsc_lite/powershell_manager' | |||
| require_relative '../../../puppet_x/puppetlabs/dsc_lite/powershell_hash_formatter' | |||
| require 'ruby-pwsh' | |||
There was a problem hiding this comment.
I think, technically, we can include this without the guard, as loading the ruby-pwsh library won't blow up on non-Windows.
| @@ -76,7 +74,7 @@ def exists? | |||
| script_content = ps_script_content('test') | |||
| Puppet.debug "\n" + self.class.redact_content(script_content) | |||
|
|
|||
| if !PuppetX::DscLite::PowerShellManager.supported? | |||
| if !Pwsh::Manager.pwsh_supported? | |||
There was a problem hiding this comment.
Should be windows_powershell_supported?
| @@ -151,7 +149,7 @@ def self.format_dsc_value(dsc_value) | |||
| end | |||
|
|
|||
| def self.format_dsc_lite(dsc_value) | |||
| PuppetX::PuppetLabs::DscLite::PowerShellHashFormatter.format(dsc_value) | |||
| Pwsh::Util.format_powershell_value(dsc_value) | |||
There was a problem hiding this comment.
🤔 this one we may have to do a bit of heavy lifting on as the DSC Lite module had to have additional formatting management for sensitive values and CIM objects.
There was a problem hiding this comment.
Unlike practically all other powershell-dependent modules, invoking DSC requires that we handle some very special objects with particular types not matched in ruby, namely CIM instances.
We also need to write a specific handler for converting from a sensitive Puppet object to the appropriate PowerShell object representation.
There was a problem hiding this comment.
Ok, have returned powershell_hash_formatter.rb, cleaned out everything in it that's mirrored in the gem and added in a call to said gem.
So it will check if it's a Hash or a Puppet::Pops::Types::PSensitiveType::Sensitive and apply the appropriate logic if it is. And if it isn't it will pass it on though to the gems format method.
| format_array(dsc_value) | ||
| elsif dsc_value.class.name == 'Hash' | ||
| format_hash(dsc_value) | ||
| elsif dsc_value.class.name == 'Puppet::Pops::Types::PSensitiveType::Sensitive' |
There was a problem hiding this comment.
We may need to monkey patch this method to retain handling sensitive objects.
| '@(' + value.map { |m| format(m) }.join(', ') + ')' | ||
| end | ||
|
|
||
| private_class_method def self.format_hash(value) |
There was a problem hiding this comment.
Similarly, we may need to borrow from this and format_cim_instance for hashes which are special DSC object types, like credentials or other CIM instances.
This commit adds the pwshlib module / ruby-pwsh gem as a dependency, updating the development gems list in the Gemfile, the dependencies in the metadata, and the fixtures list. This will enable the module to depend on the updted library code from the ruby-pwsh gem and pwshlib.
david22swan
force-pushed
the
FM-8425
branch
3 times, most recently
from
cdbcca1
to
38b34d4
Compare
|
Is it just me or is the appveyor run not executing acceptance tests? https://ci.appveyor.com/project/puppetlabs/puppetlabs-dsc-lite/builds/29365114 😶 |
Prior to this commit the PowerShell module providers depended on a vendored version of the PowerShell code manager library in the PuppetX namespace. This commit updates the providers and their tests to leverage the ruby-pwsh gem from the pwshlib Puppet module instead. This reduces the amount of maintenance required and simplifies the code base.
Small improvements to the code
|
@michaeltlombardi PDKSync had stripped the acceptance testing out. Have added back in |
Specialist formatting code is needed for the module.
lionce
added
bugfix
backwards-incompatible
and removed
backwards-incompatible
bugfix
labels
No description provided.