puppetlabs / ruby-pwsh

A ruby gem for interacting with PowerShell
MIT License
15 stars 21 forks source link

ConvertTo-CanonicalResult incorreclty forces empty_string to null #264

Closed Clebam closed 5 months ago

Clebam commented 8 months ago

Describe the Bug

When comparing a resource from manifest that is an empty string to the value set in the system, we have a mismatch between empty_string and nil.

Expected Behavior

When the value in the system is an empty_string and the value in the manifest is an empty_string, they should match and not reapply

Steps to Reproduce

Steps to reproduce the behavior:

  1. Use the webadministration module (https://forge.puppet.com/modules/dsc/webadministrationdsc/readme)
  2. Have both pwshlib and the module
  3. Have IIS installed and create a webappool with a manifest like this
    
    class sw_iis {
    dsc_webapppool { 'DefaultAppPool':
    dsc_ensure                => 'Present',
    dsc_name                  => 'DefaultAppPool',
    dsc_managedruntimeversion => '',
    dsc_state                 => Stopped,
    }
    }

Note the value with an empty string dsc_managedruntimeversion => '',

Value is base on the enum accepted by the resource (../webadministrationdsc/lib/puppet/type/dsc_webapppool.rb)

dsc_managedruntimeversion: { type: "Optional[Enum['v4.0', 'v2.0', '']]", desc: "Indicates the CLR version to be used by the application pool. The values that are allowed for this property are: v4.0, v2.0, and ''.",

  mandatory_for_get: false,
  mandatory_for_set: false,
  mof_type: 'String',
  mof_is_embedded: false,
},
4. When the resource is applied we have this at each run

Notice: /Stage[main]/Sw_iis/Dsc_webapppool[DefaultAppPool]/dsc_managedruntimeversion: dsc_managedruntimeversion changed to '' (corrective) {"rebootrequired":false,"indesiredstate":true,"errormessage":""} Notice: dsc_webapppool[{:name=>"DefaultAppPool", :dsc_name=>"DefaultAppPool"}]: Updating: Finished in 0.950356 seconds


## Environment
 - Version puppet 7.23.0
 - Platform Windows Server 2019 Standard

## Additional Context
I pinpointed the problem to originate from here : https://github.com/puppetlabs/ruby-pwsh/blob/main/lib/puppet/provider/dsc_base_provider/invoke_dsc_resource_functions.ps1#L54

if ([string]::IsNullOrEmpty($value)) {

While PowerShell can happily treat empty strings as valid for returning

# an undefined enum, Puppet expects undefined values to be nil.
$Value = $null

}


This piece of code implies that whenever it reads an empty string, it converts it to a null value.

I made a tweak to be more specific on the conversion. If it is an empty_string that is a `[string]` (ie not a `[string[]]` or else), I return the empty string.

if ([string]::IsNullOrEmpty($value)) {

While PowerShell can happily treat empty strings as valid for returning

# an undefined enum, Puppet expects undefined values to be nil.
if ($Value -is [string]) {
  $Value = ''
}
else {
  $Value = $null
}

}


I'm not absolutely sure what was the intended behavior. I saw that if I remove completely the conversion to null, I have some warning with `string[]` for instance. So I aimed only `[string]` type. I hope you have more insights on this.
brajjan commented 6 months ago

@jordanbreen28 I just validated the fix for this and the updated code works for me. @Clebam please open a PR for this!