BREAKING CHANGE: OfficeOnlineServerFarm: Added 'IsSingleInstance' parameter #65
Open
JonasRied wants to merge 7 commits into
Open
BREAKING CHANGE: OfficeOnlineServerFarm: Added 'IsSingleInstance' parameter #65JonasRied wants to merge 7 commits into
JonasRied wants to merge 7 commits into
Conversation
added 6 commits
November 6, 2023 12:40
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
====================================
Coverage 80% 80%
====================================
Files 6 6
Lines 759 766 +7
====================================
+ Hits 613 620 +7
Misses 146 146
|
WalkthroughThe pull request introduces a new mandatory Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/DSCResources/MSFT_OfficeOnlineServerFarm/MSFT_OfficeOnlineServerFarm.psm1 (1)
810-821:⚠️ Potential issue | 🟠 MajorPotential null reference when URL parameters are not provided.
The URL normalization code calls
.EndsWith('/')on$InternalURL,$ExternalURL, and$Proxywithout checking if they are null or empty. SinceInternalURLis now optional, this will throw aMethodInvocationExceptionwhen onlyExternalURLis provided (and vice versa).🐛 Proposed fix
- if ($InternalURL.EndsWith('/') -eq $false) + if (-not [string]::IsNullOrEmpty($InternalURL) -and $InternalURL.EndsWith('/') -eq $false) { $InternalURL += "/" } - if ($ExternalURL.EndsWith('/') -eq $false) + if (-not [string]::IsNullOrEmpty($ExternalURL) -and $ExternalURL.EndsWith('/') -eq $false) { $ExternalURL += "/" } - if ($Proxy.EndsWith('/') -eq $false) + if (-not [string]::IsNullOrEmpty($Proxy) -and $Proxy.EndsWith('/') -eq $false) { $Proxy += "/" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DSCResources/MSFT_OfficeOnlineServerFarm/MSFT_OfficeOnlineServerFarm.psm1` around lines 810 - 821, The URL normalization block calls .EndsWith('/') on potentially null/empty variables ($InternalURL, $ExternalURL, $Proxy) which can throw MethodInvocationException; update the logic in MSFT_OfficeOnlineServerFarm.psm1 to guard each variable with a null/empty check (e.g. use -and and [string]::IsNullOrEmpty or an if (-not [string]::IsNullOrEmpty($InternalURL)) before calling .EndsWith('/')) and only append "/" when the variable is non-empty and does not already end with "/", applying the same guarded checks for $ExternalURL and $Proxy.
🧹 Nitpick comments (5)
tests/Integration/MSFT_OfficeOnlineServerWebAppsFarm.Integration.tests.ps1 (1)
15-18: Parameter name casing inconsistency.The parameter
-DscResourceNameuses different casing than other test files in the repository. Other unit tests (e.g.,MSFT_OfficeOnlineServerInstall.tests.ps1) use-DSCResourceNamefor consistency.📝 Suggested fix for consistency
$TestEnvironment = Initialize-TestEnvironment ` -DSCModuleName $Script:DSCModuleName ` - -DscResourceName $Script:DSCResourceName ` + -DSCResourceName $Script:DSCResourceName ` -TestType Integration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/MSFT_OfficeOnlineServerWebAppsFarm.Integration.tests.ps1` around lines 15 - 18, The Initialize-TestEnvironment invocation uses inconsistent parameter casing (-DscResourceName); update the call to use the same casing as other tests by changing the parameter to -DSCResourceName so it matches convention across the test suite (look for the Initialize-TestEnvironment call and the -DscResourceName token to modify).src/Examples/Resources/OfficeOnlineServerFarm/1-NewFarm.ps1 (1)
48-53: Inconsistent casing for IsSingleInstance value.The value
'YES'differs from'Yes'used in other examples (e.g.,src/Examples/FullExamples/NewFarm.ps1). While PowerShell'sValidateSetis case-insensitive, using consistent casing improves readability and aligns with the schema definition.📝 Suggested fix
OfficeOnlineServerFarm LocalFarm { - IsSingleInstance = 'YES' + IsSingleInstance = 'Yes' InternalURL = "https://officeonline.contoso.com" EditingEnabled = $true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Examples/Resources/OfficeOnlineServerFarm/1-NewFarm.ps1` around lines 48 - 53, The IsSingleInstance value in the OfficeOnlineServerFarm LocalFarm block uses inconsistent casing ('YES'); update the IsSingleInstance property in the OfficeOnlineServerFarm LocalFarm declaration to use 'Yes' to match other examples (e.g., NewFarm.ps1) and the schema's preferred capitalization for readability and consistency.tests/Unit/OfficeOnlineserverDSC/MSFT_OfficeOnlineServerFarm.tests.ps1 (2)
21-25: Parameter name casing inconsistency.Same issue as the integration test -
-DscResourceNamediffers from-DSCResourceNameused in other test files.📝 Suggested fix
$script:testEnvironment = Initialize-TestEnvironment ` -DSCModuleName $script:dscModuleName ` - -DscResourceName $script:dscResourceName ` + -DSCResourceName $script:dscResourceName ` -ResourceType 'Mof' ` -TestType 'Unit'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/OfficeOnlineserverDSC/MSFT_OfficeOnlineServerFarm.tests.ps1` around lines 21 - 25, The Initialize-TestEnvironment call uses a lowercase-cased parameter name "-DscResourceName" which is inconsistent with other tests that use "-DSCResourceName"; update the call so the parameter casing matches the canonical form (use "-DSCResourceName") and ensure the referenced variable ($script:dscResourceName) remains the same, i.e., change only the parameter token in the Initialize-TestEnvironment invocation to "-DSCResourceName" to restore consistency across tests.
205-208: Replace deprecatedAssert-MockCalledwithShould -Invoke.Per test guidelines,
Assert-MockCalledis deprecated. This also applies to similar usages on lines 257 and 282.📝 Suggested fix
It "Creates the farm in the set method" { Set-TargetResource `@testParams` - Assert-MockCalled New-OfficeWebAppsFarm + Should -Invoke -CommandName New-OfficeWebAppsFarm -Exactly -Times 1 -Scope It }As per coding guidelines: "Never use
Assert-MockCalled, useShould -Invokeinstead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/OfficeOnlineserverDSC/MSFT_OfficeOnlineServerFarm.tests.ps1` around lines 205 - 208, The test uses the deprecated Assert-MockCalled for verifying New-OfficeWebAppsFarm after calling Set-TargetResource; replace the Assert-MockCalled New-OfficeWebAppsFarm assertion with the modern invocation assertion pattern using Should -Invoke (e.g. wrap the mocked command invocation in a scriptblock and assert it was invoked: { New-OfficeWebAppsFarm } | Should -Invoke), and make the same replacement for the other occurrences that assert New-OfficeWebAppsFarm in this spec associated with Set-TargetResource.CHANGELOG.md (1)
10-11: Consider adding issue reference to the changelog entry.Per changelog guidelines, notable changes should reference issues. Since this PR fixes
#42, consider updating the entry:📝 Suggested improvement
- OfficeOnlineServerFarm - - Added parameter 'IsSingleInstance' + - Added parameter 'IsSingleInstance' - Fixes [issue `#42`](https://github.com/dsccommunity/OfficeOnlineServerDsc/issues/42)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 10 - 11, Update the CHANGELOG entry for OfficeOnlineServerFarm to reference the issue fixed by this PR by appending the issue number to the entry for the added parameter 'IsSingleInstance' (for example add "Fixes `#42`" or "(`#42`)") so the changelog follows the guidelines and links the notable change to issue `#42`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/DSCResources/MSFT_OfficeOnlineServerFarm/MSFT_OfficeOnlineServerFarm.psm1`:
- Around line 810-821: The URL normalization block calls .EndsWith('/') on
potentially null/empty variables ($InternalURL, $ExternalURL, $Proxy) which can
throw MethodInvocationException; update the logic in
MSFT_OfficeOnlineServerFarm.psm1 to guard each variable with a null/empty check
(e.g. use -and and [string]::IsNullOrEmpty or an if (-not
[string]::IsNullOrEmpty($InternalURL)) before calling .EndsWith('/')) and only
append "/" when the variable is non-empty and does not already end with "/",
applying the same guarded checks for $ExternalURL and $Proxy.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 10-11: Update the CHANGELOG entry for OfficeOnlineServerFarm to
reference the issue fixed by this PR by appending the issue number to the entry
for the added parameter 'IsSingleInstance' (for example add "Fixes `#42`" or
"(`#42`)") so the changelog follows the guidelines and links the notable change to
issue `#42`.
In `@src/Examples/Resources/OfficeOnlineServerFarm/1-NewFarm.ps1`:
- Around line 48-53: The IsSingleInstance value in the OfficeOnlineServerFarm
LocalFarm block uses inconsistent casing ('YES'); update the IsSingleInstance
property in the OfficeOnlineServerFarm LocalFarm declaration to use 'Yes' to
match other examples (e.g., NewFarm.ps1) and the schema's preferred
capitalization for readability and consistency.
In `@tests/Integration/MSFT_OfficeOnlineServerWebAppsFarm.Integration.tests.ps1`:
- Around line 15-18: The Initialize-TestEnvironment invocation uses inconsistent
parameter casing (-DscResourceName); update the call to use the same casing as
other tests by changing the parameter to -DSCResourceName so it matches
convention across the test suite (look for the Initialize-TestEnvironment call
and the -DscResourceName token to modify).
In `@tests/Unit/OfficeOnlineserverDSC/MSFT_OfficeOnlineServerFarm.tests.ps1`:
- Around line 21-25: The Initialize-TestEnvironment call uses a lowercase-cased
parameter name "-DscResourceName" which is inconsistent with other tests that
use "-DSCResourceName"; update the call so the parameter casing matches the
canonical form (use "-DSCResourceName") and ensure the referenced variable
($script:dscResourceName) remains the same, i.e., change only the parameter
token in the Initialize-TestEnvironment invocation to "-DSCResourceName" to
restore consistency across tests.
- Around line 205-208: The test uses the deprecated Assert-MockCalled for
verifying New-OfficeWebAppsFarm after calling Set-TargetResource; replace the
Assert-MockCalled New-OfficeWebAppsFarm assertion with the modern invocation
assertion pattern using Should -Invoke (e.g. wrap the mocked command invocation
in a scriptblock and assert it was invoked: { New-OfficeWebAppsFarm } | Should
-Invoke), and make the same replacement for the other occurrences that assert
New-OfficeWebAppsFarm in this spec associated with Set-TargetResource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80b5bc84-aa75-4d76-bba9-ab894b1bcd34
📒 Files selected for processing (8)
CHANGELOG.mdsrc/DSCResources/MSFT_OfficeOnlineServerFarm/MSFT_OfficeOnlineServerFarm.psm1src/DSCResources/MSFT_OfficeOnlineServerFarm/MSFT_OfficeOnlineServerFarm.schema.mofsrc/Examples/FullExamples/NewFarm.ps1src/Examples/Resources/OfficeOnlineServerFarm/1-NewFarm.ps1tests/Integration/MSFT_OfficeOnlineServerWebAppsFarm.Integration.tests.ps1tests/Integration/MSFT_OfficeOnlineServerWebAppsFarm.config.ps1tests/Unit/OfficeOnlineserverDSC/MSFT_OfficeOnlineServerFarm.tests.ps1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
Adds the parameter 'IsSingleInstance' as suggested in #42.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is