Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deployment errors in OpenDataFunctions.kql #1242

Closed
wants to merge 2 commits into from

Conversation

MSBrett
Copy link
Contributor

@MSBrett MSBrett commented Jan 9, 2025

πŸ› οΈ Description

This PR fixes deployment caused by

  • shortening the length of the OpenDataFunctions.kql script script to less than 131K chars
  • casting the type of data returned in tmp_resourcedetails to string.

πŸ”¬ 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)

@MSBrett MSBrett added Type: Bug πŸ› Something isn't working Tool: FinOps hubs Data pipeline solution labels Jan 9, 2025
@MSBrett MSBrett requested review from flanakin and Copilot January 9, 2025 22:20
@MSBrett MSBrett self-assigned this Jan 9, 2025

Choose a reason for hiding this comment

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

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

Comments suppressed due to low confidence (1)

src/templates/finops-hub/modules/scripts/IngestionSetup.kql:1253

  • The newly introduced ResourceName variable should be covered by tests to ensure its correct behavior.
| extend ResourceName = tostring(tmp_ResourceDetails.ResourceName)
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Review πŸ‘€ PR that is ready to be reviewed label Jan 9, 2025
@MSBrett MSBrett changed the title Msbrett/dev/open data script fix Fix deployment errors in OpenDataFunctions.kql Jan 10, 2025
@@ -6,31 +6,9 @@
with (docstring = 'Return details about the specified ID.', folder = 'OpenData')
Copy link
Collaborator

Choose a reason for hiding this comment

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

My VS code is telling me that it is still 132138 characters. I can't deploy it: Error BCP184: File 'C:\Projects\finops-toolkit\src\templates\finops-hub\modules\scripts\OpenDataFunctions.kql' exceeded maximum size of 131072 characters.

@flanakin flanakin added this to the 2025-01 - January milestone Jan 26, 2025
@@ -6,31 +6,9 @@
with (docstring = 'Return details about the specified ID.', folder = 'OpenData')
resource_type(id: string) {
dynamic({
"arizeai.observabilityeval/organizations": { "SingularDisplayName": "Azure Native ArizeAi Cloud Service" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are you deciding what to keep and what to remove?

Copy link
Collaborator

@flanakin flanakin left a comment

Choose a reason for hiding this comment

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

I don't love removing values. While I feel there has to be a better solution, I submitted a PR to split the file into multiple files since we know it will grow over time. I don't love it, but it should get us past the current issue. I validated the deployment so it should resolve the current issues. Thanks for digging into this.

#1269

@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 Jan 26, 2025
@flanakin
Copy link
Collaborator

Closing in favor of #1269

@flanakin flanakin closed this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity Tool: FinOps hubs Data pipeline solution Type: Bug πŸ› Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants