Skip to content

[Hubs] Split the Recommendations app#2052

Open
flanakin wants to merge 7 commits intofeatures/hubs-recsfrom
flanakin/split-recs
Open

[Hubs] Split the Recommendations app#2052
flanakin wants to merge 7 commits intofeatures/hubs-recsfrom
flanakin/split-recs

Conversation

@flanakin
Copy link
Collaborator

🛠️ Description

Split the monolithic Recommendations app.bicep into three focused modules to improve maintainability and separation of concerns:

  • AzureResourceGraph — Azure Resource Graph query execution
  • IngestionQueries — Ingestion query definitions and management
  • Recommendations — Slimmed down to core recommendation logic only

Also added a new Build-HubIngestionQueries.ps1 build script with corresponding unit and integration tests, and updated the hub module to wire up the new sub-modules.

📋 Checklist

🔬 How did you test this change?

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

🙋‍♀️ 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)

Copilot AI review requested due to automatic review settings March 12, 2026 10:19
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Review 👀 PR that is ready to be reviewed label Mar 12, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Splits the FinOps hubs Recommendations functionality into smaller Bicep modules (IngestionQueries orchestrator + AzureResourceGraph engine + slimmed Recommendations) and introduces a build-time generator script plus Pester coverage to keep query-file wiring maintainable.

Changes:

  • Updates hub.bicep to wire new IngestionQueries and AzureResourceGraph modules and refactors inter-module dependencies/outputs.
  • Refactors Recommendations/app.bicep to focus on uploading query/schema assets, with query file entries generated at build time.
  • Adds Build-HubIngestionQueries.ps1 plus unit/integration tests, and extends Build-Toolkit.ps1 + template .build.config to run custom build scripts.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
src/templates/finops-hub/modules/hub.bicep Wires new sub-modules and updates outputs/params for the split Recommendations design.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Recommendations/app.bicep Slims Recommendations to storage uploads and introduces generated query-file section markers.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/IngestionQueries/metadata.bicep Adds exported metadata type for the new IngestionQueries app.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/IngestionQueries/app.bicep New orchestrator app: schedule trigger + pipelines to dispatch query execution and manage ingestion/manifests.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/IngestionQueries/README.md Documents the new orchestration model and engine contract.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/AzureResourceGraph/metadata.bicep Adds exported metadata type for the ARG engine app.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/AzureResourceGraph/app.bicep New engine app: ARG dataset and queries_ResourceGraph_ExecuteQuery pipeline implementation.
src/templates/finops-hub/modules/Microsoft.FinOpsHubs/AzureResourceGraph/README.md Documents the ARG engine behavior, dependencies, and limitations.
src/templates/finops-hub/.build.config Registers a template-specific custom build script to generate query-file wiring.
src/scripts/Test-PowerShell.ps1 Broadens Hubs test selection to include new Hubs*.Tests.ps1 files.
src/scripts/Build-Toolkit.ps1 Adds support for running per-template custom build scripts after copying to release output.
src/scripts/Build-HubIngestionQueries.ps1 New generator: builds Bicep loadTextContent() map(s) from query JSON files.
src/powershell/Tests/Unit/HubsIngestionQueries.Tests.ps1 New unit tests validating query/schema JSON and compiling finops-hub Bicep (when CLI is present).
src/powershell/Tests/Integration/HubsIngestionQueries.Tests.ps1 New integration tests validating ARG queries execute and match expected result schema/perf.

flanakin and others added 3 commits March 12, 2026 05:04
… path, fix conditional output

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Handle empty ARG query results gracefully instead of failing the pipeline.
Fix error propagation in ingestion query execution.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add mapComplexValuesToString to ADF schema so JSON string columns
  are preserved in parquet (was silently dropping x_RecommendationDetails)
- Wrap AdvisorCost bag_merge in tostring() for parquet compatibility
- Fix parse_resourceid() to handle bare /subscriptions/{id} resource IDs
- Add mv-apply key renaming in transform to normalize x_RecommendationDetails
  keys to x_PascalCase (Advisor extendedProperties use camelCase)
- Derive ResourceType display name from ResourceId via resource_type() lookup
- Fix description fallback to use x_RecommendationSolution then
  x_RecommendationSubCategory instead of nonexistent x_RecommendationMessage
- Use SubAccountName for ResourceName on subscription-level recommendations

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@flanakin flanakin mentioned this pull request Mar 14, 2026
2 tasks
@flanakin flanakin requested a review from Copilot March 14, 2026 13:02
@flanakin flanakin changed the title Split the Recommendations app [Hubs] Split the Recommendations app Mar 14, 2026
@flanakin flanakin added the Tool: FinOps hubs Data pipeline solution label Mar 14, 2026
Copy link
Collaborator

@RolandKrummenacher RolandKrummenacher left a comment

Choose a reason for hiding this comment

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

Code review -- 4 inline comments. Overall the architecture is solid; these are mostly guardrails for future maintainability.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

- Add safety comment on non-null assertion in hub.bicep
- Add trusted-source comment on query text in ARG app.bicep
- Fix dataset name in ARG README (resourceGraph -> azureResourceGraph)
- Add error handling for custom build scripts in Build-Toolkit.ps1

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

Co-Authored-By: RolandKrummenacher <RolandKrummenacher@users.noreply.github.com>
Co-Authored-By: copilot-pull-request-reviewer <copilot-pull-request-reviewer@users.noreply.github.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@flanakin
Copy link
Collaborator Author

🤖 [AI][Claude] PR Update Summary (Round 2)

Addressed: 11 thread(s)

  • ✅ Implemented: 4 (from RolandKrummenacher + Copilot duplicates)
  • ✅ Resolved: 7 (Copilot duplicates of already-resolved metadata/group issues)

Key changes: safety comment on ! assertion, trusted-source comment on query text, README dataset name fix, build script error handling.

- Use ingestionQueries.queries.container instead of core.containers.config for consistency

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

Co-Authored-By: copilot-pull-request-reviewer <copilot-pull-request-reviewer@users.noreply.github.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@flanakin
Copy link
Collaborator Author

🤖 [AI][Claude] PR Update Summary (Round 3)

Addressed: 3 thread(s)

  • ✅ Implemented: 1 (use IngestionQueries metadata for query container)
  • ✅ Resolved: 2 (duplicates of previously fixed issues)

Changed core.containers.configingestionQueries.queries.container in Recommendations/app.bicep for metadata contract consistency.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.

- Reset $LASTEXITCODE before custom build script invocation to prevent stale values
- Throw on missing build scripts instead of warning
- Guard mv-apply for null/empty x_RecommendationDetails bags
- Clarify integration test comment about x_Source* column origin

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

Co-Authored-By: Copilot <copilot@users.noreply.github.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@flanakin
Copy link
Collaborator Author

🤖 [AI][Claude] PR Update Summary

Addressed: 7 thread(s)

  • 🔁 Duplicates dismissed: 3
  • ✅ Implemented: 3
  • 🤔 Needs discussion: 1

Changes:

  • Build-Toolkit.ps1: Reset $LASTEXITCODE = 0 before script invocation; throw instead of Write-Warning for missing scripts
  • IngestionSetup_v1_2.kql: Guard mv-apply with bag_merge(coalesce(...)) placeholder to prevent row drops on null/empty x_RecommendationDetails
  • HubsIngestionQueries.Tests.ps1: Clarified comment — x_Source* columns come from ADF pipeline, not ARG queries (no column changes needed)

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 Tool: FinOps hubs Data pipeline solution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants