Conversation
lusassl-msft
left a comment
There was a problem hiding this comment.
Requesting changes as there are a few things that should be changed (see comments).
dpaulson45
left a comment
There was a problem hiding this comment.
Please address all open comments then let me know when this is ready for the next review.
dpaulson45
left a comment
There was a problem hiding this comment.
There might be some more things that I find later on, but here is a good starting point to address.
|
We should also look into adding pester testing here as well. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dpaulson45
left a comment
There was a problem hiding this comment.
Almost ready to go, after we get the next commit, let's also rebase this down to a single commit.
Shared/ModuleHandle.ps1
Outdated
| [Parameter(Mandatory = $false)] | ||
| [System.Version]$MinModuleVersion = $null |
There was a problem hiding this comment.
I guess we did address this by being in a loop, but the chances of you having a module that you are requesting be at the same min version is pretty low.
Shared/ModuleHandle.ps1
Outdated
| } | ||
| $installed = Get-InstalledModule @getParams | ||
|
|
||
| if ($null -eq $installed -or $installed.Name -notcontains $module) { |
There was a problem hiding this comment.
If we did find the module with the version that we are looking for, we should do Write-Verbose here that states at least the version to know if it is something newer and possibly something changed with the cmdlets that broke something.
There was a problem hiding this comment.
@iserrano76 let's do this and rebase and then we should be good for one last review before merge.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@iserrano76 let's go ahead and rebase and squash this down to a single commit. |
dbfc7cf to
8507a98
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Pending review from Lukas as well. |
Shared/ModuleHandle.ps1
Outdated
| [Parameter(Mandatory = $false)] | ||
| [System.Version]$MinModuleVersion = $null |
There was a problem hiding this comment.
We could also think about removing the MinModuleVersion parameter and instead passing a key-value pair via Module parameter. This could be a dictionary (because they are strongly typed):
$modules = New-Object "System.Collections.Generic.Dictionary[String, Version]"
$modules.Add("ModuleToLoad", "3.0.0.0")
We could then progress them one by one
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@lusassl-msft to review the final result, then we will squash this back down a commit and then merge. |
Shared/ModuleHandle.ps1
Outdated
| Write-Verbose "Checking $m PowerShell Module" | ||
| $getParams = @{ | ||
| Name = $m | ||
| ErrorAction = 'SilentlyContinue' |
There was a problem hiding this comment.
@iserrano76 should we change this to ErrorAction = Stop and put the Get-InstalledModule into a try/catch? If we fail to query the modules for whatever reason, we treat it the same as if the module was not found. I think we should change this behavior.
There was a problem hiding this comment.
If we are handling it properly, does it matter if we do the try/catch version?
There was a problem hiding this comment.
No, it shouldn't matter then. If I understand the code correctly, we're trying to install the module if $installed is $null or does not contain $m (module x). Installed would be $null in case that Get-InstalledModule fail for whatever reason. This would mean we're trying installing a module which might be already there. That's my understanding here. Please correct me if I'm wrong.
Shared/ModuleHandle.ps1
Outdated
| [Parameter(Mandatory = $false)] | ||
| [System.Version]$MinModuleVersion = $null |
There was a problem hiding this comment.
I'm okay with both approaches. If you want to keep the current approach I'm okay with it.
Shared/ModuleHandle.ps1
Outdated
|
|
||
| $noFoundError = $true | ||
|
|
||
| foreach ($m in $Module) { |
There was a problem hiding this comment.
@iserrano76 Sorry to be so petty. Wouldn't it be better to return a final statement for each module that was passed when calling the Request-Module function? We support processing multiple modules in here but return just one state. If one of the modules fail and the remaining ones are processed successfully, we just return $false because one of them has failed. I think it would be better to put the module and final state to a hashtable and finally return it. What do you think?
There was a problem hiding this comment.
Hi @lusassl-msft do not worry, it is a healthy discussion looking for the best solution, no problem at all 😉
Maybe it is easier to simplify and just allow one module each time instead of an array and return true or false for each one.
I think we are not going to request several modules.
What do you think?
There was a problem hiding this comment.
That could make it more complicated for the caller to use. If that is the case, I would just adjust to only accept a single module and you have to test against each one.
There was a problem hiding this comment.
Yep, let’s move to the one module approach then 👍
There was a problem hiding this comment.
Yes, I am trying to find some time for testing.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@lukak-msft please review as soon as you can. @iserrano76 please squash the commits down to 1 so we can clean this up and make it ready to merge once we are done reviewing. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@iserrano76 please squash this down to a single commit, then we will review and merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds standardized Microsoft 365 connection functions to the Shared module, providing unified ways to connect to Exchange Online and Microsoft Graph. The implementation includes module validation, installation prompts, and connection management with proper error handling.
- Adds
Request-Modulefunction for PowerShell module management (install/verify) - Implements
Connect-EXOAdvancedfor Exchange Online connections with multi-session support - Implements
Connect-GraphAdvancedfor Microsoft Graph connections with scope validation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Shared/ModuleHandle.ps1 | Provides module installation and verification functionality |
| Shared/M365/EXOConnection.ps1 | Exchange Online connection management with session handling |
| Shared/M365/GraphConnection.ps1 | Microsoft Graph connection management with scope validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@iserrano76 please squash this down to a single commit, then we will review and merge. |
4ae5322 to
43d2e80
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@iserrano76 please squash this down to a single commit, then we will review and merge. |
Issue:
Added two functions to Shared in case any script needs a connection to M365.
One function for Exchange Online another function for AzureAD
Reason:
Unify the way we connect to M365.
Fix:
Both functions will do the following:
Important: we do not disconnect at anytime we just verify if we have an active session