-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[Az.ConfidentialLedger] Update generation tool version: autorest.powers… #28218
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
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
… jinpei/main/ConfidentialLedger
| ### CheckViaJsonFilePath | ||
| ``` | ||
| Test-AzConfidentialLedgerNameAvailability -JsonFilePath <String> [-SubscriptionId <String>] | ||
| [-DefaultProfile <PSObject>] [-Confirm] [-WhatIf] [<CommonParameters>] |
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.
What is the schema of the JSON file?
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.
Typically this type of structure would need to be provided by the service team and we cannot speculate on this.
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.
Typically, the default syntax of a command sends the request body that is formatted to be the value of this parameter.
|
|
||
| ### CheckViaJsonString | ||
| ``` | ||
| Test-AzConfidentialLedgerNameAvailability -JsonString <String> [-SubscriptionId <String>] |
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.
What is the schema of the Json string? Is there any guidance on it?
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.
Typically this type of structure would need to be provided by the service team and we cannot speculate on this.
|
To the author of the pull request, |
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 Azure ConfidentialLedger module generation tool from AutoRest PowerShell v3 to v4. The primary purpose is to modernize the code generation toolchain for the ConfidentialLedger module.
Key changes include:
- Updated AutoRest PowerShell generation tool from v3 to v4
- Simplified type definitions and parameter structures throughout the module
- Updated Az.Accounts dependency version requirement
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tools/StaticAnalysis/Exceptions/Az.ConfidentialLedger/BreakingChangeIssues.csv |
Added breaking change exceptions for type changes introduced by the tool upgrade |
src/ConfidentialLedger/ConfidentialLedger/Az.ConfidentialLedger.psd1 |
Updated Az.Accounts dependency version and generation date |
src/ConfidentialLedger/ConfidentialLedger.sln |
Updated project GUID reference for generated project |
src/ConfidentialLedger/ConfidentialLedger.Autorest/generate-info.json |
Updated generation ID for new tool version |
src/ConfidentialLedger/ConfidentialLedger.Autorest/README.md |
Simplified configuration removing v3-specific settings and breaking change directives |
| Help files and PowerShell scripts | Updated type references and documentation to reflect simplified type system |
|
|
||
| [Parameter()] | ||
| [ArgumentCompleter([Microsoft.Azure.PowerShell.Cmdlets.ConfidentialLedger.Support.LedgerType])] | ||
| [Microsoft.Azure.PowerShell.Cmdlets.ConfidentialLedger.PSArgumentCompleterAttribute("Unknown", "Public", "Private")] |
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.
Just curious. Why are enum changed to strings? E.g. LedgerType and LedgerRoleName
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.
This is a tool-level modification, and nothing will change for the user in terms of how and what they enter.
| ## OUTPUTS | ||
| ### Microsoft.Azure.PowerShell.Cmdlets.ConfidentialLedger.Models.Api20220513.IConfidentialLedger | ||
| ### Microsoft.Azure.PowerShell.Cmdlets.ConfidentialLedger.Models.IConfidentialLedger |
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.
What is the reason for moving the models outside the Api version namespace? In my opinion, it could cause issues in the future as we wouldn't be able to make changes to the model without breaking the existing users.
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.
This is a change triggered by upgrading the autorest tool.
In the latest version of the tool, we have changed the rules for generating class namespaces, we have removed the intermediate level directory called version number.
This change does not have any impact on the actual usage by users.
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.
Without version, what is the recommended way to introduce new changes in the SDK? For instance, we could change the IConfidentialLedger interface in a future version. How do we accomplish it without versioning?
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.
We can confirm and change the Swagger API version at the location specified in this link.
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.
Left a few comments. please review
|
@lijinpei2008 While the changes look good to me, we'll hold on merging until early Nov. At the meantime, could you help authorize a markdown like this for each of the breaking change? We'll need it for publishing migration guides. Just leave a comment in the issue would be fine. Thank you! ## Az.Accounts
### `Get-AzAccessToken`
The default output type is changed from `PSAccessToken` to `PSSecureAccessToken`.That is to change plaintext `PSAccessToken.Token` to `SecureString PSSecureAccessToken.Token`
#### Before
```powershell
$authHeader = @{
'Content-Type' = 'application/json'
'Authorization' = 'Bearer ' + (Get-AccessToken).Token
}
$response = Invoke-RestMethod -Method Get -Headers $authHeader -Uri $uri
```
#### After
```powershell
$secureToken = (Get-AzAccessToken).Token
$ssPtr = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($secureToken)
try {
$plaintextToken = [System.Runtime.InteropServices.Marshal]::PtrToStringBSTR($ssPtr)
}
finally {
[System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($ssPtr)
}
$authHeader = @{
'Content-Type' = 'application/json'
'Authorization' = 'Bearer ' + $plaintextToken
}
$response = Invoke-RestMethod -Method Get -Headers $authHeader -Uri $uri
``` |
… jinpei/main/ConfidentialLedger
…hell v3->v4
Description
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.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.