-
Notifications
You must be signed in to change notification settings - Fork 4k
[Az.ServiceFabric] Updating SDK After Typespec Migration #28169
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Service Fabric managed clusters SDK to match the latest Typespec-based REST spec, adjusts cmdlet implementations accordingly, and refreshes scenario tests to use dynamic values.
- Renamed VM-related properties from camelCase (
VmInstanceCount
,DataDiskSizeGB
, etc.) to PascalCase (VMInstanceCount
,DataDiskSizeGb
, etc.). - Swapped exception types from
ErrorModelException
/ErrorModel
toErrorResponseException
/ErrorResponse
and added typed overloads for long-running operation polling with enhanced progress reporting. - Updated cmdlets to use named parameters for clarity and adjusted tests to generate random passwords, include tags, and supply
PackageUrl
where required.
Reviewed Changes
Copilot reviewed 15 out of 200 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
PSManagedNodeType.cs | Corrected casing on VM properties to match generated model. |
ServiceFabricCommonCmdletBase.cs | Replaced ErrorModelException with ErrorResponseException and ErrorModel with ErrorResponse . |
SetAzServiceFabricManagedClusterService.cs | Updated PollLongRunningOperation call to use new generic overload. |
ServiceFabricManagedCmdletBase.cs | Swapped management client type, added three overloads of PollLongRunningOperation with enhanced polling logic. |
SetAzServiceFabricManagedNodeType.cs & related cmdlets | Adjusted all VM property references (Vm* → VM* ). |
ManagedApplicationCmdletBase.cs & SetAzServiceFabricManagedClusterApplicationType.cs | Switched to named parameters for CreateOrUpdateWithHttpMessagesAsync . |
ServiceFabric.Test.csproj | Added PEM files to test output. |
ScenarioTests/*.ps1 & Common.ps1 | Refactored tests to use random secure strings, tags, and updated package URLs. |
@@ -71,15 +71,15 @@ protected void PollLongRunningOperation(Rest.Azure.AzureOperationResponse beginR | |||
this.PollLongRunningOperation(response2); | |||
} | |||
|
|||
protected T PollLongRunningOperation<T>(AzureOperationResponse<T> beginRequestResponse) where T : class | |||
protected TBody PollLongRunningOperation<TBody>(AzureOperationResponse<TBody> beginRequestResponse) where TBody : class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The three overloads of PollLongRunningOperation
share nearly identical polling logic; consider refactoring common code into a helper method to reduce duplication.
Copilot uses AI. Check for mistakes.
@@ -157,6 +157,180 @@ protected T PollLongRunningOperation<T>(AzureOperationResponse<T> beginRequestRe | |||
return result?.Body; | |||
} | |||
|
|||
protected void PollLongRunningOperation<THeader>(AzureOperationHeaderResponse<THeader> beginRequestResponse) where THeader : class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this copied over from somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There used to be just one T PollLongRunningOperation<T>(AzureOperationHeaderResponse<T> beginRequestResponse)
, but it wouldn't accept inputs of type AzureOperationHeaderResponse<TBody, THeader>
or AzureOperationHeaderResponse<THeader>
. There is probably a far more elegant way to do refactor the overloads, but Copilot wasn't helpful, and I didn't come across anything too similar in the other modules. Was hoping for suggestions
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Please update the ChangeLog. |
@YanaXu Updated the change log, let me know if there are other details I need to add. The type property changes do change, the capitalization from the generated code from updated spec is different from previous version. I'm not sure why, since the casing in the spec hasn't changed. VmManagedIdentity 2023-11-01-preview: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/servicefabricmanagedclusters/resource-manager/Microsoft.ServiceFabric/preview/2023-11-01-preview/nodetype.json But the type references have been updated appropriately in the code. |
/azp run |
Commenter does not have sufficient privileges for PR 28169 in repo Azure/azure-powershell |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@@ -19,6 +19,9 @@ | |||
--> | |||
## Upcoming Release | |||
|
|||
## Version 3.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 21 and 22. Don't bump the version in ChangeLog. It'll be done automatically in the release.
Hi @iliu816, regarding the properties detected by CI as changed, if these properties only differ in case, please suppress them. |
Hi @YanaXu, do you have an example of how suppression works for the powershell repo CI? A search for "suppress" in the repo isn't returning documentation for me, and I clicked through a few modules and couldn't find examples. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Build failed. Please fix it. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description
Service fabric managed clusters specification was migrated to Typespec in March 2025 (Azure/azure-rest-api-specs#32790).
Complications with SDK generation and release delayed work on Powershell cmdlets. This PR brings the autorest generated SDK in-line with the latest servicefabricmanagedclusters spec (Azure/azure-rest-api-specs#35603) as a basis for further development work. There have been no service side changes for the existing cmdlets.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.