-
Notifications
You must be signed in to change notification settings - Fork 378
Added M365 connections to shared #2090
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
lusassl-msft
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are handling it properly, does it matter if we do the try/catch version?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, let’s move to the one module approach then 👍
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.
@iserrano76 are you able to make this change?
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.
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.
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 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). |
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