-
Notifications
You must be signed in to change notification settings - Fork 4k
[HDInsight] Update api version to 2025-01-15-preview #27837
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
…ayEntraUserInfo ; New-AzHDInsightClusterGatewayEntraUserInfo modify cmd: New-AzHDInsightCluster
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 HDInsight API version to 2025-01-15-preview and adapts the cmdlets, SDK, and tests accordingly. Key changes include:
- Parameter updates in cmdlets to support the new RestAuthEntraUsers input and making HttpCredential optional.
- Updates to test session records and PowerShell scripts to align with the new API version and associated behavior.
- Modifications in the SDK and documentation to reference the updated API endpoints.
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/HDInsight/HDInsight/ManagementCommands/SetAzureHDInsightGatewaySettingsCommand.cs | Adjusted parameter handling and error conditions to support either HttpCredential or RestAuthEntraUsers. |
src/HDInsight/HDInsight/ManagementCommands/NewAzureHDInsightClusterGatewayEntraUserInfoCommand.cs | Added a new cmdlet to create Entra user info objects for gateway access. |
src/HDInsight/HDInsight/ManagementCommands/NewAzureHDInsightClusterCommand.cs | Made HttpCredential optional and added the RestAuthEntraUsers parameter when invoking the cluster creation helper. |
src/HDInsight/HDInsight/ManagementCommands/GetAzureHDInsightClusterGatewayEntraUserInfoCommand.cs | Updated parameter sets to correctly resolve input by name, resource ID, or input object. |
src/HDInsight/HDInsight/Constants.cs | Added a new constant key for EntraUsers under GatewayConfigurations. |
src/HDInsight/HDInsight/ClusterCreateHelper.cs | Updated the method signature to accept RestAuthEntraUsers and added related parameter validation; left some commented-out legacy code. |
Various test and PSD1 files | Updated API version endpoints, cmdlet exports, and test script parameters to reflect the new API version and updated command behavior. |
@@ -310,6 +310,9 @@ public HDInsightManagementClient(System.Uri baseUri, Microsoft.Rest.ServiceClien | |||
/// <param name='rootHandler'> | |||
/// Optional. The http client handler used to handle http transport. | |||
/// </param> | |||
/// <param name='handlers'> |
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.
Is this generated? why is there a new added handlers param?
@@ -310,6 +310,9 @@ public HDInsightManagementClient(System.Uri baseUri, Microsoft.Rest.ServiceClien | |||
/// <param name='rootHandler'> | |||
/// Optional. The http client handler used to handle http transport. | |||
/// </param> | |||
/// <param name='handlers'> |
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.
Is this generated? why is there a new added handlers param?
@@ -8,7 +8,8 @@ namespace Microsoft.Azure.Management.HDInsight.Models | |||
using System.Linq; | |||
|
|||
/// <summary> | |||
/// The update gateway settings request parameters. | |||
/// The update gateway settings request parameters. Note either basic or entra | |||
/// user should be provided at a time. |
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.
the same?
@@ -28,7 +28,7 @@ function Test-AutoscaleRelatedCommands{ | |||
$cluster = New-AzHDInsightCluster -Location $params.location -ResourceGroupName $params.resourceGroupName ` | |||
-ClusterName $params.clusterName -ClusterSizeInNodes $params.clusterSizeInNodes -ClusterType $params.clusterType ` | |||
-StorageAccountResourceId $params.storageAccountResourceId -StorageAccountKey $params.storageAccountKey ` | |||
-HttpCredential $params.httpCredential -SshCredential $params.sshCredential ` | |||
-HttpCredential $params.httpCredential -SshCredential $params.sshCredential -Version "5.1" ` |
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.
if without -Version, what is the default version?
# prepare parameter for creating parameter | ||
# $params= Prepare-ClusterCreateParameter -location $location | ||
|
||
# create cluster that will be used throughout test | ||
$cluster = Get-AzHDInsightCluster -ResourceGroupName yuchen-ps-test -ClusterName spark51 | ||
$cluster = Get-AzHDInsightCluster -ResourceGroupName "wut-tip2-esp" -ClusterName "test5-wut-tip2" |
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.
change this to group-ps-test
# prepare parameter for creating parameter | ||
# $params= Prepare-ClusterCreateParameter -location $location | ||
|
||
# create cluster that will be used throughout test | ||
$cluster = Get-AzHDInsightCluster -ResourceGroupName yuchen-ps-test -ClusterName spark51 | ||
$cluster = Get-AzHDInsightCluster -ResourceGroupName "wut-tip2-esp" -ClusterName "test5-wut-tip2" |
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.
change this
@@ -102,7 +102,7 @@ function Test-AzureMonitorAgentRelatedCommands{ | |||
|
|||
Assert-NotNull $workspaceId | |||
Assert-NotNull $primaryKey | |||
Enable-AzHDInsightAzureMonitorAgent -ClusterName $cluster.Name -ResourceGroup $cluster.ResourceGroup -WorkspaceId $workspaceId -Primary $primaryKey | |||
Enable-AzHDInsightAzureMonitorAgent -ClusterName $cluster.Name -ResourceGroup $cluster.ResourceGroup -WorkspaceId $workspaceId -PrimaryKey $primaryKey |
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.
did you changed this parameter name?
} | ||
finally | ||
{ | ||
# Delete cluster and resource group |
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.
shall you delete the cluster?
finally | ||
{ | ||
# Delete cluster and resource group | ||
# Remove-AzResourceGroup -ResourceGroupName $params.resourceGroupName |
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.
do you cleanup this after all test cases finished?
|
||
|
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.
change to 1 line?
|
||
|
||
|
||
|
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.
reduce the empty line
try{ | ||
$params= Prepare-ClusterCreateParameter | ||
$entraUserFullInfo = @(@{ObjectId = "00000000-0000-0000-0000-000000000000"; Upn = "[email protected]"; DisplayName = "DisplayName" },@{ObjectId = "00000000-0000-0000-0000-000000000000"; Upn = "[email protected]"; DisplayName = "DisplayName" }) | ||
$entraUserIdentity = "test_test.com#EXT#@microsoft.onmicrosoft.com" |
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.
change this value
function Test-CreateEntraCluster{ | ||
try{ | ||
$params= Prepare-ClusterCreateParameter | ||
$entraUserFullInfo = @(@{ObjectId = "00000000-0000-0000-0000-000000000000"; Upn = "[email protected]"; DisplayName = "DisplayName" },@{ObjectId = "00000000-0000-0000-0000-000000000000"; Upn = "[email protected]"; DisplayName = "DisplayName" }) |
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.
change this use like contoso.com
StorageContainer = $params.clusterName | ||
StorageAccountKey = $params.storageAccountKey | ||
StorageAccountResourceId = $params.storageAccountResourceId | ||
# EntraUserFullInfo = $entraUserFullInfo |
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 the unused property definition
$subnetName="default" | ||
|
||
# create cluster | ||
$cluster = New-AzHDInsightCluster -Location $params.location -ResourceGroupName $params.resourceGroupName ` | ||
-ClusterName $params.clusterName -ClusterSizeInNodes $params.clusterSizeInNodes -ClusterType $params.clusterType ` | ||
-ClusterName $params.clusterName -ClusterSizeInNodes $params.clusterSizeInNodes -ClusterType "Spark" ` |
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.
don't hard code this, use the params to pass the cluster type
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.
The same with below
@@ -567,8 +651,8 @@ Test Update cluster System Assigned Identity | |||
function Test-UpdateClusterSystemAssigned{ | |||
try | |||
{ | |||
$rg="yuchen-ps-test" | |||
$clusterName="h1-spark" | |||
$rg="yukundemo1" |
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.
change this value and check similar issue
@@ -68,6 +68,6 @@ function Test-MonitoringRelatedCommands{ | |||
finally | |||
{ | |||
# Delete cluster and resource group | |||
Remove-AzResourceGroup -ResourceGroupName $cluster.ResourceGroup | |||
# Remove-AzResourceGroup -ResourceGroupName $cluster.ResourceGroup |
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.
change this back, and make sure can clean up this rg after tests finished
src/HDInsight/HDInsight/ChangeLog.md
Outdated
- Added parameter `-EntraUserIdentity` to cmdlet `New-AzHDInsightCluster` to support creating Entra user clusters using one or more ObjectId or UPNs. Automatically resolves Entra user information. | ||
- Added parameter `-EntraUserFullInfo` to cmdlet `New-AzHDInsightCluster` to support creating Entra user clusters using an array of hashtables that include ObjectId, UPN, and DisplayName. | ||
* Added two parameters `-EntraUserIdentity` and `-EntraUserFullInfo` to cmdlet `Set-AzHDInsightGatewayCredential` | ||
- Added parameter `-EntraUserIdentity` to cmdlet `New-AzHDInsightCluster` to support update Entra user clusters using one or more ObjectId or UPNs. Automatically resolves Entra user information. |
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.
wrong cmdlet name here
src/HDInsight/HDInsight/ChangeLog.md
Outdated
- Added parameter `-EntraUserFullInfo` to cmdlet `New-AzHDInsightCluster` to support creating Entra user clusters using an array of hashtables that include ObjectId, UPN, and DisplayName. | ||
* Added two parameters `-EntraUserIdentity` and `-EntraUserFullInfo` to cmdlet `Set-AzHDInsightGatewayCredential` | ||
- Added parameter `-EntraUserIdentity` to cmdlet `New-AzHDInsightCluster` to support update Entra user clusters using one or more ObjectId or UPNs. Automatically resolves Entra user information. | ||
- Added parameter `-EntraUserFullInfo` to cmdlet `New-AzHDInsightCluster` to support update Entra user clusters using an array of hashtables that include ObjectId, UPN, and DisplayName. |
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.
change this
var dict = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
foreach (DictionaryEntry entry in user) | ||
{ | ||
dict[entry.Key.ToString() ?? ""] = entry.Value.ToString() ?? ""; |
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.
refactor this, when the entry.key.ToString() will be null?
foreach (var userDict in userDicts) | ||
{ | ||
string objectId = userDict.TryGetValue("ObjectId", out var oid) ? oid : null; | ||
string upn = userDict.TryGetValue("Upn", out var u) ? u : 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.
so there you required the key is fixed value, you shall write doc for this requirement when define the entrauserfullinfo
{ | ||
return restAuthEntraUsers; | ||
} | ||
if(!string.IsNullOrWhiteSpace(EntraUserIdentity) && (EntraUserFullInfo != null && EntraUserFullInfo.Length > 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.
don't do such check in this util file
public static List<EntraUserInfo> GetHDInsightGatewayEntraUser(string EntraUserIdentity, Hashtable[] EntraUserFullInfo) | ||
{ | ||
List<EntraUserInfo> restAuthEntraUsers = new List<EntraUserInfo>(); | ||
if (String.IsNullOrEmpty(EntraUserIdentity) && (EntraUserFullInfo == null || EntraUserFullInfo.Length == 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.
why allowing both empty or null here and return empty list?
public string EntraUserIdentity { get; set; } | ||
|
||
[Parameter( | ||
HelpMessage = "Gets or sets a list of Entra users as an array of hashtables. Each hashtable should contain keys such as ObjectId, UPN, and DisplayName.")] |
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.
here it is UPN but in the Util file you tryGet("Upn")
@@ -17,15 +17,15 @@ Adds a cluster identity to a cluster configuration object. | |||
``` | |||
Add-AzHDInsightClusterIdentity [-Config] <AzureHDInsightConfig> [-ObjectId] <Guid> | |||
[-CertificateFilePath] <String> [-CertificatePassword] <String> [[-AadTenantId] <Guid>] | |||
[[-ApplicationId] <Guid>] [-DefaultProfile <IAzureContextContainer>] | |||
[[-ApplicationId] <Guid>] [-DefaultProfile <IAzureContextContainer>] [-ProgressAction <ActionPreference>] | |||
[<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.
did you change this command?
[<CommonParameters>] | ||
``` | ||
|
||
### CertificateFileContents | ||
``` | ||
Add-AzHDInsightClusterIdentity [-Config] <AzureHDInsightConfig> [-ObjectId] <Guid> | ||
[-CertificateFileContents] <Byte[]> [-CertificatePassword] <String> [[-AadTenantId] <Guid>] | ||
[[-ApplicationId] <Guid>] [-DefaultProfile <IAzureContextContainer>] | ||
[[-ApplicationId] <Guid>] [-DefaultProfile <IAzureContextContainer>] [-ProgressAction <ActionPreference>] |
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.
did you change this?
@@ -208,6 +208,21 @@ Accept pipeline input: True (ByValue) | |||
Accept wildcard characters: False | |||
``` | |||
|
|||
### -ProgressAction |
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.
don't need to change this command right?
@@ -15,7 +15,7 @@ Adds a version for a service running in a cluster to a cluster configuration obj | |||
|
|||
``` | |||
Add-AzHDInsightComponentVersion [-Config] <AzureHDInsightConfig> [-ComponentName] <String> | |||
[-ComponentVersion] <String> [-DefaultProfile <IAzureContextContainer>] | |||
[-ComponentVersion] <String> [-DefaultProfile <IAzureContextContainer>] [-ProgressAction <ActionPreference>] |
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.
check all the commands which we shall not change
Accept pipeline input: False | ||
Accept wildcard characters: False | ||
``` | ||
|
||
### -Version | ||
Specifies the HDI version of the HDInsight cluster. |
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.
Add more examples for this command
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.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.