Skip to content

Align Azure dependencies with Az.Accounts 5.5.1 - #61

Merged
potatoqualitee merged 1 commit into
mainfrom
codex/azure-core-az-compat
Sep 16, 2026
Merged

potatoqualitee merged 1 commit into
mainfrom
codex/azure-core-az-compat

Conversation

@potatoqualitee

Copy link
Copy Markdown
Member

Summary

  • update the packaged Azure stack to match Az.Accounts 5.5.1
  • pin Azure.Core 1.56.0, Azure.Identity 1.21.0, System.ClientModel 1.12.0, and MSAL 4.84.0
  • add regression coverage for both module import orders on PowerShell Core and Windows PowerShell
  • run that coverage in the existing assembly-conflict workflow

Why

A community report identified conflicts between dbatools.library's older core/lib/Azure.Core.dll and the newer version loaded by the Az module. Aligning the shared dependency stack avoids the order-dependent assembly mismatch.

Testing

  • dotnet build project/dbatools/dbatools.csproj --configuration Release
  • dotnet test project/dbatools.Tests/dbatools.Tests.csproj --framework net472 (331 passed)
  • ./build/build.ps1
  • ./tests/test-avoidconflicts.ps1 (5 passed)
  • ./tests/test-az-compatibility.ps1 (both import orders on PowerShell Core and Windows PowerShell)

@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review

Solid, well-motivated change — pinning the whole Azure/MSAL stack together (rather than just Azure.Identity) to match what Az.Accounts 5.5.1 ships is the right fix for the order-dependent conflict described in the summary, and extending the workflow trigger paths + adding a dedicated compatibility test is good CI hygiene.

Possible bug: hashtable likely doesn't survive the process boundary in test-az-compatibility.ps1

In Invoke-ImportOrderTest (tests/test-az-compatibility.ps1:60-113), the required-version map is passed to a spawned external process:

$result = & $PowerShellExecutable -NoProfile -Command {
    param($DbatoolsManifest, $AzManifest, $DbatoolsMsalPath, $AzMsalPath, $RequiredAzVersion, $Order, $RequiredVersions)
    ...
} -args $ManifestPath, $AzAccountsManifest, $DbatoolsMsalExtensionsPath, $AzMsalExtensionsPath, $AzAccountsVersion, $ImportOrder, $minimumVersions

$PowerShellExecutable is a file path ((Get-Command pwsh).Source), so & here launches a brand-new pwsh/powershell.exe process — it's not an in-process scriptblock invocation. When PowerShell builds arguments for a native executable it stringifies each one; Hashtable/OrderedDictionary don't override ToString(), so $minimumVersions most likely arrives in the child as the literal string "System.Collections.Specialized.OrderedDictionary" rather than a real dictionary.

Downstream, foreach ($dependencyName in $RequiredVersions.Keys) calls .Keys on that string. Since the child scriptblock sets $ErrorActionPreference = 'Stop' before the try, a missing-member error there would be escalated to terminating and land in the catch block as FAIL: ... — meaning the per-assembly version assertions (the actual point of the test) may never run correctly, and the test would either always fail regardless of the real assembly state, or (if the property-not-found is non-terminating in some PS version) silently no-op and always report PASS without validating anything. Both outcomes defeat the regression check this file exists to provide.

Given the PR description reports this script passing locally, it'd be worth double-checking empirically (e.g., temporarily Write-Host $RequiredVersions.GetType().FullName inside the child scriptblock) rather than taking my analysis on faith. If it is broken, a fix could be to serialize the required versions as JSON and ConvertFrom-Json on the other side, or simpler: have the child process just report back the loaded assembly names/versions as plain strings and do the comparison against $minimumVersions in the parent process, which already has it in scope.

Minor notes

  • Assert-PackagedDependencyVersions hard-fails if Azure.Core or Microsoft.Identity.Client.Extensions.Msal aren't loaded in a given import order, but silently continues past Azure.Identity, System.ClientModel, and Microsoft.Identity.Client when they weren't loaded (tests/test-az-compatibility.ps1:83-90). System.ClientModel in particular is the assembly that's caused conflicts before (see the hasIncompatibleClientModel check in dbatools.library.psm1), so it might be worth asserting it's loaded too rather than letting that check be skippable.
  • The new script signals failure via throw rather than exit 1 (unlike test-avoidconflicts.ps1, which explicitly does exit 0/exit 1). The workflow's if ($LASTEXITCODE -ne 0) { ...; exit 1 } guard added for this step (.github/workflows/test-avoidconflicts.yml:320-325) will likely never trigger, since an uncaught throw already aborts the step before that line runs — not wrong, just presumably dead code copied from the sibling pattern.
  • CLAUDE.md points to a dependency-constraints doc for version ceilings; none of Azure.Core/Azure.Identity/System.ClientModel/MSAL currently have documented ceilings, but this PR is bumping them meaningfully (e.g. Azure.Core 1.41→1.56, Azure.Identity 1.18→1.21). Might be worth a follow-up note there now that they're pinned together, so future bumps know to keep the whole stack (plus Microsoft.Identity.Client.Extensions.Msal) in lockstep with whatever Az.Accounts version is targeted.

Nits (non-blocking)

  • No C# was touched, so the LangVersion/cmdlet rules in CLAUDE.md don't apply here; the new test file is 173 lines, well under the 400-line limit.

Nice catch tracking this down to a real community-reported conflict — the packaged-DLL version assertions (Assert-PackagedDependencyVersions) are a good, low-risk sanity check regardless of the process-boundary issue above.

@potatoqualitee
potatoqualitee merged commit b1b63e0 into main Sep 16, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants