Skip to content

Enhanced unified function to handle errors, added NoWait and fix endless loop for LRO #132

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

Merged
merged 17 commits into from
Jun 25, 2025

Conversation

NowinskiK
Copy link
Contributor

@NowinskiK NowinskiK commented Jun 17, 2025

Pull Request

Fixes #131
Fixes #123

Pull Request (PR) description

  • Removed Revision History from Get-FabricSQLDatabase, Get-FabricSQLDatabase
  • Added NoWait switch parameter to New-FabricSQLDatabase
  • Enhanced logic in unified function Test-FabricApiResponse to handle API results and moved it to private functions
  • Remove-FabricSQLDatabase uses unified function to handle API results
  • Fixed bug in Get-FabricLongRunningOperation - Uri was incorectly created
  • Fixed bug in Get-FabricLongRunningOperationResult - uses correct statusCode
  • Applied splatting for several parameters in Invoke-FabricRestMethod and output results in debug mode

Task list

  • The PR represents a single logical change. i.e. Cosmetic updates should go in different PRs.
  • Added an entry under the Unreleased section of in the CHANGELOG.md as per format.
  • Local clean build passes without issue or fail tests (build.ps1 -ResolveDependency -Tasks build, test).
  • Comment-based help added/updated.
  • Examples appropriately added/updated.
  • Unit tests added/updated..
  • Integration tests added/updated (where possible).
  • Documentation added/updated (where applicable).
  • Code follows the contribution guidelines.

@NowinskiK NowinskiK changed the title Bug #131 and nowait #123 Enhanced unified function to handle errors, added NoWait and fix endless loop for LRO Jun 17, 2025
Copy link

github-actions bot commented Jun 17, 2025

Linux Test Results

4 864 tests   4 863 ✅  40s ⏱️
  591 suites      1 💤
    1 files        0 ❌

Results for commit 8d70e40.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 17, 2025

WinPS51 Test Results

5 068 tests   5 067 ✅  44s ⏱️
  592 suites      1 💤
    1 files        0 ❌

Results for commit 8d70e40.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 17, 2025

WinPS71 Test Results

5 068 tests   5 067 ✅  46s ⏱️
  592 suites      1 💤
    1 files        0 ❌

Results for commit 8d70e40.

♻️ This comment has been updated with latest results.

@NowinskiK NowinskiK requested a review from tiagobalabuch June 17, 2025 01:45
@SQLDBAWithABeard
Copy link
Contributor

So that you understand, this is one of my bug bears and I am frequently on video saying that I will call it out publicly when I see it so I am duty bound to do this :-)

Please do not use the commit message for the what was done as that does not provide value.

To summarise an hour long session, The commit message should be the why it was done or the intended result of the change so that the commit history is of use to future you and your team. I am not asking you to change the three commits that you have done in this PR but please, from this point on.

@jpomfret
Copy link
Contributor

Hey @NowinskiK - some good updates, thanks for the PR!

Just a reminder though to try and keep each PR small and representing one logical change, not always easy I know! But it's ok to open multiple PRs for different things - just makes reviewing and testing easier.

Co-authored-by: Jess Pomfret <jpomfret7@gmail.com>
Signed-off-by: Kamil Nowinski <kamil@nowinski.net>
@SQLDBAWithABeard
Copy link
Contributor

@NowinskiK - please read the comment about commit messages.

Co-authored-by: Jess Pomfret <jpomfret7@gmail.com>
Signed-off-by: Kamil Nowinski <kamil@nowinski.net>
@NowinskiK
Copy link
Contributor Author

Hey @NowinskiK - some good updates, thanks for the PR!

Just a reminder though to try and keep each PR small and representing one logical change, not always easy I know! But it's ok to open multiple PRs for different things - just makes reviewing and testing easier.

@jpomfret, yes, I know, I remember and agree. Believe me, 😊 I already minimised the scope of this PR as I was working on new sets of features when I noticed this error. The other functions in this change set are mainly for testing the fixed approach.
I will try to do this better next time!

