OfficeOnlineServerHost: New resource proposal #66

JonasCassier · 2023-11-09T16:19:34Z

Pull Request (PR) description

Adds new resource OfficeOnlineServerHost
Replacement of #63

This Pull Request (PR) fixes the following issues

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

codecov · 2023-11-09T16:24:45Z

Codecov Report

Merging #66 (60df501) into master (7358992) will increase coverage by 0%.
Report is 2 commits behind head on master.
The diff coverage is 92%.

Additional details and impacted files

@@         Coverage Diff          @@
##           master   #66   +/-   ##
====================================
  Coverage      80%   81%           
====================================
  Files           6     7    +1     
  Lines         759   804   +45     
====================================
+ Hits          613   657   +44     
- Misses        146   147    +1

Files	Coverage Δ
...eServerInstall/MSFT_OfficeOnlineServerInstall.psm1	`84% <100%> (ø)`
...ck/MSFT_OfficeOnlineServerInstallLanguagePack.psm1	`88% <100%> (ø)`
...eServerMachine/MSFT_OfficeOnlineServerMachine.psm1	`98% <100%> (ø)`
...ctUpdate/MSFT_OfficeOnlineServerProductUpdate.psm1	`77% <100%> (ø)`
...eOnlineServerFarm/MSFT_OfficeOnlineServerFarm.psm1	`95% <98%> (ø)`
...eOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1	`94% <94%> (ø)`
...lineServerDsc.Util/OfficeOnlineServerDsc.Util.psm1	`57% <0%> (-2%)`	⬇️

johlju

This is what I had time for today. Barely started with the review, but now there are a few things for you to look over. 🙂

Reviewed 1 of 10 files at r1, all commit messages.
Reviewable status: 1 of 10 files reviewed, 8 unresolved discussions (waiting on @JonasCassier)

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 42 at r2 (raw file):

    Test-OfficeOnlineServerHostPSBoundParameters @PSBoundParameters

    $nullReturn = $PSBoundParameters

SUggest using @{} + $PSBoundParameters so when get a new hashtable, otherwise we just get the ref of a type System.Management.Automation.PSBoundParametersDictionary (which is not a hashtable which we should return) and also, any changes made to the variable (in a future PR) will also change the values of PSBoundParameters since we get a ref to the type PSBoundParametersDictionary.

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 43 at r2 (raw file):

    $nullReturn = $PSBoundParameters
    $nullReturn.Domains = @()

Should this also be set to the type [System.Array] so we consequent (it is set to the type below in another code path).

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 45 at r2 (raw file):

    $nullReturn.Domains = @()

    try

The try-catch should only be around the command that we catch, so that we can catch errors and throw for things that should be terminating errors (those can be set in a future PR. But here I would rather not see we use try-catch to switch code path. I rather see that the catch-statement sets a boolean value that we evaluate to determine if we could access the needed property, then return a hashtable accordingly.

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 47 at r2 (raw file):

    try
    {
        return @{

We should always return all properties in the hashtable (as null or the value passed if they cannot be set to a current configured value).

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 49 at r2 (raw file):

        return @{
            IsSingleInstance = 'Yes'
            Domains          = [Array](Get-OfficeWebAppsHost -ErrorAction 'Stop').AllowList

We shuould use the full type name [System.Array].

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 70 at r2 (raw file):

        [Parameter()]
        [System.String[]]
        $Domains,

Thw parameter Domains should be mutually exclusive to DomainsToInclude and DomainsToExclude. All three should not be allowed to be used at once., Only DomainsToInclude and DomainsToExclude should be bale to be used at the same time.

There is a command Assert-BoundParameter in the module DscResource.Common that can help with this. Though I see this module does not use the helper module (yet). There is a blog post on how to add the helper module to a DSC module, though this might break other modules that need to be fixed, so I think that should be done in a separate PR. So another option is to copy the code from DscResource.Common and put it in # OfficeOnlineServerDsc.Util. 🤔 You decide 😃

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 81 at r2 (raw file):

    )

    Write-Verbose -Message "Updating Office Online Server Farm host allow list"

Can we move the strings to the localization files? Throughout the code. Then use them similar to here:

OfficeOnlineServerDsc/src/DSCResources/MSFT_OfficeOnlineServerFarm/MSFT_OfficeOnlineServerFarm.psm1

Line 522 in 7358992

Write-Verbose -Message $script:localizedData.InstallingNewFarm

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 96 at r2 (raw file):

        if ($null -eq $Domains)
        {
            $PSBoundParameters.Add('DomainsToExclude', $CurrentValues.Domains) | Out-Null

See previous comment about $PSBoundParameters. We should not add properties to the $PSBoundParameters because then we cannot determine if they were actually passed by the user/configuration. Suggest using a new hashtable here to control the flow.

JonasCassier

Reviewable status: 1 of 10 files reviewed, 8 unresolved discussions (waiting on @johlju)

src/DSCResources/MSFT_OfficeOnlineServerHost/MSFT_OfficeOnlineServerHost.psm1 line 43 at r2 (raw file):