Repository navigation
[Open Data] Sort instance size flexibility output ordinally so regeneration is host-independent - #2331
Conversation
…ependent The generator sorted with a bare Sort-Object, whose comparer follows the current culture. Culture-aware collation orders "Dasv4 Series" before "DCadsv5-series DedicatedHost" while an ordinal comparison puts the uppercase letter first, so the same records regenerated on a differently configured host reorder a few hundred rows without changing a value. A dispatch of the Update Instance Size Flexibility workflow after #2308 reproduced the published dataset exactly -- same 1,363 rows, same groups, same ratios -- and still pushed a 247-line diff that was pure reordering. Left alone, every scheduled run would push that diff whether or not any ISF data changed, burying real changes in review. #2308 pinned InvariantCulture on both parse sites and the write path but left the sort key culture-dependent. This applies the same treatment there, via an explicit [string]::CompareOrdinal comparer. Ordinal ordering is the order already committed, so InstanceSizeFlexibility.csv is unchanged. Adds four tests: group ordering, SKU ordering within a group, ordering held under de-DE, and an assertion that the published file is in ordinal order, which catches a file regenerated by hand on a differently configured host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Pull request overview
This pull request makes ISF CSV regeneration deterministic across host cultures using ordinal sorting, without changing the dataset.
Changes:
- Applies ordinal sorting to groups and SKUs.
- Adds culture-independent ordering and published-file validation tests.
File summaries
| File | Summary |
|---|---|
src/scripts/Update-InstanceSizeFlexibility.ps1 |
Applies deterministic ordinal sorting before export. |
src/powershell/Tests/Unit/Update-InstanceSizeFlexibility.Tests.ps1 |
Tests sorting behavior across cultures and published-file ordering. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude] PR Review
Summary: The fix is correct and focused. The 28 tests in the ISF suite pass locally, and reverting the script to Sort-Object makes 3 of the new tests fail, so they guard the change. No blockers or should-fix items.
💡 Suggestions (2)
List.Sortis unstable, unlikeSort-Object. Ties on group + SKU could reorder between runs with-Raw.- The comparer is duplicated in the script and the published-file test. Note that they must stay in sync.
Addresses review feedback on the ISF ordinal sort: - Record why List.Sort's instability cannot surface here: $seen is keyed by flexibility group + ArmSkuName and a duplicate ArmSkuName across groups throws, so the two sort keys order every record uniquely in the normalized and the -Raw path alike. No tiebreaker is reachable. - Note that the comparer in the published-file order test is a copy of the script's and has to stay in sync with it. Comments only; no behavior change. 28/28 ISF tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6adc0cc
into
dev
|
All Contributors (@all-contributors) please add Roland Krummenacher (@RolandKrummenacher) for code |
|
@microsoft-github-policy-service[bot] Roland Krummenacher (@RolandKrummenacher) already contributed before to code |
Summary
Follow-up to #2308. The ISF generator sorted with a bare
Sort-Object, whose comparer follows the current culture, so the same records regenerated on a differently configured host come out in a different order.InstanceSizeFlexibility.csvis unchanged — ordinal ordering is the order already committed ondev. This fixes only the script that produces it.How it surfaced
Dispatching Update Instance Size Flexibility after #2308 merged (run 35117329813) succeeded in 4m23s and reproduced the published dataset exactly — same 1,363 rows, same 213 groups, same ratios, verified as a set with line endings normalized. It still pushed a 247-insertion / 247-deletion diff, every line of it pure reordering.
Measured on a Windows host at
en-CH:DCadsv5-series DedicatedHostvsDasv4 SeriesDasv4firstDCadsv5firstBS Series High MemoryvsBasv2 SeriesBasv2firstBS SeriesfirstCulture-aware collation interleaves case; ordinal puts the uppercase letter first. The file committed on
devis in ordinal order while the script emitted culture-aware order, so every scheduled run would have pushed this same diff whether or not any ISF data changed — burying real changes in review, and flipping back and forth as the generator is run by hand on Windows per the README and onubuntu-latestby the workflow.The fix
#2308 pinned
InvariantCultureon both parse sites and the write path but left the sort key culture-dependent. This applies the same treatment there, with an explicit[string]::CompareOrdinalcomparer.One behavioral note:
Sort-Objectsilently tolerated a null record set, whichList[object]does not. An empty sweep is guarded explicitly — two existing tests (Queries VirtualMachines, RedisCache, and DedicatedHost but not BlockBlob by defaultandDoes not enumerate regions when -Location is supplied) caught that on the first revision.Test plan
Invoke-Pester ./src/powershell/Tests/Unit/* ./src/powershell/Tests/Lint/*— 6,023 passed, 0 failed (4 pre-existing skips), exactly +4 over the baseline of 6,019de-DE, and an assertion that the published file is in ordinal order — the last catches a file regenerated by hand on a differently configured hostInstanceSizeFlexibility.csvis byte-identical todevNo changelog entry: the ISF dataset is still under Unreleased → Added as a new dataset, and row ordering is not a documented property of it. Nothing user-visible changes.
🤖 Generated with Claude Code