Fix opendata-ci auto-commit gap and add ISF/eligibility PowerShell commands - #2298
Fix opendata-ci auto-commit gap and add ISF/eligibility PowerShell commands#2298Michael Flanakin (flanakin) wants to merge 5 commits into
Conversation
…commands and fix opendata-ci auto-commit gap Build-OpenData.ps1 -PowerShell already generates Get-OpenData* functions for every CSV in src/open-data/, but InstanceSizeFlexibility.csv and CommitmentDiscountEligibility.csv (added in #2199/#2251) never had their generated Private/ functions committed, and neither had a public Get-FinOps* cmdlet like the other open data (PricingUnit, Region, ResourceType, Service). Root cause: the opendata-ci.yml workflow used `git commit -a`, which only stages modified/deleted tracked files and silently ignores new (untracked) files. Since these two datasets' generated .ps1 files had never been committed, they stayed untracked forever and `git commit -a` never picked them up, so CI's "no diff" result masked a standing gap on every PR that touched either CSV (most recently #2295). Switched to `git add -A` before committing. Adds Get-FinOpsInstanceSizeFlexibility and Get-FinOpsCommitmentDiscountEligibility public cmdlets following the existing Get-FinOpsPricingUnit/Get-FinOpsService pattern, plus docs and TOC entries. Get-FinOpsCommitmentDiscountEligibility drops the `-Unique` post-filter used by other Get-FinOps* cmdlets: MeterId is already unique in the source data (92k+ rows), and Select-Object -Unique over that many PSCustomObjects was measured to hang for minutes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…merged CSV #2284 corrected which CommitmentDiscountEligibility column maps to reservations vs. savings plans (x_CommitmentDiscountSpendEligibility = savings plan/spend commitment, x_CommitmentDiscountUsageEligibility = reservation/usage commitment). Fixed the same mix-up in the new Get-FinOpsCommitmentDiscountEligibility cmdlet's help text and docs page. Also regenerated Get-OpenDataCommitmentDiscountEligibility.ps1 against the CSV that came in through the #2284 merge (72k rows changed) to keep the committed private function in sync with the source data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
A few newly added/edited docs strings are internally inconsistent with the cmdlet defaults and contain an incorrect command description that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the Open Data CI auto-commit behavior so newly generated PowerShell files are actually staged/committed, and adds new PowerShell cmdlets + docs/tests to expose the Instance Size Flexibility and Commitment Discount Eligibility datasets via the FinOpsToolkit module.
Changes:
- Update
opendata-ci.ymltogit add -Abefore committing generated files so untracked outputs are not silently skipped. - Add
Get-FinOpsInstanceSizeFlexibilityandGet-FinOpsCommitmentDiscountEligibilitypublic cmdlets (and corresponding unit tests). - Add/refresh generated
Get-OpenData*private functions + unit tests and wire up Microsoft Learn docs/TOC entries.
File summaries
| File | Description |
|---|---|
| .github/workflows/opendata-ci.yml | Fixes CI auto-commit to include newly generated (previously untracked) files. |
| src/powershell/Public/Get-FinOpsInstanceSizeFlexibility.ps1 | New public cmdlet for querying ISF open data with wildcard filters. |
| src/powershell/Public/Get-FinOpsCommitmentDiscountEligibility.ps1 | New public cmdlet for querying commitment discount eligibility by meter ID and eligibility flags. |
| src/powershell/Private/Get-OpenDataInstanceSizeFlexibility.ps1 | Generated private lookup function for ISF dataset. |
| src/powershell/Private/Get-OpenDataCommitmentDiscountEligibility.ps1 | Generated private lookup function for commitment discount eligibility dataset. |
| src/powershell/Tests/Unit/Get-OpenDataInstanceSizeFlexibility.Tests.ps1 | Unit test ensuring generated ISF function matches CSV row count. |
| src/powershell/Tests/Unit/Get-OpenDataCommitmentDiscountEligibility.Tests.ps1 | Unit test ensuring generated eligibility function matches CSV row count. |
| src/powershell/Tests/Unit/Get-FinOpsInstanceSizeFlexibility.Tests.ps1 | Unit tests for new public ISF cmdlet behavior and typing. |
| src/powershell/Tests/Unit/Get-FinOpsCommitmentDiscountEligibility.Tests.ps1 | Unit tests for new public eligibility cmdlet behavior and filters. |
| docs-mslearn/toolkit/powershell/powershell-commands.md | Adds the new cmdlets to the PowerShell command reference list. |
| docs-mslearn/toolkit/powershell/data/open-data-commands.md | Adds the new cmdlets to the open data command list. |
| docs-mslearn/toolkit/powershell/data/get-finopsinstancesizeflexibility.md | New Microsoft Learn reference page for the ISF cmdlet. |
| docs-mslearn/toolkit/powershell/data/get-finopscommitmentdiscounteligibility.md | New Microsoft Learn reference page for the eligibility cmdlet. |
| docs-mslearn/toolkit/open-data.md | Adds “See PowerShell” links for the two datasets. |
| docs-mslearn/TOC.yml | Adds TOC entries for the new cmdlet docs pages. |
Review details
Suppressed comments (1)
src/powershell/Public/Get-FinOpsCommitmentDiscountEligibility.ps1:16
- The comment-based help for UsageEligibility says the default is null, but the parameter default is "*" and the filter uses -like. Update the help text so it matches the actual default behavior.
.PARAMETER UsageEligibility
Optional. Filters to meters with the specified reservation (usage commitment) eligibility. FOCUS classifies a reservation as a usage commitment because you commit to a quantity of usage. Expected values: Eligible, Not Eligible. Default = null (all).
- Files reviewed: 13/15 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| | SpendEligibility | Optional. Filters to meters with the specified savings plan (spend commitment) eligibility. FOCUS classifies a savings plan as a spend commitment because you commit to an amount of money. Expected values: Eligible, Not Eligible. Default = null (all). | | ||
| | UsageEligibility | Optional. Filters to meters with the specified reservation (usage commitment) eligibility. FOCUS classifies a reservation as a usage commitment because you commit to a quantity of usage. Expected values: Eligible, Not Eligible. Default = null (all). | |
|
|
||
| - [Get-FinOpsCommitmentDiscountEligibility](Get-FinOpsCommitmentDiscountEligibility.md) – Gets commitment discount eligibility for a meter. | ||
| - [Get-FinOpsInstanceSizeFlexibility](Get-FinOpsInstanceSizeFlexibility.md) – Gets the instance size flexibility group and ratio for an ARM SKU. | ||
| - [Get-FinOpsPricingUnit](Get-FinOpsPricingUnit.md) – Gets an Azure region ID and name. |
|
|
||
| - [Get-FinOpsCommitmentDiscountEligibility](data/Get-FinOpsCommitmentDiscountEligibility.md) – Gets commitment discount eligibility for a meter. | ||
| - [Get-FinOpsInstanceSizeFlexibility](data/Get-FinOpsInstanceSizeFlexibility.md) – Gets the instance size flexibility group and ratio for an ARM SKU. | ||
| - [Get-FinOpsPricingUnit](data/Get-FinOpsPricingUnit.md) – Gets an Azure region ID and name. |
| .PARAMETER SpendEligibility | ||
| Optional. Filters to meters with the specified savings plan (spend commitment) eligibility. FOCUS classifies a savings plan as a spend commitment because you commit to an amount of money. Expected values: Eligible, Not Eligible. Default = null (all). | ||
|
|
Roland Krummenacher (RolandKrummenacher)
left a comment
There was a problem hiding this comment.
Thanks for splitting the CI fix out — git add -A is the right call, and I confirmed the old git commit -a genuinely skipped newly generated (untracked) files. The generated data itself checks out: both private functions match their CSVs exactly (92,624 and 1,353 rows, field for field), so the "generator dropped rows" hazard isn't present in this output. Eligibility semantics line up with the post-#2284 orientation, and the 6 new tests pass locally under Pester 6.
Requesting changes on one thing that's much cheaper to decide now than after this ships, plus a small correctness bug (inline).
The 16.2 MB generated file is the one I'd like to settle before merge. GitHub won't let me anchor a comment there (no diff entry for a file that size), so it goes here.
src/powershell/Private/Get-OpenDataCommitmentDiscountEligibility.ps1 is 16.2 MB / 92,633 lines, and FinOpsToolkit.psm1:7-17 dot-sources every Private/*.ps1 eagerly on import. Measured cold import: 0.72s without this file, 3.6-4.8s with it — every user of every cmdlet pays that, and Build-PsModule.ps1:83 ships the whole Private folder to PSGallery.
Second cost: Tests/Lint/Lint.Tests.ps1:43-44 runs Invoke-ScriptAnalyzer -IncludeRule $_ once per rule per file. I measured 10.9s for a single rule on this file, against 1.57s for the previously largest generated file. At ~75 rules that's roughly +14 minutes of lint on every src/powershell/** PR locally, and more on the Windows CI runner.
Embedding generated data as a function is the established pattern here, so this isn't a wrong approach — it's that this file is about 10x larger than anything we've embedded before, while the source CSV is only 5.5 MB. Import-Csv on demand, or lazy-loading behind a script-scoped cache, would avoid both costs. Happy to be overruled if the data has to be in-module, but I'd rather make that call now than after v15 is on the gallery.
Two more that can't be anchored because the files aren't in this diff:
src/templates/agent-plugin/skills/finops-toolkit/SKILL.md — the "Open data commands" table still lists only the five existing references. The plugin bundles the two new reference docs but never points at them, and no test enforces that table, so it will stay stale silently.
docs/open-data.md:40 — the Jekyll instance size flexibility tile keeps its hidden placeholder button (visibility:hidden) where every other dataset tile has a "See PowerShell" link. The docs-mslearn twin was updated in this PR, so the two sites now disagree on whether a cmdlet exists.
| Optional. The meter ID to look up. Accepts wildcards. Default = * (all). | ||
|
|
||
| .PARAMETER SpendEligibility | ||
| Optional. Filters to meters with the specified savings plan (spend commitment) eligibility. FOCUS classifies a savings plan as a spend commitment because you commit to an amount of money. Expected values: Eligible, Not Eligible. Default = null (all). |
There was a problem hiding this comment.
Default = null (all) doesn't match the implementation — the default is "*" (line 41), and passing $null explicitly doesn't mean "all": [string] $null coerces to "", and "Eligible" -like "" is $false, so -SpendEligibility $null returns 0 rows rather than everything. Verified against the branch.
Get-FinOpsPricingUnit:58 guards its equivalent case ($null -eq $BlockSize -or ...); this cmdlet doesn't. Either fix the help text to say Default = * (all) — matching how MeterId is documented two lines up — or add the null guard. Same wording on line 15 for UsageEligibility, and both are repeated in get-finopscommitmentdiscounteligibility.md.
| Ratio = $_.Ratio | ||
| } | ||
| } ` | ||
| | Select-Object -Property * -Unique |
There was a problem hiding this comment.
This is a no-op: the source has 1,353 rows and 1,353 distinct (InstanceSizeFlexibilityGroup, ArmSkuName, Ratio) triples — ArmSkuName alone is already unique — so nothing is ever deduplicated. It costs roughly 0.8s of the cmdlet's ~1.1s runtime.
Same reasoning you used to drop -Unique from the sibling cmdlet — worth dropping here too, or adding the equivalent "already unique per row" comment if you'd rather keep it defensively.
| git config --global user.email "finopstoolkit@users.noreply.github.com" | ||
| git commit -a -m "${{ env.CI_COMMIT_MESSAGE }}" | ||
| git add -A | ||
| git commit -m "${{ env.CI_COMMIT_MESSAGE }}" |
There was a problem hiding this comment.
Confirmed this is the actual gap: git commit -a stages only modifications to already-tracked files, so newly generated open-data files were never committed. Worth noting the failure was silent because a failing git commit is followed by git push, which exits 0 — the job stayed green while doing nothing. Might be worth a set -e or an explicit check later, but not in this PR.
|
@Michael Flanakin (@flanakin): you have some new feedback! Please review and resolve all comments and I'll let reviewers know by removing the |
|
Heads-up on an interaction with #2308, which grows
So merging this one first is the cleaner order, and I will handle the regeneration on my side. Worth noting for the auto-commit fix itself: Open Data CI ran green on #2308 (head |
|
Update on the interaction I flagged earlier: still there, but much smaller than it was. #2308 no longer backfills retired SKUs — that part was dropped. It now only widens the region sweep, so the CSV goes from 1,353 to 1,363 rows rather than 2,573. Same mechanism as before, so merging this one first remains the cleaner order and I will regenerate on my side afterwards. If #2308 lands first instead, note that Open Data CI would not catch it: its |
🛠️ Description
opendata-ci.ymlusedgit commit -aafter regenerating PowerShell functions fromsrc/open-data/*.csv.git commit -aonly stages modified/deleted tracked files — it silently skips new untracked files.InstanceSizeFlexibility.csv(#2199) andCommitmentDiscountEligibility.csv(#2251) never had their generatedGet-OpenData*.ps1functions committed, so they stayed untracked forever and every PR touching either CSV (most recently #2295) saw a "no diff" CI result that masked the standing gap. Fixed by switching togit add -Abeforegit commit.Also adds
Get-FinOpsInstanceSizeFlexibilityandGet-FinOpsCommitmentDiscountEligibilitypublic cmdlets, matching the existingGet-FinOpsPricingUnit/Get-FinOpsServicepattern — these two datasets had no PowerShell-facing lookup at all, only direct CSV consumption (Power BI, Optimization Engine).Bugs found and fixed along the way
Get-FinOpsCommitmentDiscountEligibility's initial draft used the sameSelect-Object -Property * -Uniquepost-filter as the otherGet-FinOps*cmdlets. At 92k+ rows this hung for minutes.MeterIdis already unique in the source data, so-Uniquewas dropped (both from the cmdlet and its test'sBeforeAll).Build-OpenData.ps1'sWrite-Command/Out-Filegeneration path silently dropped rows on large datasets on some runs (662 rows missing from oneCommitmentDiscountEligibilitygeneration, 1 row from anInstanceSizeFlexibilitygeneration). Regenerating fixed it both times. Not root-caused further — flagging here as a latent reliability risk in the generator for anyone touching it next.devpicked up fix(open-data,hubs): align commitment discount eligibility with FOCUS spend and usage categories #2284, which swapped which eligibility column maps to reservations vs. savings plans. Corrected the new cmdlet's help text/docs to match and regenerated the private function against the updated CSV.Fixes # (opendata-ci not running as expected on PRs like #2295)
📷 Screenshots
N/A — PowerShell module change
📋 Checklist
🔬 How did you test this change?
📦 Deploy to test?
🙋♀️ Do any of the following that apply?
📑 Did you update
docs/changelog.md?📖 Did you update documentation?
🤖 [AI] Generated with Claude Code