-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
{HDInsight}Hdi migrate Microsoft.Azure.Graph to MicrosoftGraph #17219
Conversation
403571c
to
669d80f
Compare
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.
Please put the following content in the help document of NewAzureHDInsightClusterCommand:
The cmdlet may call below Microsoft Graph API according to input parameters:
GET /servicePrincipals/{id}
src/HDInsight/HDInsight/ChangeLog.md
Outdated
@@ -18,6 +18,7 @@ | |||
- Additional information about change #1 | |||
--> | |||
## Upcoming Release | |||
This release migrates Microsoft.Azure.Graph SDK to MicrsoftGraph SDK, there is not customer impact. |
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 migration might have impact on customers because the permission required to call AAD graph and MS graph APIs are different.
This release migrates Microsoft.Azure.Graph SDK to MicrsoftGraph SDK, there is not customer impact. | |
This release migrates Microsoft.Azure.Graph SDK to MicrsoftGraph SDK. |
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.
Fixed
try | ||
{ | ||
sp = graphClient.ServicePrincipals.Get(ObjectId.ToString()); | ||
sp = graphClient.ServicePrincipals.GetServicePrincipalWithHttpMessagesAsync(ObjectId.ToString()).Result.Body;//graphClient.ServicePrincipals.Get(ObjectId.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.
You can use this extension method to simplify code: https://github.com/Azure/azure-powershell-common/blob/main/src/Graph.Rbac/MicrosoftGraph/Version1_0/Applications/ServicePrincipalsOperationsExtensions.cs#L210
sp = graphClient.ServicePrincipals.GetServicePrincipalWithHttpMessagesAsync(ObjectId.ToString()).Result.Body;//graphClient.ServicePrincipals.Get(ObjectId.ToString()); | |
sp = graphClient.ServicePrincipals.GetServicePrincipal(ObjectId.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.
Changed
{ | ||
string errorMessage = e.Message + ". Please specify Application Id explicitly by providing ApplicationId parameter and retry."; | ||
throw new Microsoft.Azure.Graph.RBAC.Version1_6.Models.GraphErrorException(errorMessage); | ||
throw new Exception(errorMessage); |
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.
Please use AzPSArgumentException so we can get detailed telemetry data.
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.
Updated
…powershell into HdiMigrate2MSGraph
Hi @isra-fel Thanks for your review, I have updated the PR. |
Added |
@@ -100,6 +100,10 @@ New-AzHDInsightCluster [-Location] <String> [-ResourceGroupName] <String> [-Clus | |||
## DESCRIPTION | |||
The New-AzHDInsightCluster creates an Azure HDInsight cluster by using the specified parameters or by using a configuration object that is created by using the New-AzHDInsightClusterConfig cmdlet. | |||
|
|||
The cmdlet may call below Microsoft Graph API according to input parameters: | |||
|
|||
- GET /servicePrincipals/{id} |
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.
Added in the help doc
/azp run azure-powershell - security-tools |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Migrate Microsoft.Azure.Graph to MicrosoftGraph
Checklist
CONTRIBUTING.md
ChangeLog.md
file(s) has been updated:ChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header -- no new version header should be added