microsoft / finops-toolkit

Tools and resources to help you adopt and implement FinOps capabilities that automate and extend the Microsoft Cloud.
https://aka.ms/finops/toolkit
MIT License
305 stars 106 forks source link

fixed issue "Remove-FinOpsHub fails when there's only one resource" #983

Closed jamelachahbar closed 1 month ago

jamelachahbar commented 2 months ago

πŸ› οΈ Description

Get-HubIdentifier.ps1:

I optimized the logic to directly extract a valid 13-character alphanumeric string and improve efficiency. The current method generates substrings unnecessarily (tested it with Verbose Output), slowing down performance and producing irrelevant results. Regex Matching: Uses a regular expression to directly extract 13-character alphanumeric strings, removing the need for substring generation. Excludes Special Characters: Filters out matches with hyphens or underscores to ensure a clean identifier. Efficiency: Processes each string once, significantly improving performance for large collections.

Remove-FinOpsHub.ps1

This PR simplifies the Remove-FinOpsHub logic by replacing the .Reduce() method with a more sequential foreach loop for deleting resources. It improves error handling, ensuring each resource deletion attempt is properly logged and handled individually. The script now includes clearer verbose messages for filtered resources, deletion attempts, and success statuses. These changes provide better transparency and avoid potential issues caused by parallel deletion, making the script more predictable and easier to debug.

Fixes #

554

πŸ“‹ Checklist

πŸ”¬ How did you test this change?

  • [X] 🀏 Lint tests
  • [ ] 🀞 PS -WhatIf / az validate
  • [X] πŸ‘ Manually deployed + verified
  • [X] πŸ’ͺ Unit tests
  • [x] πŸ™Œ 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)
jamelachahbar commented 1 month ago

@flanakin , do you want me to squash and merge this?

flanakin commented 1 month ago

@jamelachahbar I looked at the integration tests we have here, which weren't working before. I was able to get something working for now, so we have an automated test for this. I updated that in the branch, so we're in a good state.

Thanks for getting this in!