Skip to content

Fix opendata-ci auto-commit gap and add ISF/eligibility PowerShell commands - #2298

Open
Michael Flanakin (flanakin) wants to merge 6 commits into
devfrom
flanakin/opendata-ci-untracked-files
Open

Michael Flanakin (flanakin) wants to merge 6 commits into
devfrom
flanakin/opendata-ci-untracked-files

Conversation

@flanakin

Copy link
Copy Markdown
Collaborator

🛠️ Description

opendata-ci.yml used git commit -a after regenerating PowerShell functions from src/open-data/*.csv. git commit -a only stages modified/deleted tracked files — it silently skips new untracked files. InstanceSizeFlexibility.csv (#2199) and CommitmentDiscountEligibility.csv (#2251) never had their generated Get-OpenData*.ps1 functions 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 to git add -A before git commit.

Also adds Get-FinOpsInstanceSizeFlexibility and Get-FinOpsCommitmentDiscountEligibility public cmdlets, matching the existing Get-FinOpsPricingUnit/Get-FinOpsService pattern — 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 same Select-Object -Property * -Unique post-filter as the other Get-FinOps* cmdlets. At 92k+ rows this hung for minutes. MeterId is already unique in the source data, so -Unique was dropped (both from the cmdlet and its test's BeforeAll).
  • Build-OpenData.ps1's Write-Command/Out-File generation path silently dropped rows on large datasets on some runs (662 rows missing from one CommitmentDiscountEligibility generation, 1 row from an InstanceSizeFlexibility generation). Regenerating fixed it both times. Not root-caused further — flagging here as a latent reliability risk in the generator for anyone touching it next.
  • Merging in dev picked 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?

  • 🤏 Lint tests
  • 🤞 PS -WhatIf / az validate
  • 👍 Manually deployed + verified
  • 💪 Unit tests
  • 🙌 Integration tests

📦 Deploy to test?

  • Hubs + ADX (managed)
  • Hubs + Fabric (manual) — URI:
  • Hubs (manual)
  • Hubs (no data)
  • Workbooks
  • Alerts

🙋‍♀️ Do any of the following that apply?

  • 🚨 This is a breaking change.
  • 🤏 The change is less than 20 lines of code.

📑 Did you update docs/changelog.md?

  • ✅ Updated changelog (required for dev PRs)
  • ➡️ Will add log in a future PR (feature branch PRs only)
  • ❎ Log not needed (small/internal change)

📖 Did you update documentation?

  • ✅ Public docs in docs (required for dev)
  • ✅ Public docs in docs-mslearn (required for dev)
  • ✅ Internal dev docs in docs-wiki (required for dev)
  • ✅ Internal dev docs in src (required for dev)
  • ➡️ Will add docs in a future PR (feature branch PRs only)
  • ❎ Docs not needed (small/internal change)

🤖 [AI] Generated with Claude Code

…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>
@flanakin
Michael Flanakin (flanakin) marked this pull request as ready for review September 2, 2026 15:55
Copilot AI lite review requested due to automatic review settings September 2, 2026 15:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.yml to git add -A before committing generated files so untracked outputs are not silently skipped.
  • Add Get-FinOpsInstanceSizeFlexibility and Get-FinOpsCommitmentDiscountEligibility public 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.

Comment thread docs-mslearn/toolkit/powershell/data/get-finopscommitmentdiscounteligibility.md Outdated
Comment thread docs-mslearn/toolkit/powershell/data/open-data-commands.md Outdated
Comment thread docs-mslearn/toolkit/powershell/powershell-commands.md Outdated
Comment thread src/powershell/Public/Get-FinOpsCommitmentDiscountEligibility.ps1
@flanakin Michael Flanakin (flanakin) added this to the v15 milestone Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/powershell/Public/Get-FinOpsCommitmentDiscountEligibility.ps1 Outdated
Comment thread src/powershell/Public/Get-FinOpsInstanceSizeFlexibility.ps1 Outdated
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 }}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI] 👍 Acknowledged — thanks for confirming the root cause. Agreed a set -e or explicit exit-code check would help catch this class of failure earlier; leaving that for a separate follow-up as you suggested.

@microsoft-github-policy-service microsoft-github-policy-service Bot added Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity and removed Needs: Review 👀 PR that is ready to be reviewed labels Sep 7, 2026
@microsoft-github-policy-service

Copy link
Copy Markdown

@Michael Flanakin (@flanakin): you have some new feedback!

Please review and resolve all comments and I'll let reviewers know by removing the Needs: Attention label. If I miss anything, just reply with #needs-review and I'll update the status.

@RolandKrummenacher

Copy link
Copy Markdown
Collaborator

Heads-up on an interaction with #2308, which grows src/open-data/InstanceSizeFlexibility.csv from 1,353 to 2,573 rows (restoring retired SKUs the Catalogs API no longer returns).

Get-OpenDataInstanceSizeFlexibility.ps1 in this PR is generated from that CSV at its current 1,353 rows, so the merge order matters:

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 77949cd8) and generated nothing, because git commit -a skipped the still-untracked ISF file — exactly the gap git add -A closes here.

@RolandKrummenacher

Copy link
Copy Markdown
Collaborator

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. Get-OpenDataInstanceSizeFlexibility.ps1 here is generated at 1,353, so it would still be stale by 10 rows.

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 paths: src/open-data/*.csv filter does not match this PR.

@flanakin

Copy link
Copy Markdown
Collaborator Author

🤖 [AI] Addressing the review-body items that couldn't be anchored to a diff line:

SKILL.md "Open data commands" table — ✅ Implemented. Added the two missing rows (get-finopscommitmentdiscounteligibility.md, get-finopsinstancesizeflexibility.md); both docs were already bundled under references/, just unlisted.

docs/open-data.md:40 (ISF tile missing "See PowerShell") — ✅ Implemented. Added the link, matching the pattern used by the Pricing units / Regions / Services tiles.

16.2 MB Get-OpenDataCommitmentDiscountEligibility.ps1 (import time + lint cost) — 🤔 Needs discussion, but agreed and planned. You're right that this isn't a wrong approach, just wrong at this scale — we don't want to lazy-load just this one dataset and leave the other five inconsistent. Rather than block this PR on a design-relayout, I've scoped a follow-up PR that reworks all six Get-OpenData* functions (and Build-OpenData.ps1, the generator) to lazy-load via Import-Csv with a script-scoped cache, replacing the embedded-literal pattern everywhere at once. Will land as its own PR referencing this thread.

Roland Krummenacher (@RolandKrummenacher) — also thanks for the heads-up on the #2308 merge-order interaction; agreed this PR should land first and appreciate you handling the regeneration on your side.

@microsoft-github-policy-service microsoft-github-policy-service Bot added Needs: Triage 🔍 Untriaged issue needs to be reviewed Needs: Review 👀 PR that is ready to be reviewed and removed Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity labels Sep 16, 2026
- Fix eligibility docs/help to say Default = * (all) instead of null, matching actual behavior
- Fix real bug: explicit -SpendEligibility/-UsageEligibility $null coerced to empty string and silently returned 0 rows; now normalized to * (all), with a regression test
- Fix copy-pasted Get-FinOpsPricingUnit description in both command list docs
- Drop no-op -Unique from Get-FinOpsInstanceSizeFlexibility (ArmSkuName is already unique per row), mirroring the sibling cmdlet fix
- Add missing ISF/eligibility rows to the agent plugin SKILL.md open-data commands table
- Add See PowerShell link to the ISF tile in docs/open-data.md

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: RolandKrummenacher <RolandKrummenacher@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@flanakin

Copy link
Copy Markdown
Collaborator Author

🤖 [AI] PR Update Summary

Addressed: 8 thread(s)/comment(s)

  • ✅ Implemented: 6
  • 🤔 Needs discussion: 1 (16.2 MB generated file — agreed, scoped as a separate follow-up PR reworking all six Get-OpenData* functions + the generator to lazy-load)
  • 👍 Acknowledged: 1 (silent-failure root cause confirmation)

Key changes: Fixed the SpendEligibility/UsageEligibility explicit-$null bug (was silently returning 0 rows), corrected copy-pasted/stale docs (Get-FinOpsPricingUnit description, eligibility parameter defaults, SKILL.md table, ISF tile link), and dropped a no-op -Unique call from Get-FinOpsInstanceSizeFlexibility. All 10 relevant Pester tests pass; PSScriptAnalyzer clean on changed files.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Review 👀 PR that is ready to be reviewed Needs: Triage 🔍 Untriaged issue needs to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants