-
Notifications
You must be signed in to change notification settings - Fork 343
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
FreeBusyChecker Final Version #2264
base: main
Are you sure you want to change the base?
Conversation
New PR for FreeBusyChecker script due to difficulty in squashing previous commits |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Old PR #1934 |
|
||
$Script:ExchangeOnlineDomain = ($Script:UserOnline -split "@")[1] | ||
|
||
if ($Script:ExchangeOnlineDomain -like "*.mail.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.
What about other cloud? Is the script expected to work on these (e.g., GCC, Gallatin…)?
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.
They were not considered at start, I had no reference for this clouds and no way to test.
We can include.
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.
Not sure if this is 100% required before we release. It would be nice yes, but I would rather get something out there and us create an issue for this and address it soon.
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.
It's okay to address this with a later update. We should mention in the documentation that the script works with WW cloud only.
$Script:IntraOrgCon = Get-IntraOrganizationConnector -WarningAction SilentlyContinue -ErrorAction SilentlyContinue | Select-Object Name, TarGetAddressDomains, DiscoveryEndpoint, Enabled | ||
ShowParameters | ||
CheckParameters | ||
if ($Script:IntraOrgCon.enabled -like "True") { |
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.
Isn’t the object type bool here? If it is -eq $true would be better
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 ($Script:IntraOrgCon.enabled -like "True") { | ||
$Auth = hostOutputIntraOrgConEnabled($Auth) | ||
} | ||
if ($Script:IntraOrgCon.enabled -like "False") { |
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.
Same here
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 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.
Can you check what the return type of $Script:IntraOrgCon.enabled
is?
$Script:IntraOrgCon.enabled.GetType()
If it's Boolean
the check should be like this:
$Script:IntraOrgCon.enabled -eq $true
or $Script:IntraOrgCon.enabled -eq $false
Write-Host -ForegroundColor Green " Checking Exchange Online Configuration" | ||
Write-Host " Testing Connection to Exchange Online with EO Prefix." | ||
try { | ||
$CheckExoMailbox = get-EOMailbox $Script:UserOnline -ErrorAction Stop |
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.
Should be Get-EOMailbox (Pascal Case) as we do with the other cmdlets.
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.
} | ||
function FetchEWSInformation { | ||
if (-not $Script:WebServicesVirtualDirectory -or -not $Script:WebServicesVirtualDirectoryOAuth) { | ||
$Script:WebServicesVirtualDirectory = Get-WebServicesVirtualDirectory -Server $Script:Server | Select-Object Identity, Name, ExchangeVersion, *Authentication*, *url -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.
These Get-
calls to query the virtual directory information are very time consuming as we query IIS metabase. Can we check if the required information are available in AD tool (by using -ADPropertiesOnly
switch parameter). This would improve the execution time of the script. Not all information are stored in AD but we should at least confirm for the use-case of the script.
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.
Thank you, great point! Will do.
$MissingParameters += "Exchange Online Domain. Example: contoso.mail.onmicrosoft.com" | ||
} | ||
if ([string]::IsNullOrWhiteSpace($Script:ExchangeOnPremLocalDomain)) { | ||
$MissingParameters += "Exchange On Premises Local Domain. Example: . 'C:\scripts\FreeBusyChecker\FreeBusyChecker.ps1' -OnPremisesUser [email protected]" |
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.
It would be better to do something like this for the example as this will reflect the name of the script (even if it was renamed for whatever reason):
.\$($script:MyInvocation.MyCommand.Name) -OnPremisesUser [email protected]
Same for the following examples (line 51
, 54
and 57
)
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.
Will $($script:MyInvocation.MyCommand.Name)
return the script name here as we are in a different function.
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.
As far as I remember it should return the name of the script even if $($script:MyInvocation.MyCommand.Name)
is part of a function / method within the main script.
``` | ||
Imports and Installs the following Modules (if not available): | ||
|
||
PSSnapin: microsoft.exchange.management.powershell.snapin |
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 don't think that we should ask customers to load the PSSnapin
this way. You can use the shared Confirm-ExchangeShell
function to check and import it in a supported way.
|
||
Example Screen Output: | ||
|
||
![image](./image1.png) |
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.
Make sure to redact the valid tenant information (e.g., DomainNames). We are not allowed to use non-CELA approved domains in our documentations.
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.
Good catch, this needs to be done and squashed to avoid a commit history showing the incorrect names.
|
||
Example TXT Output: | ||
|
||
![image](./image2.png) |
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.
Make sure to redact the valid tenant information (e.g., DomainNames). We are not allowed to use non-CELA approved domains in our documentations.
|
||
Example HTML Output | ||
|
||
![image](./image3.png) |
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.
Make sure to redact the valid tenant information (e.g., DomainNames). We are not allowed to use non-CELA approved domains in our documentations.
$Script:WebServicesVirtualDirectoryOAuth = $Script:WebServicesVirtualDirectory | ||
} | ||
} | ||
function CheckIfExchangeServer { |
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'd recommend using Confirm-ExchangeShell
here (shared function).
[Parameter(Mandatory = $false, ParameterSetName = "Test")] | ||
[string]$OnPremLocalDomain | ||
) | ||
begin { |
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.
Can we add GenericScriptUpdate.ps1
here? This adds update capabilities to the script and allows the script to perform an auto-update whenever a new version is released.
@iserrano76 would you be interested of being a co-owner of this script? Similar to what you do with the MDO script. |
Issue:
Free/Busy cases are a challenge as they involve many moving parts and configurations in hybrid topology.
Reason:
Free/Busy cases are very time-consuming and demanding due to the complexity of verifying different settings across both Exchange On-Premises and Exchange Online.
Fix:
Validation: