Skip to content

Conversation

flanakin
Copy link
Collaborator

πŸ› οΈ Description

This PR reorganizes FinOps hubs bicep into a cleaner, more extensible structure while fixing build issues and improving maintainability.

  • Moved all JSON schemas into relevant module directories
  • Split large monolithic files (e.g., 5K+ line dataFactory.bicep) into smaller modules
  • Moved all framework modules into the fx folder
  • Organized apps into X.Y/Z folders for each publisher/app:
    • Microsoft.CostManagement/Exports – Cost export functionality
    • Microsoft.CostManagement/ManagedExports – Managed exports
    • Microsoft.FinOpsHubs/Analytics – Data Explorer and analytics
    • Microsoft.FinOpsHubs/Core – Core hub infrastructure
    • Microsoft.FinOpsHubs/RemoteHub – Remote hub functionality
  • Resolved all Bicep compilation errors/warnings
  • Added #disable-next-line BCP318 with explanations for null safety
  • Fixed Build-Toolkit.ps1 param generation bug
  • Updated PowerShell build scripts with correct paths

πŸ“‹ 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)
  • βœ… 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)

@flanakin flanakin marked this pull request as draft August 12, 2025 07:21
@flanakin flanakin marked this pull request as ready for review August 12, 2025 10:45
@RolandKrummenacher
Copy link
Collaborator

RolandKrummenacher commented Aug 20, 2025

I'm getting an error while deploying from your branch, since I'm using a hub name that is a bit longer:

image

I'm trying to update an existing deployment.

@flanakin
Copy link
Collaborator Author

flanakin commented Sep 1, 2025

@RolandKrummenacher Sorry for the delay. I fixed this.

@RolandKrummenacher
Copy link
Collaborator

@RolandKrummenacher Sorry for the delay. I fixed this.

Works now. Thanks.

@MSBrett
Copy link
Contributor

MSBrett commented Sep 3, 2025

image Getting a bunch of timing related failures.

flanakin and others added 2 commits September 15, 2025 10:46
- Add @maxlength(22) to clusterName parameter in Analytics app.bicep
- Implement automatic truncation in deployment script
- Ensure hub names up to 18 chars work (18 + 4 for '-adx' = 22 max)
- Prevent deployment failures for longer hub names
Copy link
Contributor

@MSBrett MSBrett left a comment

Choose a reason for hiding this comment

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

βœ… LGTM - Tested and validated

Deployments tested successfully:

  • βœ… Public networking (eastus2)
  • βœ… Private networking (eastus2)
  • βœ… Long hub names with ADX truncation
  • βœ… Multiple subscriptions

Changes validated:

  • βœ… Modular bicep organization works
  • βœ… ADX cluster name validation fixed (@maxlength(22))
  • βœ… Private endpoints correctly configured
  • βœ… Resource creation and networking successful

Note for future optimization:
Multiple StopTriggers deployment scripts (one per module) run sequentially with private endpoints, adding ~1 min overhead per script. Consider consolidating into a single script in a future PR for efficiency.

Commits added:

  • b4b1b13: Fix ADX cluster name length validation

@MSBrett MSBrett removed the Needs: Review πŸ‘€ PR that is ready to be reviewed label Oct 1, 2025
@flanakin
Copy link
Collaborator Author

@microsoft-github-policy-service agree [company="Xerilium"]

@flanakin
Copy link
Collaborator Author

@microsoft-github-policy-service agree company="Xerilium"

@flanakin flanakin merged commit ce22272 into dev Oct 11, 2025
3 of 4 checks passed
@flanakin flanakin deleted the flanakin/hubs-ext branch October 11, 2025 03:34
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.

4 participants