puppetlabs / ruby-pwsh

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

Fix empty string nullification #292

Closed Clebam closed 6 months ago

Clebam commented 8 months ago

Summary

Linked to https://github.com/puppetlabs/ruby-pwsh/issues/264

Related Issues (if any)

https://github.com/puppetlabs/ruby-pwsh/issues/264

Checklist

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.98%. Comparing base (900a944) to head (3c1b39e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #292 +/- ## ======================================= Coverage 91.98% 91.98% ======================================= Files 6 6 Lines 711 711 ======================================= Hits 654 654 Misses 57 57 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Clebam commented 7 months ago

Hi @jordanbreen28 , for this one I 'm not sure what is going on. I can see the specs are using dsc_xwebsite resources and not dsc_website (https://github.com/dsccommunity/WebAdministrationDsc) (though I don't think this is the main cause of the failure)

jordanbreen28 commented 7 months ago

hey @Clebam - yeah i don't think thats the issue as our nightly builds are passing. Something that has changed in this PR has caused it - Not to worry though! I'll be happy to collaborate, spin up an environment and take a look at this myself shortly. Appreciate all your work on these!

jordanbreen28 commented 7 months ago

@Clebam I've been looking into this some more, running tests and trying to get a working solution, but the more time I've spent on this I'm starting to think this maybe isn't the way to go 👎 I think a fix is more appropriate in the puppet-resource_api, and how it's compares the empty or nil enum values.

Clebam commented 7 months ago

I gave a look at resource-api, it's a bit overwhelming at first glance. Not sure if I can spot something useful.

jordanbreen28 commented 7 months ago

@Clebam agreed, it is definitely a biggy. No pressure. I can make note of this and bring it up with the team to get prioritised!

Clebam commented 7 months ago

@jordanbreen28 However I'm curious to know what you saw that is going wrong with my fix. Maybe there is more I can do to it 🤔 (and I'm also interested if I can still try to use my quick fix in my env without breaking something. From my point of view, it did not have any wrong effects, but given you found issues, I'm a bit worried to merge it into my production environment 😓 )

jordanbreen28 commented 7 months ago

@Clebam actually disregard my previous message! This should work. I'm going to spend some more time in prying into the code to see if I can spot what is exactly happening here.

Could you rebase this off main? Some changes have went in, wondering if they effect the behaviour in anyway.

Clebam commented 7 months ago

@jordanbreen28, I rebased from main. If you have any logs from the puppet run that fails, I could give a look.

jordanbreen28 commented 7 months ago

@jordanbreen28, I rebased from main. If you have any logs from the puppet run that fails, I could give a look.

@Clebam apologies, I was off last week. I'll get to looking at this and providing some logs as soon as I can. Whats interesting is I noticed this only fails on puppet 8 - with changes to strict variable settings from puppet 7 ~> 8, I can bet thats the issue.

jordanbreen28 commented 7 months ago

@Clebam so the issue here is that we are converting nil enums to empty strings even when that is not a valid enum value. So I would recommend not running this in any of your environments as this will cause unexpected behaviour! I'm sure there is a way we can convert only valid values..

Error: /Stage[main]/Main/Dsc_xwebsite[DefaultSite]: Could not evaluate: Provider returned data that does not match the Type Schema for `dsc_xwebsite[DefaultSite]`
 Value type mismatch:
    * dsc_bindinginfo: [{"bindinginformation"=>"*:80:", "certificatestorename"=>"", "certificatesubject"=>nil, "certificatethumbprint"=>"", "hostname"=>"", "ipaddress"=>"*", "port"=>80, "protocol"=>"http", "s
slflags"=>"0"}] (index 0 entry 'certificatestorename' expects an undef value or a match for Enum['My', 'WebHosting'], got '')

Debug: dsc_xwindowsfeature: Collecting data from the DSC Resource
Debug: dsc_xwindowsfeature: Canonicalized Resources: [{:dsc_displayname=>"ASP.NET 4.7", :dsc_ensure=>"Present", :dsc_includeallsubfeature=>false, :dsc_logpath=>nil, :dsc_name=>"Web-Asp-Net45", :name=>"AspNet4
5"}]
Debug: Current State: {:dsc_displayname=>"ASP.NET 4.7", :dsc_ensure=>"Present", :dsc_includeallsubfeature=>false, :dsc_logpath=>nil, :dsc_name=>"Web-Asp-Net45", :name=>"AspNet45"}
Debug: dsc_xwebsite: Collecting data from the DSC Resource
Error: /Stage[main]/Main/Dsc_xwebsite[NewWebsite]: Could not evaluate: Provider returned data that does not match the Type Schema for `dsc_xwebsite[NewWebsite]`
 Value type mismatch:
    * dsc_bindinginfo: [{"bindinginformation"=>"*:80:", "certificatestorename"=>"", "certificatesubject"=>nil, "certificatethumbprint"=>"", "hostname"=>"", "ipaddress"=>"*", "port"=>80, "protocol"=>"http", "s
slflags"=>"0"}] (index 0 entry 'certificatestorename' expects an undef value or a match for Enum['My', 'WebHosting'], got '')

The only way we can access the resource type properties is within the dsc_base_provider.rb, there is a similiar method implemented here which could be tweaked with some regex to fit the use case here.

Clebam commented 7 months ago

I see. Not at work today but will give a shot tomorrow. At first glance the enum_values function I added might be useful here to know if '' (empty string) is among the valid values of the enum. Though I have to be careful to not override undefined values.

Enum['something', 'else', ''] myParam

myParam = '' ==> should be empty string
myParam (undefined) ==> should be nil

Will keep you updated tomorrow

Clebam commented 7 months ago

@jordanbreen28 Seems to be working 😄 Please check if I did not alter the behavior you intended first. (I focused only on values that are supplied by in name_hash in order to avoid setting '' value instead of leaving the value unmanaged (for instance if for some reason it was set manually or another way)

jordanbreen28 commented 7 months ago

@Clebam amazing! Yes, like you said, let me spin up an environment and to just reassure this doesn't break the existing logic :) Looking good though!