SPDistributedCacheService: Fix domain doubled when server name is already an FQDN#1480
SPDistributedCacheService: Fix domain doubled when server name is already an FQDN#1480luigilink wants to merge 1 commit into
Conversation
WalkthroughUpdated distributed cache server provisioning to preserve already-FQDN server names, added regression coverage, and documented the fix in the changelog. ChangesDistributed cache server naming
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Unit/SharePointDsc/SharePointDsc.SPDistributedCacheService.Tests.ps1 (2)
404-407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Should -Invokeinstead ofAssert-MockCalled.The path instructions for test files explicitly state: "Never use
Assert-MockCalled, useShould -Invokeinstead" and "UseShould -Invoke -Exactly -Times <n> -Scope Itfor call-count assertions". The new test should follow these rules even though the existing tests use the older pattern.As per coding guidelines: "Follow instructions over existing code patterns."
♻️ Proposed fix
It "Should not append the domain again if the server name is already an FQDN" { Set-TargetResource `@testParams` - Assert-MockCalled Get-SPServiceInstance -ParameterFilter { $Server -eq "Server1.contoso.com.contoso.com" } -Times 0 -Exactly + Should -Invoke Get-SPServiceInstance -ParameterFilter { $Server -eq "Server1.contoso.com.contoso.com" } -Exactly -Times 0 -Scope It }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/SharePointDsc/SharePointDsc.SPDistributedCacheService.Tests.ps1` around lines 404 - 407, Replace the Assert-MockCalled assertion in the FQDN test with Should -Invoke, targeting Get-SPServiceInstance and filtering for the duplicated-domain server value; specify -Exactly -Times 0 -Scope It to enforce the call-count assertion.Source: Path instructions
374-408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion and a non-FQDN regression test.
The current test only verifies the negative case (doubly-qualified name is never queried). Consider also asserting that
Get-SPServiceInstanceis called with the correct FQDN (Server1.contoso.com), and add a companion test for a non-FQDN server name to verify the domain is still appended when needed. This protects against regressions where the conditional is inverted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/SharePointDsc/SharePointDsc.SPDistributedCacheService.Tests.ps1` around lines 374 - 408, Extend the distributed-cache test context to positively assert that Get-SPServiceInstance is called exactly once with the existing FQDN Server1.contoso.com, while retaining the negative assertion for the doubly-qualified name. Add a companion context or test using a non-FQDN server name and verify Get-SPServiceInstance is queried with the domain-appended name, confirming the conditional behavior in Set-TargetResource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 24-28: Add the required issue reference for issue `#1468` to the
SPDistributedCacheService changelog entry, using the exact linked format [issue
`#1468`](https://github.com/<owner>/<repo>/issues/1468) with the repository’s
actual owner and name.
---
Nitpick comments:
In `@tests/Unit/SharePointDsc/SharePointDsc.SPDistributedCacheService.Tests.ps1`:
- Around line 404-407: Replace the Assert-MockCalled assertion in the FQDN test
with Should -Invoke, targeting Get-SPServiceInstance and filtering for the
duplicated-domain server value; specify -Exactly -Times 0 -Scope It to enforce
the call-count assertion.
- Around line 374-408: Extend the distributed-cache test context to positively
assert that Get-SPServiceInstance is called exactly once with the existing FQDN
Server1.contoso.com, while retaining the negative assertion for the
doubly-qualified name. Add a companion context or test using a non-FQDN server
name and verify Get-SPServiceInstance is queried with the domain-appended name,
confirming the conditional behavior in Set-TargetResource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a74dac1-b84d-497c-bc1e-8702b23f7058
📒 Files selected for processing (3)
CHANGELOG.mdSharePointDsc/DSCResources/MSFT_SPDistributedCacheService/MSFT_SPDistributedCacheService.psm1tests/Unit/SharePointDsc/SharePointDsc.SPDistributedCacheService.Tests.ps1
| - SPDistributedCacheService | ||
| - Fixed the domain being appended a second time when a server name in ServerProvisionOrder | ||
| is already a fully qualified domain name (FQDN). The doubled domain (server.domain.domain) | ||
| prevented the distributed cache from starting. The domain is now only added when the | ||
| server name is not already an FQDN. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add issue reference to the changelog entry.
The changelog guidelines require referencing issues using the format [issue #<issue_number>](https://github.com/<owner>/<repo>/issues/<issue_number>). The PR objectives state this fixes issue #1468, but the entry does not include a reference.
As per coding guidelines: "Reference issues using format issue #<issue_number>".
📝 Proposed fix
- SPDistributedCacheService
- Fixed the domain being appended a second time when a server name in ServerProvisionOrder
is already a fully qualified domain name (FQDN). The doubled domain (server.domain.domain)
prevented the distributed cache from starting. The domain is now only added when the
- server name is not already an FQDN.
+ server name is not already an FQDN. [issue `#1468`](https://github.com/dsccommunity/SharePointDsc/issues/1468)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - SPDistributedCacheService | |
| - Fixed the domain being appended a second time when a server name in ServerProvisionOrder | |
| is already a fully qualified domain name (FQDN). The doubled domain (server.domain.domain) | |
| prevented the distributed cache from starting. The domain is now only added when the | |
| server name is not already an FQDN. | |
| - SPDistributedCacheService | |
| - Fixed the domain being appended a second time when a server name in ServerProvisionOrder | |
| is already a fully qualified domain name (FQDN). The doubled domain (server.domain.domain) | |
| prevented the distributed cache from starting. The domain is now only added when the | |
| server name is not already an FQDN. [issue `#1468`](https://github.com/dsccommunity/SharePointDsc/issues/1468) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 24 - 28, Add the required issue reference for
issue `#1468` to the SPDistributedCacheService changelog entry, using the exact
linked format [issue `#1468`](https://github.com/<owner>/<repo>/issues/1468) with
the repository’s actual owner and name.
Source: Path instructions
There was a problem hiding this comment.
Since this PR is fixing a specific issue, please include this under the issue description. For examples, check further down the changelog.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
grep -n "issues/" CHANGELOG.md HISTORIC_CHANGELOG.md | head -50Length of output: 214
🏁 Script executed:
#!/bin/bash
grep -n -i "issue #\|fixes #\|resolves #" CHANGELOG.md HISTORIC_CHANGELOG.md | head -50Length of output: 238
🏁 Script executed:
#!/bin/bash
grep -n "#[0-9]" CHANGELOG.md HISTORIC_CHANGELOG.md | head -80Length of output: 213
🏁 Script executed:
#!/bin/bash
grep -n -i "issue" CHANGELOG.md | head -80Length of output: 4358
🏁 Script executed:
#!/bin/bash
grep -n "https://github.com" HISTORIC_CHANGELOG.md | head -20
grep -n -i "issue #\|#[0-9]\{3,\}" HISTORIC_CHANGELOG.md | head -40Length of output: 280
🏁 Script executed:
#!/bin/bash
grep -n "#[0-9][0-9][0-9]" CHANGELOG.md HISTORIC_CHANGELOG.mdLength of output: 381
🏁 Script executed:
#!/bin/bash
grep -n "#" CHANGELOG.md | grep -v "^[0-9]*:#" | grep -v "^\s*[0-9]*:##" | grep "#[A-Za-z0-9]" | grep -v "^[0-9]*:- \["Length of output: 440
🏁 Script executed:
#!/bin/bash
fd -i "PULL_REQUEST_TEMPLATE" -o -i "CONTRIBUTING"
cat .github/PULL_REQUEST_TEMPLATE.md 2>/dev/null
find . -iname "*.md" | xargs grep -l "issues/" 2>/dev/nullLength of output: 2955
@ykuijs Thanks for the clarification — you're right. I checked the repo's .github/PULL_REQUEST_TEMPLATE.md, which has a dedicated section for this:
#### This Pull Request (PR) fixes the following issues
...
- Fixes `#123`
- Fixes `#124`
So the issue reference belongs in the PR description under that section (e.g., Fixes #1468``), using GitHub's closing-keyword convention, rather than being embedded inline in the CHANGELOG.md entry text. I withdraw my earlier suggestion to add the issue link directly in the changelog line.
✏️ Learnings added
Learnt from: ykuijs
Repo: dsccommunity/SharePointDsc PR: 1480
File: CHANGELOG.md:24-28
Timestamp: 2026-07-10T11:01:43.107Z
Learning: In SharePointDsc (dsccommunity/SharePointDsc), issue references for a PR should be added to the PR description under the "This Pull Request (PR) fixes the following issues" section using GitHub closing keywords (e.g., "Fixes `#1468`"), not embedded inline within the CHANGELOG.md entry text under the Unreleased section.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
Pull Request (PR) description
When
ServerProvisionOrdercontains a server name that is already a fully qualifieddomain name (FQDN),
Set-TargetResourceunconditionally appended the domain a secondtime (
$currentServer = "$currentServer.$domain"), producingserver.domain.domain.Get-SPServiceInstancenever matched that doubled name, so the distributed cacheservice instance was never found and the cache failed to start.
The domain is now only appended when the server name is not already an FQDN
(
if ($currentServer -notlike "*.$domain")).Validated live on a real SharePoint Server Subscription Edition farm where SharePoint
stores the DC instance under the short name. With an FQDN entry in
ServerProvisionOrder:WFE1.<domain>.<domain>(doubled) → DC never found.WFE1.<domain>→ correct.This Pull Request (PR) fixes the following issues
Fixes #1468
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
help.
This change is