DSR_iSCSIInitiator: Feature/someone24 simplify code layout in initiator and fix Issue #18

[MarkPerry24]

Pull Request (PR) description

This Pull Request (PR) fixes the following issues:

Task list:

• Change details added to Unreleased section of CHANGELOG.md?
• Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
• Examples appropriately updated?
• New/changed code adheres to Style Guidelines?
• Unit and (optional) Integration tests created/updated where possible?

Apologies if pull is too large and goes against agile. I admit I got a little carried away. I split out the functions from the module file as I dislike monolithic module files and make harder to maintain or read and debug. Replacing Get-WMIObject with Get-CimInstance works in this module as it's not invoking any methods. This should fix the Nano server issue but I've not verified it on Nano.

---

This change is 

[codecov]

Codecov Report

Merging #32 into dev will increase coverage by 3%.
The diff coverage is 100%.

@@        Coverage Diff         @@
##           dev   #32    +/-   ##
==================================
+ Coverage   90%   93%    +3%     
==================================
  Files        3     3            
  Lines      655   291   -364     
==================================
- Hits       590   271   -319     
+ Misses      65    20    -45

[PlagueHO]

Thanks for submitting this @Someone24github ! It is much appreciated. I will take a proper look over this as soon as I can. But there are a bunch of failing tests occurring which I think are affecting the codecoverage results. I also suspect the file split may cause the DSCresources.tests (the shared tests we implement in the dsc resource kit) to fail. I've adopted the patterns and guidelines from the dsc resource kit including the HQRM guidelines, so I'm not sure the split is supported in there.

[johlju]

When dot sourcing in .ps1 files into the .psm1 file those .ps1 files are not covered by Pester, as the code coverage is only checked on .psm1 files, and Pester does not execute the module files so that the code is dot sourced before it add “check marks”, so the only code that is covered is the code that is dot sourcing.

Curious, when is DSR_iSCSIInitiator.psd1 used/read? Is it used by Import-DSCResource instead of/after the iSCSIDsc.psd1 at some point? I have never seen that being done for a resource.

[PlagueHO]

@johlju - I'm not sure I've seen that pattern either. It may have some other side effects, as we generally don't expect to see separate manifests for each resource in a resource module.

@Someone24github - I tend to split modules into dot sourced ps1 files as well in my other projects (but not DSC), so I appreciate the approach. However, I recently found out there are other problems with this approach (suggest reading through this PR over at PowerShellGet ).

A suggested better approach to this is to actually maintain the functions in separate files in the repo and merge them into a single PSM1 file during the CI. However, this functionality hasn't been implemented this in the DSCResource.Tests CI process. So that would mean implementing a change over there to support this.

[MarkPerry24]

@PlagueHO Hi, I ran the pester tests locally before creating a pull request and didn't see the failures above. If it's not a pattern you're comfortable with I'm happy to simply abandon and create a new pull request with only the Cim-Instance changes that provides a fix for Issue #18

I've only written a couple of DSc modules so may not be fully aware of the nuances and pitfalls that DSc modules introduce into the mix. I will review that PR to see what pitfalls are being seen there as you suggest.

@johlju In this instance i'm not sure why the Pester fails though I have used the dot source approach in a psm1 file and with Pester without issue so I don't believe the approach breaks Pester inherently. It may be unusual to use a manifest and is optional but I have found it to be an effective way of adding control to your modules. You do not need to call export-modulemember as the exported functions are handled by the manifest. When using Import-Module the manifest is automatically found and evaluated there is no need to do anything special. It is mentioned in the following documentation, https://docs.microsoft.com/en-us/powershell/dsc/authoringresourcemof

[johlju]

@Someone24github My mistake, Pester seems to handle the dot-sourced .psm1 files correctly with code coverage. Failing tests seems like a spelling error - Get-CimInstnace should be Get-CimInstance (in the Mock maybe)?