@NowinskiK
Copy link
Contributor Author

So that you understand, this is one of my bug bears and I am frequently on video saying that I will call it out publicly when I see it so I am duty bound to do this :-)

Please do not use the commit message for the what was done as that does not provide value.

To summarise an hour long session, The commit message should be the why it was done or the intended result of the change so that the commit history is of use to future you and your team. I am not asking you to change the three commits that you have done in this PR but please, from this point on.

Read this. Makes sense! I will do my best and improve moving forward.

Copy link
Contributor

@tiagobalabuch tiagobalabuch left a comment

Choose a reason for hiding this comment

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

  1. In the Test-FabricApiResponse, there will be LROs with no results, so we need to consider how to handle these scenarios (I got stuck on this and my solution was to add a new parameter. Let's work together to see if there's a better approach). Otherwise, you'll end up with an object that has an error column saying there's no result.
  2. Now, there are two calls that must be made in a function: Invoke-FabricRestMethod and Test-FabricApiResponse. This means whenever a new function is developed, it must call Test-FabricApiResponse, and people might forget. My suggestion is to make this call inside Test-FabricApiResponse. That way, there's only one place to change in the future if needed.

@NowinskiK
Copy link
Contributor Author

@tiagobalabuch thanks for the input.
ad.1) Could you elaborate, please, and give an example?
ad.2) Sure. I was thinking about that in the next iteration. I will remain Invoke-FabricRestMethod as main function to execute, and add Test-FabricApiResponse at the end. I hope you put typo over there and you meant "make this call inside Invoke-FabricRestMethod" (not Test-FabricApiResponse).
I will make the run of Test optional, so all existing functions will not run it by default, so they can handle it themselves.

@tiagobalabuch
Copy link
Contributor

@NowinskiK and I had a call and decided to keep two separate functions for now. For LRO with no results, we'll leave it as is since the function currently returns the results.

Copy link
Contributor

@SQLDBAWithABeard SQLDBAWithABeard left a comment

Choose a reason for hiding this comment

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

image
I dont think we should be returning this output to the users when we specify nowait

@NowinskiK
Copy link
Contributor Author

Why not? What do you suggest instead?

@SQLDBAWithABeard
Copy link
Contributor

Why not? What do you suggest instead?

What does this output mean?
What does it have to do with creating a new Fabric SQL Database?

@NowinskiK
Copy link
Contributor Author

This output contains details about LRO (Long-Running Operations), which normally (without -NoWait) would be handled within the function and return the final result of the operation, but in this case, the operation has been kicked off (async) - hence the details.
If the user wants to check the operation's status later, they need at least the operationId.

@SQLDBAWithABeard
Copy link
Contributor

This output contains details about LRO (Long-Running Operations), which normally (without -NoWait) would be handled within the function and return the final result of the operation, but in this case, the operation has been kicked off (async) - hence the details. If the user wants to check the operation's status later, they need at least the operationId.

So it sounds to me like it would be much more useful for folk using the commands if they had a message that included that information instead of the entire hashtable.

@tiagobalabuch
Copy link
Contributor

@NowinskiK maybe personalize the return a bit more. Most of the info isn’t really needed, so it’d be better to highlight what actually matters, like retry, operation ID, and location. Other than that, I’m happy with this PR and will go ahead and approve it.

@NowinskiK
Copy link
Contributor Author

@SQLDBAWithABeard @tiagobalabuch it's done now. The result:
image

Copy link
Contributor

@tiagobalabuch tiagobalabuch left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@SQLDBAWithABeard SQLDBAWithABeard dismissed their stale review June 25, 2025 18:17

No longer required

@NowinskiK NowinskiK merged commit ed114fe into develop Jun 25, 2025
8 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in FabricTools-FeatureRelease Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Long Running Operations do not complete and return to the host New-FabricSQLDatabase should provide progress and maybe a -NoWait option
4 participants