-
Notifications
You must be signed in to change notification settings - Fork 4k
Fixing SQL Import Export issue with Managed Identity. #27965
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 fixes an issue where the SQL administrator password was still required when using Managed Identity for Import/Export operations by making the password parameter optional in the cmdlet and adjusting the adapter logic.
- Skip decryption and assignment of
AdministratorLoginPassword
whenAuthenticationType
is ManagedIdentity - Change
AdministratorLoginPassword
parameter in the cmdlet from mandatory to optional - Update SDK generation commit and add new input files in the README
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Sql/Sql/ImportExport/Service/ImportExportDatabaseAdapter.cs | Conditionally assign AdministratorLoginPassword only for non-managed identities |
src/Sql/Sql/ImportExport/Cmdlet/ImportExportCmdletArmBase.cs | Made AdministratorLoginPassword parameter optional |
src/Sql/Sql.Management.Sdk/README.md | Bumped commit and added new input-file entries |
Comments suppressed due to low confidence (3)
src/Sql/Sql/ImportExport/Cmdlet/ImportExportCmdletArmBase.cs:83
- Introduce distinct parameter sets or conditional parameter binding so that AdministratorLoginPassword remains mandatory for SQL Authentication scenarios and only optional for ManagedIdentity, preventing runtime confusion.
[Parameter(Mandatory = false, HelpMessage = "The Azure SQL Server administrator password")]
src/Sql/Sql/ImportExport/Cmdlet/ImportExportCmdletArmBase.cs:83
- After changing the parameter to optional, regenerate the cmdlet help markdown files so that the documentation reflects the new optional status of AdministratorLoginPassword.
[Parameter(Mandatory = false, HelpMessage = "The Azure SQL Server administrator password")]
src/Sql/Sql/ImportExport/Service/ImportExportDatabaseAdapter.cs:84
- Add unit tests covering the ManagedIdentity code path in both Import and Export methods to verify that AdministratorLoginPassword is not required and is correctly skipped.
if (exportRequest.AuthenticationType != AuthenticationType.ManagedIdentity)
@@ -59,7 +59,7 @@ public AzureSqlDatabaseImportExportBaseModel Export(AzureSqlDatabaseImportExport | |||
ExportDatabaseDefinition parameters = new ExportDatabaseDefinition() | |||
{ | |||
AdministratorLogin = exportRequest.AdministratorLogin, | |||
AdministratorLoginPassword = AzureSqlServerAdapter.Decrypt(exportRequest.AdministratorLoginPassword), | |||
AdministratorLoginPassword = null, |
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] Consider removing the initial null assignment in the object initializer and only setting AdministratorLoginPassword inside the conditional block to reduce redundancy.
AdministratorLoginPassword = null, |
Copilot uses AI. Check for mistakes.
@debalinaroy Please fix the test failure and static analysis issue detected. When updating generated SDK, please check in all the updated files and avoid uploading those that are related to your code or API. That will definitely cause analysis failure. |
In addition, please add release notes in the ChangeLog.md under the Upcoming Release section. |
Description
There exists an issue with Import Export using Managed Identity where it asks for SQL Authentication Password even though it is not required. This PR fixes the issue by making the password parameter from the Import Export cmdlet optional
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.