The only reason I see to add a module manifest would be if the module have functions (other than the Get-, Set and Test-TargetResource) that should be exported. There is no need to export the Get-, Set and Test-TargetResource function using either a manifest or Export-ModuleMember. Import-DscResource and Get-DcResource works without Export-ModuleMember or a manifest (both are optional). As far as I know, there should be no need to use Import-Module on DSC resource modules unless they have other (helper) functions that should be exported too? Is that the intended behavior to export other functions/cmdlets?
The reason I got curious about the module manifest is that maybe that could be used when a resource module has both mof-based resources and several class-based resources.

[MarkPerry24]

@johlju You were on the money with the Typo. I have fixed that. That'll teach me for using find and replace....
According to the Microsoft article I linked on DSC you do need to export the functions, Get, Test, Set-TargetResource. Also the Module was already calling Export-ModuleMember with *-TargetResource. This leads me to believe that the module does require importing in addition to Import-DScresource. According to https://blogs.msdn.microsoft.com/powershell/2014/04/25/understanding-import-dscresource-keyword-in-desired-state-configuration/ Import-DScResource isn't a function but simply a keyword only valid inside a configuration block. DSc finds the resource by enumerating though all valid psmodule paths in order of priority then if finds first name that matches. Unless you specify a specific module. If a module lives in any of the default PowerShell module paths or in a path included in $PSModulePath it'll be implicitly imported. I would have thought if the TargetResource functions were not exported they would only ever be valid within the module itself which would mean no external resource would be able to access them as they would be out of scope. I must confess that the DSc class based resources do look cleaner to Mof based on the face of it. I was hoping to look at extending this to actually be able to configure multipath feature correctly in another bug I created. Currently you can set multipath enabled but it won't currently work properly as there is nothing to ensure MPIO is installed, configured and is claiming iscsi devices. Is it possible or preferable in that instance to create and MPIO resource and make iSCSIDsc depend on it if ISMultiPathEnabled = $true is set... Thoughts for another day perhaps.

@PlagueHO , @johlju The failing tests currently appear to be something on the appveyor side I have no control over which means that the test would currently always fail. It is explicitly looking for Test,Set.Get in the psm1 file. I'm not sure if this is configurable at all? However if not, it is possible to simply have Get, Test, Set placeholder functions in the psm1 that do nothing other than call the actual functions with same params and pass $PSBoundParameters as a splat...

[MarkPerry24]

@PlagueHO Hi, just finished reviewing that PR you linked me to. It's quite interesting. I'd be surprised if this module right now suffered the same performance issue as that one as the Module is immense. It takes near 3 seconds to load on a good day. I do fundamentally agree that a "compiled" psm1 out of a release pipeline might be beneficial though arguably you'd probably only notice when you manually import the module. It would be fairly straight forward to do somehting a bit neater but ultimately the similar to gci *.ps1 | get-content | add-content module.psm1 in a release pipe.

[johlju]

The errors you are seeing is from the PS Script Analyzer, and that is a rule we are not suppose to override (to follow the guidelines from DSC Resource Kit). See rules https://github.com/PowerShell/DscResources/blob/master/PSSARuleSeverities.md.
Personally I would suggest to just put back the *-TargetResource in the module file, to pass all tests and follow the pattern for all other resource modules in the DSC Resource Kit. 🙂

[MarkPerry24]

@johlju @PlagueHO Perhaps it's best if I simply do enough to resolve #18 and move on. Splitting the files out for dev purposes seemed logical at the time but it appears requires more preparation. Thanks

[johlju]

@Someone24github Yes, I can see the benefit for development purpose having the functions split, but unfortunately, as you say, this is a pattern that need more work if we should use it. I suggest to revert the file and just resolve issue #18.

[PlagueHO]

Sorry @johlju and @Someone24github it's taken me so long to get back (super busy week at the day job). But yes, it is probably easier to keep the files together for now until we've got some mechanism to handle this in the shared test automation in DSCResource.Tests.

[PlagueHO]

Hi @MarkPerry24 - were you still keen to complete this one? Otherwise I can look at incorporating just the issue #18 fix in

[MarkPerry24]

Hi @PlagueHO,

Just fixing issue #18 is fine. I changed companies and iSCSI doesn't feature in my world since became cloud engineer in azure/aws and focussed on CI/CD pipelines and infrastructure as code. Thanks for the mention though.

[PlagueHO]

No problem @MarkPerry24 (I completely understand) - and will do!