Enganga/bulk url update #1439
Open
Enganga/bulk url update #1439
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Learn Build status updates of commit a8c91af: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 4f5bec0: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 5e69f15: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
emmanuel-karanja
force-pushed
the
enganga/bulk-url-update
branch
from
5e69f15 to
0ece451
Compare
|
Learn Build status updates of commit 0ece451: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
emmanuel-karanja
force-pushed
the
enganga/bulk-url-update
branch
from
0ece451 to
bfa26d5
Compare
|
Learn Build status updates of commit bfa26d5: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 7034cc2: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 66f535f: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 32bcb8e: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
SamErde
reviewed
Apr 3, 2025
module/Entra/Microsoft.Entra/Applications/Add-EntraServicePrincipalOwner.ps1
Outdated
Show resolved
Hide resolved
SamErde
reviewed
Apr 3, 2025
module/Entra/Microsoft.Entra/Applications/Get-EntraApplicationTemplate.ps1
Show resolved
Hide resolved
SamErde
reviewed
Apr 3, 2025
module/Entra/Microsoft.Entra/DirectoryManagement/Get-EntraPartnerInformation.ps1
Outdated
Show resolved
Hide resolved
|
Learn Build status updates of commit eb932c2: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit aefad9a: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit dc805e5: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 9283270: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit b30bc9d: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit 354e134: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
|
Learn Build status updates of commit e746fdb: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
SamErde
suggested changes
Apr 7, 2025
There was a problem hiding this comment.
Looks good! Requested changes are only "nit-picks" and requests for clarification.
- Pascal case, per project guidelines
- Consistent spacing
- Question about use of
$BaseUrivs$RootUri
And a thought for future iterations: is there any potential benefit to creating a reusable function for defining the BaseUri dynamically based on what Graph resource type is being queried / CRUD?
Comment on lines
25
to
26
| $environment = (Get-EntraContext).Environment | ||
| $rootUri = (Get-EntraEnvironment -Name $environment).GraphEndpoint |
There was a problem hiding this comment.
How do you feel about replacing these two lines with one to reduce the number of variables being created? The alternate cost is that it's not quite as easy to read at first glance.
$RootUri = (Get-MgEnvironment -Name (Get-MgContext).Environment).GraphEndpoint
Comment on lines
25
to
26
| $environment = (Get-EntraContext).Environment | ||
| $rootUri = (Get-EntraEnvironment -Name $environment).GraphEndpoint |
There was a problem hiding this comment.
I encourage the same design guidelines that are documented in this project, including the use of Pascal case for names of functions, parameters, etc. It might be a nit-pick, but consistency is very helpful in a project. :)
| @@ -16,7 +16,7 @@ function Get-EntraScopedRoleMembership { | |||
| $params = @{} | |||
| $customHeaders = New-EntraCustomHeaders -Command $MyInvocation.MyCommand | |||
| $isList = $false | |||
| $baseUri = "https://graph.microsoft.com/v1.0/directory/administrativeUnits" | |||
| $baseUri = "/v1.0/directory/administrativeUnits" | |||
There was a problem hiding this comment.
Nit-pick / observation: many of these could be in single quotes when variables do not need to be expanded within the string. Change not required, but it can be a helpful practice to adopt.
| @@ -59,7 +61,7 @@ function Add-EntraGroupOwner { | |||
| } | |||
| if ($null -ne $PSBoundParameters["OwnerId"]) { | |||
| $TmpValue = $PSBoundParameters["OwnerId"] | |||
| $Value = @{ "@odata.id" = "https://graph.microsoft.com/beta/users/$TmpValue" } | |||
| $Value = @{ "@odata.id" = "$rootUri/beta/users/$TmpValue" } | |||
There was a problem hiding this comment.
I just now noticed that some of these changes use $RootUri and some use $BaseUri. Is this an intentional distinction for when utilizing different MgGraph cmdlets?
module/EntraBeta/Microsoft.Entra.Beta/Groups/Add-EntraBetaGroupOwner.ps1
Outdated
Show resolved
Hide resolved
module/EntraBeta/Microsoft.Entra.Beta/NetworkAccess/New-EntraBetaPrivateAccessApplication.ps1
Show resolved
Hide resolved
module/EntraBeta/Microsoft.Entra.Beta/SignIns/Add-EntraBetaServicePrincipalPolicy.ps1
Show resolved
Hide resolved
module/EntraBeta/Microsoft.Entra.Beta/Users/Set-EntraBetaUserManager.ps1
Outdated
Show resolved
Hide resolved
module/EntraBeta/Microsoft.Entra.Beta/Users/Set-EntraBetaUserSponsor.ps1
Outdated
Show resolved
Hide resolved
|
Learn Build status updates of commit 3d6d252: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
|
Learn Build status updates of commit 4e9bdf0: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
|
Learn Build status updates of commit 68569f6: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
|
Learn Build status updates of commit 701650a: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
SamErde
reviewed
Apr 15, 2025
| @@ -66,7 +66,8 @@ function Set-EntraBetaPartnerInformation { | |||
| $params.Keys | ForEach-Object { "$_ : $($params[$_])" } | Write-Debug | |||
| Write-Debug("=========================================================================`n") | |||
| if ([string]::IsNullOrWhiteSpace($TenantId)) { | |||
| $TenantID = ((Invoke-MgGraphRequest -Method GET -Uri "/beta/organization").value).id | |||
|
|
|||
There was a problem hiding this comment.
Looks fine, just noting the extra line that got added here. :)
SamErde
reviewed
Apr 15, 2025
| Write-Debug("============================ TRANSFORMATIONS ============================") | ||
| $params.Keys | ForEach-Object { "$_ : $($params[$_])" } | Write-Debug | ||
| Write-Debug("=========================================================================`n") | ||
| if ([string]::IsNullOrWhiteSpace($TenantId)) { |
There was a problem hiding this comment.
Would it be simpler to mark the TenantId parameter as Mandatory and add the [ValidateNotNullOrEmpty()] check in the parameter definition? You could also use a ValidateScript to check for whitespace and throw an error.
KenitoInc
requested changes
Apr 16, 2025
| @@ -22,8 +22,10 @@ function Add-EntraApplicationOwner { | |||
|
|
|||
| $newOwner = @{} | |||
|
|
|||
| $rootUri = (Get-EntraEnvironment -Name (Get-EntraContext).Environment).GraphEndpoint | |||
|
|
|||
There was a problem hiding this comment.
Can you add a default Graph EndPoint similar to this
There was a problem hiding this comment.
Fix that in all cmdlets with odataid
| @@ -13,7 +13,7 @@ function Remove-EntraBetaDeletedDirectoryObject { | |||
| Write-Debug("============================ TRANSFORMATIONS ============================") | |||
| $params.Keys | ForEach-Object {"$_ : $($params[$_])" } | Write-Debug | |||
| Write-Debug("=========================================================================`n") | |||
| $URI = "https://graph.microsoft.com/v1.0/directory/deletedItems/$Id" | |||
| $URI = "/v1.0/directory/deletedItems/$Id" | |||
There was a problem hiding this comment.
Suggested change
| $URI = "/v1.0/directory/deletedItems/$Id" | |
| $URI = "/beta/directory/deletedItems/$Id" |
| @@ -35,7 +35,7 @@ function Add-EntraBetaDirectoryRoleMember { | |||
| } | |||
| if ($null -ne $PSBoundParameters["MemberId"]) { | |||
| $TmpValue = $PSBoundParameters["MemberId"] | |||
| $Value = "https://graph.microsoft.com/v1.0/directoryObjects/$TmpValue" | |||
| $Value = "/v1.0/directoryObjects/$TmpValue" | |||
There was a problem hiding this comment.
Suggested change
| $Value = "/v1.0/directoryObjects/$TmpValue" | |
| $Value = "/beta/directoryObjects/$TmpValue" |
| @@ -33,7 +33,7 @@ function Add-EntraBetaFeatureRolloutPolicyDirectoryObject { | |||
| } | |||
| if ($null -ne $PSBoundParameters["RefObjectId"]) { | |||
| $TmpValue = $PSBoundParameters["RefObjectId"] | |||
| $Value = "https://graph.microsoft.com/v1.0/directoryObjects/$TmpValue" | |||
| $Value = "/v1.0/directoryObjects/$TmpValue" | |||
There was a problem hiding this comment.
Suggested change
| $Value = "/v1.0/directoryObjects/$TmpValue" | |
| $Value = "/beta/directoryObjects/$TmpValue" |
| @@ -226,7 +226,7 @@ function New-EntraBetaUser { | |||
| $params.Keys | ForEach-Object { "$_ : $($params[$_])" } | Write-Debug | |||
| Write-Debug("=========================================================================`n") | |||
| $params = $params | ConvertTo-Json | |||
| $response = Invoke-GraphRequest -Headers $customHeaders -Uri 'https://graph.microsoft.com/v1.0/users?$select=*' -Method POST -Body $params | |||
| $response = Invoke-GraphRequest -Headers $customHeaders -Uri '/v1.0/users?$select=*' -Method POST -Body $params | |||
There was a problem hiding this comment.
Suggested change
| $response = Invoke-GraphRequest -Headers $customHeaders -Uri '/v1.0/users?$select=*' -Method POST -Body $params | |
| $response = Invoke-GraphRequest -Headers $customHeaders -Uri '/beta/users?$select=*' -Method POST -Body $params |
| @@ -35,7 +37,7 @@ function Set-EntraBetaUserManager { | |||
| } | |||
| if ($null -ne $PSBoundParameters["ManagerId"]) { | |||
| $TmpValue = $PSBoundParameters["ManagerId"] | |||
| $Value = @{ "@odata.id" = "https://graph.microsoft.com/v1.0/users/$TmpValue" } | |||
| $Value = @{ "@odata.id" = "$rootUri/v1.0/users/$TmpValue" } | |||
There was a problem hiding this comment.
Suggested change
| $Value = @{ "@odata.id" = "$rootUri/v1.0/users/$TmpValue" } | |
| $Value = @{ "@odata.id" = "$rootUri/beta/users/$TmpValue" } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bulk Update of root url (https://graph.microsoft.*) for both GA and Beta to pick it from the Context.