xExchInstall: add support of installing Exchange CUs as well. #320
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 20 unresolved discussions (waiting on @gborus)
.github/PULL_REQUEST_TEMPLATE.md, line 17 at r1 (raw file):
Adding support of Exchange CU installation to xExchInstall resource. Added new function Get-ExchangeDisplayVersion to xExchangeHelper.psm1. Modified Get-ExchangeInstallStatus function in xExchangeHelper.psm1 in order to check for CU install / update scenario.
You should not be modifying the pull request template. This template is what gets displayed to everyone when they open a new pull request.
.github/PULL_REQUEST_TEMPLATE.md, line 20 at r1 (raw file):
#### This Pull Request (PR) fixes the following issues None.
You should not be modifying the pull request template. This template is what gets displayed to everyone when they open a new pull request.
.github/PULL_REQUEST_TEMPLATE.md, line 31 at r1 (raw file):
For those task that don't apply to you PR, leave those as is. --> - [x] Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
You should not be modifying the pull request template. This template is what gets displayed to everyone when they open a new pull request.
DSCResources/MSFT_xExchInstall/MSFT_xExchInstall.psm1, line 157 at r1 (raw file):
Write-FunctionEntry -Parameters @{'Path' = $Path; 'Arguments' = $Arguments} -Verbose:$VerbosePreference $installStatus = Get-ExchangeInstallStatus -Path $Path -Arguments $Arguments -Verbose:$VerbosePreference
There are 10+ other calls to Get-ExchangeInstallStatus across 'DSCResources\MSFT_xExchInstall\MSFT_xExchInstall.psm1', 'Tests\Unit\MSFT_xExchInstall.tests.ps1', and 'Tests\Unit\xExchangeHelper.tests.ps1'. These will need to be updated to utilize the new Path parameter.
Modules/xExchangeHelper.psm1, line 229 at r1 (raw file):
<# .SYNOPSIS Gets the installed Exchange buildnumber, which refers to the installed updates / CU,
Function comment based help should be properly indented per https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions-have-comment-based-help. Make sure each line of the help isn't longer than 80 characters.
Modules/xExchangeHelper.psm1, line 243 at r1 (raw file):
function Get-ExchangeDisplayVersion { [CmdletBinding()]
Need proper indentation through the function. See: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#indentation
Modules/xExchangeHelper.psm1, line 279 at r1 (raw file):
if($null -ne $displayVersionString) { $displayVersionString -match '(?<VersionMajor>\d+).(?<VersionMinor>\d+).(?<VersionBuild>\d+)'
May want to move this and the next code block into a common function that returns a hashtable with the version elements given a version display string. Since you are doing the same thing here, and later in Get-ExchangeInstallStatus.
Modules/xExchangeHelper.psm1, line 280 at r1 (raw file):
{ $displayVersionString -match '(?<VersionMajor>\d+).(?<VersionMinor>\d+).(?<VersionBuild>\d+)' if($Matches)
Add a blank line before this if block
Modules/xExchangeHelper.psm1, line 284 at r1 (raw file):
$displayVersion = @{ VersionMajor = [int]$Matches.VersionMajor
Only use a single space on each side of the = for these variables. Use either Int (capitable I) or the case correct full name for [int] (i.e. [System.Int32]. Add a space between [System.Int32] and the $Matches variable (see https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#one-space-between-type-and-variable-name)
Modules/xExchangeHelper.psm1, line 292 at r1 (raw file):
else { Write-Error "Get-ExchangeDisplayVersion: Major, Minor, Update versions cannot be parsed."
Seems like this should throw an exception if requested, since we couldn't determine the version.
Use -Message parameter. See the comment about Named parameters here: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-parameter-usage-in-function-and-cmdlet-calls.
Also use single quotes instead of double
Modules/xExchangeHelper.psm1, line 298 at r1 (raw file):
else { Write-Error "Get-ExchangeDisplayVersion function could not read the 'DisplayVersion' of Exchange from registry."
Seems like this should throw an exception if requested, since we couldn't determine the version.
Use -Message parameter. See the comment about Named parameters here: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-parameter-usage-in-function-and-cmdlet-calls
Modules/xExchangeHelper.psm1, line 444 at r1 (raw file):
# Exchange CU install / update support if(($Arguments -match "/mode:upgrade") -or ($Arguments -match "/m:upgrade"))
Use single quotes for these strings (https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-use-of-single--and-double-quotes)
Modules/xExchangeHelper.psm1, line 446 at r1 (raw file):
if(($Arguments -match "/mode:upgrade") -or ($Arguments -match "/m:upgrade")) { # get Exchange setup.exe version
Capitalize first letter of comment
Modules/xExchangeHelper.psm1, line 447 at r1 (raw file):
{ # get Exchange setup.exe version $setupexeVersionString = (Get-ChildItem -Path $Path).VersionInfo.ProductVersionRaw
You may want to do a Test-Path -Path $Path first, and throw an exception if the path isn't valid. You also
Modules/xExchangeHelper.psm1, line 450 at r1 (raw file):
Write-Verbose "Setup.exe version is: '$setupexeVersionString'" $setupexeVersionString -match '(?<VersionMajor>\d+).(?<VersionMinor>\d+).(?<VersionBuild>\d+)'
May want to move this and the next code block into a common function that returns a hashtable with the version elements given a version display string.
Modules/xExchangeHelper.psm1, line 465 at r1 (raw file):
if($null -ne $exchangeVersion) { # if we have an exchange installed
Capitalize first comment letter
Modules/xExchangeHelper.psm1, line 467 at r1 (raw file):
{ # if we have an exchange installed Write-Verbose "Comparing setup.exe version and installed Exchange's version."
Use -Message parameter. See the comment about Named parameters here: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-parameter-usage-in-function-and-cmdlet-calls
Modules/xExchangeHelper.psm1, line 473 at r1 (raw file):
if(($exchangeDisplayVersion.VersionMajor -eq $setupExeVersion.VersionMajor)` -and ($exchangeDisplayVersion.VersionMinor -eq $setupExeVersion.VersionMinor)` -and ($exchangeDisplayVersion.VersionBuild -le $setupExeVersion.VersionBuild) ) # if server has lower version of CU installed
Capitalize first comment letter
Modules/xExchangeHelper.psm1, line 475 at r1 (raw file):
-and ($exchangeDisplayVersion.VersionBuild -le $setupExeVersion.VersionBuild) ) # if server has lower version of CU installed { Write-Verbose "Version upgrader is requested."
Should use single quotes here. Also use -Message parameter. See the comment about Named parameters here: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-parameter-usage-in-function-and-cmdlet-calls
Modules/xExchangeHelper.psm1, line 483 at r1 (raw file):
else { Write-Error "Get-ExchangeInstallStatus: Script cannot determin installed Exchange's version. Please check if Exchange is installed."
Use -Message parameter. Also this has a misspelling for determin(e)
johlju
added
the
waiting for code fix
|
Hi @gborus , I wanted to add a couple more comments. First, we'll probably need to get new Unit tests created for any new function that is being added as part of this, and if possible, any function that is being changed. Specifically, we'll need new Unit tests added in 'MSFT_xExchInstall.tests.ps1' and 'xExchangeHelper.tests.ps1'. Ping me if you need help getting started on that. Next, we should probably move the code which gets the Exchange uninstall key into a dedicated function, since Get-ExchangeVersion already does this operation as well. That will help prevent duplication of code. |
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 21 unresolved discussions (waiting on @gborus)
Modules/xExchangeHelper.psm1, line 425 at r1 (raw file):
$exchangePresent = Test-ExchangePresent····
Remove added whitespace from end of line
|
On get-exchangeversion I would add a -returnversionhash switch or something
to tell the function to return with the hash instead of string version.
Do you like this?
Mike Hendrickson <notifications@github.com> ezt írta (időpont: 2018. szept.
25., K 16:45):
Hi @gborus <https://github.com/gborus> , I wanted to add a couple more
comments. First, we'll probably need to get new Unit tests created for any
new function that is being added as part of this, and if possible, any
function that is being changed. Specifically, we'll need new Unit tests
added in 'MSFT_xExchInstall.tests.ps1' and 'xExchangeHelper.tests.ps1'.
Ping me if you need help getting started on that.
Next, we should probably move the code which gets the Exchange uninstall
key into a dedicated function, since Get-ExchangeVersion already does this
operation as well. That will help prevent duplication of code.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMfLZvm2esYvtJxj09PThx3B44Tiyks5uekGPgaJpZM4W3hW8>
.
2018. szept. 25. 16:45 ezt írta ("Mike Hendrickson" <
notifications@github.com>):
Hi @gborus <https://github.com/gborus> , I wanted to add a couple more
comments. First, we'll probably need to get new Unit tests created for any
new function that is being added as part of this, and if possible, any
function that is being changed. Specifically, we'll need new Unit tests
added in 'MSFT_xExchInstall.tests.ps1' and 'xExchangeHelper.tests.ps1'.
Ping me if you need help getting started on that.
Next, we should probably move the code which gets the Exchange uninstall
key into a dedicated function, since Get-ExchangeVersion already does this
operation as well. That will help prevent duplication of code.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMfLZvm2esYvtJxj09PThx3B44Tiyks5uekGPgaJpZM4W3hW8>
.
|
|
I think it may still be best to have separate functions for Get-ExchangeVersion and Get-ExchangeDisplayVersion. It may make testing difficult to have a single function that can return multiple types, and it may also be confusing to whoever decides to use the function. Perhaps we can rename these both to be more indicative of what they do, as in Get-ExchangeVersion -> Get-ExchangeVersionYear, and Get-ExchangeDisplayVersion -> Get-ExchangeVersionFull. Note that we'd have to rename all function calls to Get-ExchangeVersion as well throughout the module (VSCode makes it very easy to do bulk find/replace across files). As for getting the uninstall key, I think that could be moved to a new function, Get-ExchangeUninstallKey. Then update related code in Get-ExchangeVersionYear and Get-ExchangeVersionFull to get the key via that function. That should simplify testing of things as well. |
|
Ok that is ok with me.
Can you please point me to the right direction for the ps Unit tests?
Thanks
G.
Mike Hendrickson <notifications@github.com> ezt írta (időpont: 2018. szept.
25., K 19:38):
… I think it may still be best to have separate functions for
Get-ExchangeVersion and Get-ExchangeDisplayVersion. It may make testing
difficult to have a single function that can return multiple types, and it
may also be confusing to whoever decides to use the function. Perhaps we
can rename these both to be more indicative of what they do, as in
Get-ExchangeVersion -> Get-ExchangeVersionYear, and
Get-ExchangeDisplayVersion -> Get-ExchangeVersionFull. Note that we'd have
to rename all function calls to Get-ExchangeVersion as well throughout the
module (VSCode makes it very easy to do bulk find/replace across files).
As for getting the uninstall key, I think that could be moved to a new
function, Get-ExchangeUninstallKey. Then update related code in
Get-ExchangeVersionYear and Get-ExchangeVersionFull to get the key via that
function. That should simplify testing of things as well.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMSnskXx3kSKPCT8GtFpt0GPdaOtyks5uemojgaJpZM4W3hW8>
.
|
|
Something else I noticed.
I don't think this regex is necessary. $file.VersionInfo.ProductVersionRaw already returns the split version, as in: |
|
As for Unit testing, start by taking a look at what's already in 'MSFT_xExchInstall.tests.ps1' and 'xExchangeHelper.tests.ps1'. Essentially we'll need to create a new Describe block for each new function being added, or function being modified. Then we'll need to do a new Context block and It block for each test type. I recommend reading up on Pester here: https://github.com/pester/Pester. Before modifying any tests, make sure you can execute the current tests successfully as is. I.e. go to xExchange\Tests\Unit, run 'Invoke-Pester', and make sure everything passes. Get-ExchangeInstallStatus already has a Describe block in xExchangeHelper.tests.ps1, so we'll just need to make sure that gets updated so that all new code in that function is hit and tested. To do that you'll need to add new tests which test various combinations of VersionMajor, VersionMinor, and VersionBuild, to make sure it behaves as expected. You'll need to do this by Mock'ing whatever function returns your version. So either Mock Get-ChildItem to return a custom object with a VersionInfo.ProductVersionRaw property. Or create a specific function to get a file version given a path, and then Mock that function (I think that's probably a more testable approach). You would confirm that all the new code is actually covered using: Invoke-Pester -Path Tests\Unit\xExchangeHelper.tests.ps1 -CodeCoverage Modules\xExchangeHelper.psm1 |
|
Just to level set, there's a bunch of related common functions that don't have Unit tests yet, but that would be good to have Unit tests before we check in this change. If you want, I can get the initial set of Unit tests added for all the existing common setup code, and then you can update the tests with your newly added code. That should greatly reduce the amount of Unit tests you need to write. Does that work for you? If so, I can probably get these new tests written and checked in within a week or so (if not sooner). The common functions I'm thinking we need new Unit tests for (now; eventually we need all functions to have Unit tests): Test-ExchangePresent Within xExchInstallStatus, I would add Unit tests for Set-TargetResource. |
|
Any help would be good i am not familiar nem with ps unit tests, but happy
to learn that!
Mike Hendrickson <notifications@github.com> ezt írta (időpont: 2018. szept.
25., K 20:19):
… Just to level set, there's a bunch of related common functions that don't
have Unit tests yet, but that would be good to have Unit tests before we
check in this change. If you want, I can get the initial set of Unit tests
added for all the existing common setup code, and then you can update the
tests with your newly added code. That should greatly reduce the amount of
Unit tests you need to write. Does that work for you? If so, I can probably
get these new tests written and checked in within a week or so (if not
sooner).
The common functions I'm thinking we need new Unit tests for (now;
eventually we need all functions to have Unit tests):
Test-ExchangePresent
Get-ExchangeVersion
Test-ExchangeSetupComplete
Test-ExchangeSetupPartiallyCompleted
Test-ShouldInstallUMLanguagePack
Test-ExchangeSetupRunning
Within xExchInstallStatus, I would add Unit tests for Set-TargetResource.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMYLXPivRfDV_pmE3xjsJaJCIMd2Iks5uenOmgaJpZM4W3hW8>
.
|
|
Ok, I'll work on getting some tests checked in for common, existing code. After that I'll leave new/updated unit tests to you. I'll ping you as soon as these get checked in, as you'll probably need to do a rebase on your repo. |
|
Oh no, rebase again... :)
Mike Hendrickson <notifications@github.com> ezt írta (időpont: 2018. szept.
25., K 21:07):
… Ok, I'll work on getting some tests checked in for common, existing code.
After that I'll leave new/updated unit tests to you. I'll ping you as soon
as these get checked in, as you'll probably need to do a rebase on your
repo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMUp4F0XbVMpNb_HDcAzT1j6DNmDAks5uen77gaJpZM4W3hW8>
.
|
|
talking about rebase, I started to setup my repo from scratch.
I was following these steps :
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md
however I received an error at this step:
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#making-changes-and-pushing-them-to-the-fork
the problematic command was: git checkout -b
xExchangeInstall_upgrade_support my/dev, the error said it cannot create a
new branch and switch to it.
however the thing is that the list of steps differs from these steps:
https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#fixing-an-issue
in this second one, you create your own fork and you clone that, while the
first one is doing something strange for me, cloning the original repo to
local, then make a fork, then setup the fork as remote
for me git didn't allow anything to do with the fork I added as remote, I'm
not sure why.
how your setup looks like?
thanks
-Gabor.
…On Tue, Sep 25, 2018 at 9:07 PM Mike Hendrickson ***@***.***> wrote:
Ok, I'll work on getting some tests checked in for common, existing code.
After that I'll leave new/updated unit tests to you. I'll ping you as soon
as these get checked in, as you'll probably need to do a rebase on your
repo.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMUp4F0XbVMpNb_HDcAzT1j6DNmDAks5uen77gaJpZM4W3hW8>
.
|
I follow those same steps almost exactly, except I don't add my/dev to the git checkout -b BRANCH command (not sure why... perhaps old instructions didn't call for it). You're welcome to try that, although I don't know if it's the correct way to go about things. Adding @johlju, as he may have an idea why this isn't working for you. |
|
at the end I was doing the same like yourself, I add the branch locally, so
the command without the my/dev, but then I wasn't able to push into my
fork, it always wanted to push to the origin, which was main repo, where I
don't have rights to push.
as a workaround I change the remote origin's url to my forked repo, but
this I believe is the same as I would first just fork and then clone my
fork, which the proposed method here:
https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#fixing-an-issue
I'm just worried about the rebase stuff, as I would follow the steps from
here:
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts
but that relies on the setup describe on that site, and I'm not sure about
the steps then :-D
…On Tue, Sep 25, 2018 at 11:37 PM Mike Hendrickson ***@***.***> wrote:
the problematic command was: git checkout -b
xExchangeInstall_upgrade_support my/dev, the error said it cannot create a
new branch and switch to it.
I follow those same steps almost exactly, except I don't add my/dev to the
git checkout -b BRANCH command (not sure why... perhaps old instructions
didn't call for it). You're welcome to try that, although I don't know if
it's the correct way to go about things. Adding @johlju
<https://github.com/johlju>, as he may have an idea why this isn't
working for you.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMUMRCVUei5fdFmUdJZ_rKtq68THFks5ueqIcgaJpZM4W3hW8>
.
|
|
Using Hope the below give some help. Started by cloning the upstream repo (as per Getting started with GitHub)
Started by cloning the forked repo
Get an existing working-branch in the forked repo
Update: Made a typo in one of the commands. |
|
@gborus if you run I know rebasing can be challenging, I know because I felt that too in the beginning, but when you get a hang of it, and with the help of VS Code, normally it just take (literally) a minute to rebase. 🙂 |
|
Hi there,
her you are the output
c:\GitRepo\xExchange>git remote -v
my https://github.com/gborus/xExchange (fetch)
my https://github.com/gborus/xExchange (push)
origin https://github.com/gborus/xExchange.git (fetch)
origin https://github.com/gborus/xExchange.git (push)
c:\GitRepo\xExchange>git branch -a
dev
* xExchangeInstall_upgrade_support
remotes/origin/HEAD -> origin/dev
remotes/origin/dev
remotes/origin/hotfix1
remotes/origin/master
remotes/origin/revert-197-FixUnlimitedQuotaIssue
remotes/origin/xExchangeInstall_upgrade_support
I did include the current branch list too.
Currently there's no need to rebase, but it seems it will be.
as you can see I modified the origin's remote url to my fork for now, this
was I was able to push the changes to my fork.
I couldn't push to my for some reason, it might be just a gap in my
knowledge about git commands.
thanks
-Gabor.
…On Wed, Sep 26, 2018 at 10:27 AM Johan Ljunggren ***@***.***> wrote:
@gborus <https://github.com/gborus> if you run git remote -v and post the
result here I can help you with the commands to rebase.
I know rebasing can be challenging, I know because I felt that too in the
begnining, but when you get a hang of it, and with the help of VS Code,
normally it just take (literally) a minute to rebase. 🙂
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMZvIC9Qf-nMgDW21juCIyaFI0IRvks5uezqFgaJpZM4W3hW8>
.
|
|
Hi Mike,
I'm doing this part now. I'd name the Get-ExchangeVersionFull rather
to Get-ExchangeVersionDetailed.
Any objections?
thanks
-Gabor.
…On Tue, Sep 25, 2018 at 7:38 PM Mike Hendrickson ***@***.***> wrote:
I think it may still be best to have separate functions for
Get-ExchangeVersion and Get-ExchangeDisplayVersion. It may make testing
difficult to have a single function that can return multiple types, and it
may also be confusing to whoever decides to use the function. Perhaps we
can rename these both to be more indicative of what they do, as in
Get-ExchangeVersion -> Get-ExchangeVersionYear, and
Get-ExchangeDisplayVersion -> Get-ExchangeVersionFull. Note that we'd have
to rename all function calls to Get-ExchangeVersion as well throughout the
module (VSCode makes it very easy to do bulk find/replace across files).
As for getting the uninstall key, I think that could be moved to a new
function, Get-ExchangeUninstallKey. Then update related code in
Get-ExchangeVersionYear and Get-ExchangeVersionFull to get the key via that
function. That should simplify testing of things as well.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfTMSnskXx3kSKPCT8GtFpt0GPdaOtyks5uemojgaJpZM4W3hW8>
.
|
Codecov Report
@@ Coverage Diff @@
## dev #320 +/- ##
========================================
+ Coverage 5.82% 7.02% +1.19%
========================================
Files 38 38
Lines 4448 4500 +52
========================================
+ Hits 259 316 +57
+ Misses 4189 4184 -5
Continue to review full report at Codecov.
|
Hi, I was able to push to my using this command : git push my |
|
Great that you can push to the remote now! There is a conflicting file now - CHANGELOG.md. So to resolve that you need to do the following. First I see that the remote 'my' and 'origin' point to the same URL. So to fix that, let's replace origin to point to the upstream. Then rebase your working branch (using Visual Studio Code). |
|
@gborus This is starting to look much cleaner. There's still some review comments that need to be addressed, but this is a good start. Another change that I think would be good would be to move the entire new ' # Exchange CU install / update support ' block in Get-ExchangeInstallStatus into a dedicated function. Perhaps named: Test-ShouldUpgradeExchange. That will make things easier to test, and keep each function simpler. I'm fine with doing Get-ExchangeVersionFull instead of Get-ExchangeVersionDetailed. |
Hi Mike, I will not create the Get-ExchangeUninstallKey function, I'll use the Get-ExchangeVersionDetailed to feed the Get-ExchangeVersionYear. thanks |
|
Hi Gabor, I think Get-ExchangeUninstallKey is still good for testability reasons. I agree in your assessment that Get-ExchangeUninstallKey should feed Get-ExchangeVersionDetailed which should feed Get-ExchangeVersionYear. I already started taking a crack at this and wrote some Pester tests, but you can run with these if you want: Updated Common Code<#
.SYNOPSIS
Gets the installed Exchange Version, and returns the number as a
string. Returns N/A if the version cannot be found, and will
optionally throw an exception if ThrowIfUnknownVersion was set to
$true.
.PARAMETER ThrowIfUnknownVersion
Whether the function should throw an exception if the version cannot
be found. Defauls to $false.
#>
function Get-ExchangeVersionYear
{
[CmdletBinding()]
[OutputType([System.String])]
param
(
[Parameter()]
[System.Boolean]
$ThrowIfUnknownVersion = $false
)
$version = 'N/A'
$installedVersionDetails = Get-DetailedInstalledVersion
switch ($installedVersionDetails.VersionMajor)
{
15
{
switch ($installedVersionDetails.VersionMinor)
{
0 {$version = '2013'}
1 {$version = '2016'}
2 {$version = '2019'}
}
}
}
if ($version -eq 'N/A' -and $ThrowIfUnknownVersion)
{
throw 'Failed to discover a known Exchange Version'
}
return $version
}
function Get-ExchangeUninstallKey
{
[CmdletBinding()]
[OutputType([Microsoft.Win32.RegistryKey])]
param()
# First try to get the Exchange 2016 / 2019 uninstall key.
$uninstallKey = Get-Item -Path 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{CD981244-E9B8-405A-9026-6AEB9DCEF1F1}' -ErrorAction SilentlyContinue
# If the first key attempt is NULL, this may be a 2013 server. Try the 2013 key.
if ($null -eq $uninstallKey)
{
$uninstallKey = Get-Item -Path 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{4934D1EA-BE46-48B1-8847-F1AF20E892C1}' -ErrorAction SilentlyContinue
}
return $uninstallKey
}
function Get-DetailedInstalledVersion
{
[CmdletBinding()]
[OutputType([System.Management.Automation.PSCustomObject])]
param()
$installedVersionDetails = $null
$uninstallKey = Get-ExchangeUninstallKey
if ($null -ne $uninstallKey)
{
$versionDetails = @{
VersionMajor = Get-ItemProperty -Path $uninstallKey.Name -Name 'VersionMajor' -ErrorAction SilentlyContinue
VersionMinor = Get-ItemProperty -Path $uninstallKey.Name -Name 'VersionMinor' -ErrorAction SilentlyContinue
DisplayVersion = Get-ItemProperty -Path $uninstallKey.Name -Name 'DisplayVersion' -ErrorAction SilentlyContinue
}
$installedVersionDetails = New-Object -TypeName PSCustomObject -Property $versionDetails
}
return $installedVersionDetails
}New corresponding tests Describe 'xExchangeHelper\Get-ExchangeVersionYear' -Tag 'Helper' {
AfterEach {
Assert-VerifiableMock
}
$validProductVersions = @(
@{
VersionMajor = 15
VersionMinor = 0
Year = '2013'
}
@{
VersionMajor = 15
VersionMinor = 1
Year = '2016'
}
@{
VersionMajor = 15
VersionMinor = 2
Year = '2019'
}
)
$invalidProductVersions = @(
@{
VersionMajor = 15
VersionMinor = 7
Year = 'N/A'
}
@{
VersionMajor = 14
VersionMinor = 0
Year = 'N/A'
}
)
Context 'When Get-ExchangeVersionYear is called and finds a valid VersionMajor and VersionMinor' {
It 'Should return the correct Exchange year' -TestCases $validProductVersions {
param
(
[System.Int32]
$VersionMajor,
[System.Int32]
$VersionMinor,
[System.String]
$Year
)
Mock -CommandName Get-DetailedInstalledVersion -Verifiable -MockWith {
return @{
VersionMajor = $VersionMajor
VersionMinor = $VersionMinor
}
}
Get-ExchangeVersionYear | Should -Be $Year
}
}
Context 'When Get-ExchangeVersionYear is called and finds an invalid VersionMajor or VersionMinor without ThrowIfUnknownVersion' {
It 'Should return N/A' -TestCases $invalidProductVersions {
param
(
[System.Int32]
$VersionMajor,
[System.Int32]
$VersionMinor,
[System.String]
$Year
)
Mock -CommandName Get-DetailedInstalledVersion -Verifiable -MockWith {
return @{
VersionMajor = $VersionMajor
VersionMinor = $VersionMinor
}
}
Get-ExchangeVersionYear | Should -Be $Year
}
}
Context 'When Get-ExchangeVersionYear is called and finds an invalid VersionMajor or VersionMinor and ThrowIfUnknownVersion is specified' {
It 'Should throw an exception' -TestCases $invalidProductVersions {
param
(
[System.Int32]
$VersionMajor,
[System.Int32]
$VersionMinor,
[System.String]
$Year
)
Mock -CommandName Get-DetailedInstalledVersion -Verifiable -MockWith {
return @{
VersionMajor = $VersionMajor
VersionMinor = $VersionMinor
}
}
{ Get-ExchangeVersionYear -ThrowIfUnknownVersion $true } | Should -Throw -ExpectedMessage 'Failed to discover a known Exchange Version'
}
}
}
Describe 'xExchangeHelper\Get-ExchangeUninstallKey' -Tag 'Helper' {
AfterEach {
Assert-VerifiableMock
}
Context 'When Get-ExchangeUninstallKey is called and Exchange 2016 or 2019 is installed' {
It 'Should return the 2016/2019 uninstall key' {
Mock -CommandName Get-Item -Verifiable -ParameterFilter {$Path -like 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{CD981244-E9B8-405A-9026-6AEB9DCEF1F1}'} -MockWith { return $true }
Get-ExchangeUninstallKey | Should -Be -Not $Null
}
}
Context 'When Get-ExchangeUninstallKey is called and Exchange 2013 is installed' {
It 'Should return the 2016/2019 uninstall key' {
Mock -CommandName Get-Item -Verifiable -ParameterFilter {$Path -like 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{CD981244-E9B8-405A-9026-6AEB9DCEF1F1}'} -MockWith { return $null }
Mock -CommandName Get-Item -Verifiable -ParameterFilter {$Path -like 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{4934D1EA-BE46-48B1-8847-F1AF20E892C1}'} -MockWith { return $true }
Get-ExchangeUninstallKey | Should -Be -Not $Null
}
}
Context 'When Get-ExchangeUninstallKey is called and no Exchange is installed' {
It 'Should return NULL' {
Mock -CommandName Get-Item -Verifiable -ParameterFilter {$Path -like 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{CD981244-E9B8-405A-9026-6AEB9DCEF1F1}'} -MockWith { return $null }
Mock -CommandName Get-Item -Verifiable -ParameterFilter {$Path -like 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{4934D1EA-BE46-48B1-8847-F1AF20E892C1}'} -MockWith { return $null }
Get-ExchangeUninstallKey | Should -Be $Null
}
}
}
Describe 'xExchangeHelper\Get-DetailedInstalledVersion' -Tag 'Helper' {
AfterEach {
Assert-VerifiableMock
}
Context 'When DetailedInstalledVersion is called and a valid key is returned by Get-ExchangeUninstallKey' {
It 'Should return custom object with VersionMajor and VersionMinor properties' {
Mock -CommandName Get-ExchangeUninstallKey -Verifiable -MockWith { return @{Name = 'SomeKeyName'} }
Mock -CommandName Get-ItemProperty -Verifiable -ParameterFilter {$Name -eq 'VersionMajor'} -MockWith { return 15 }
Mock -CommandName Get-ItemProperty -Verifiable -ParameterFilter {$Name -eq 'VersionMinor'} -MockWith { return 1 }
$installedVersionDetails = Get-DetailedInstalledVersion
$installedVersionDetails.VersionMajor | Should -Be 15
$installedVersionDetails.VersionMinor | Should -Be 1
}
}
Context 'When DetailedInstalledVersion is called and no valid key is returned by Get-ExchangeUninstallKey' {
It 'Should return NULL' {
Mock -CommandName Get-ExchangeUninstallKey -Verifiable -MockWith { return $null }
Get-DetailedInstalledVersion | Should -Be $null
}
}
} |
|
A couple more comments. You do not need to rename instances of Get-ExchangeVersion in TransportMaintenance.psm1. That module appears to have its own version of this function. I'm not sure that we need to have a $ThrowIfUnknownDisplayVersion parameter in Get-ExchangeVersionDetailed. We definitely still need it in Get-ExchangeVersionYear, as there's callers that expect to use that switch. I think we're fine to keep that functionality there alone, unless you can think of a reason to have it in Get-ExchangeVersionDetailed. I also think we need to be careful about throwing anything in Get-ExchangeVersionDetailed, as I think with the way we're inserting this in to the chain, that could get called and fail during a normal Exchange install where no Exchange is present to begin with. I think if anything, just return a null value from this function, and leave it up to the caller to throw if that's a problem. |
|
@gborus just fyi, I have a corresponding pull request over at #321 (waiting code review). This adds full tests for all of the setup functions except the ones you are working on. We'll want to get this one checked in before we check in yours so we can be more sure that the upgrade changes don't break existing functionality. |
@mhendric , k, let me update the files. |
|
@gborus My corresponding pull request has been checked in, so we should be good to go to do yours as soon as it's cleaned up and ready. You'll definitely need to do a rebase, but after that, let me know once you're ready for another code review. Also just a quick rebase tip, I advise making a copy of the relevant xExchange folder prior to attempting the rebase. That way if the rebase goes horribly wrong, you can start over with the pre-rebase copy. |
gborus
force-pushed
the
xExchangeInstall_upgrade_support
branch
from
022bda5 to
c569302
Compare
|
Hi @mhendric , it seems rebase did the trick :) |
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 7 unresolved discussions (waiting on @mhendric and @gborus)
Modules/xExchangeHelper.psm1, line 272 at r13 (raw file):
$DisplayVersion
Local variable name should be in camelCase (start with lowercase letter)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#variables
Modules/xExchangeHelper.psm1, line 274 at r13 (raw file):
$VersionBuild
Local variable name should be in camelCase (start with lowercase letter)
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#variables
Modules/xExchangeHelper.psm1, line 275 at r13 (raw file):
$DisplayVersion
Local variable name should be in camelCase (start with lowercase letter)
Modules/xExchangeHelper.psm1, line 278 at r13 (raw file):
$VersionBuild
Local variable name should be in camelCase (start with lowercase letter)
Modules/xExchangeHelper.psm1, line 284 at r13 (raw file):
$VersionBuild
Local variable name should be in camelCase (start with lowercase letter) (although the Hashtable member VersionBuild should still start with upper case)
Modules/xExchangeHelper.psm1, line 285 at r13 (raw file):
$DisplayVersion
Local variable name should be in camelCase (start with lowercase letter) (although the Hashtable member DisplayVersion should still start with upper case)
Modules/xExchangeHelper.psm1, line 471 at r13 (raw file):
$exchangeDisplayVersion.Major,$exchangeDisplayVersion.Minor, $exchangeDisplayVersion.Build
These should be .VersionMajor, .VersionMinor, .VersionBuild
|
@gborus , still a couple minor changes left, however I did test, and Upgrades are now functional. |
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 8 unresolved discussions (waiting on @mhendric and @gborus)
Modules/xExchangeHelper.psm1, line 276 at r13 (raw file):
if($Matches)
Add empty line between previous line and if block
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1, 10 of 14 files at r3, 1 of 6 files at r6, 1 of 4 files at r9, 3 of 3 files at r13.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @gborus)
Modules/xExchangeHelper.psm1, line 200 at r13 (raw file):
$version = 'N/A'
Is there a reason we return N/A here? Could we return $null instead, which would be the normal to return if something wasn't found?
This was not part of this PR, so okay to leave it as-is.
Modules/xExchangeHelper.psm1, line 212 at r13 (raw file):
switch ($installedVersionDetails.VersionMinor) { 0 {$version = '2013'}
We should have opening and closing braces on separate rows. This line and the two lines below.
Modules/xExchangeHelper.psm1, line 220 at r13 (raw file):
$version -eq 'N/A'
If N/A was changed in previous comment, it should change here too. Please see previous comment.
Modules/xExchangeHelper.psm1, line 276 at r13 (raw file):
if($Matches)
We should have a space between the statement and the parenthesis if (.
Modules/xExchangeHelper.psm1, line 403 at r13 (raw file):
if(Test-Path -Path $Path -ErrorAction SilentlyContinue)
We should have a space between the statement and the parenthesis if (.
Modules/xExchangeHelper.psm1, line 449 at r13 (raw file):
if(($Arguments -notmatch '/mode:upgrade') ...
We should have a space between the statement and the parenthesis if (. Throughout this function.
Modules/xExchangeHelper.psm1, line 470 at r13 (raw file):
-and $null -ne $exchangeDisplayVersion.VersionBuild) { # If we have an exchange installed Write-Verbose -Message "Comparing setup.exe version and installed Exchange's version."
Is it possible to merge the two Write-Verbose together so both strings are written in the same Write-Verbose?
Or move the first Write-Verbose to the beginning of the function?
Modules/xExchangeHelper.psm1, line 538 at r13 (raw file):
if(($shouldInstallLanguagePack ...
We should have a space between the statement and the parenthesis if (.
Modules/xExchangeHelper.psm1, line 2374 at r13 (raw file):
if(-Not (Test-Path ...
We should have a space between the statement and the parenthesis if (.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @gborus)
Modules/xExchangeHelper.psm1, line 200 at r13 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$version = 'N/A'Is there a reason we return
N/Ahere? Could we return$nullinstead, which would be the normal to return if something wasn't found?
This was not part of this PR, so okay to leave it as-is.
There is no particular reason this returns N/A; it's just a random design decision I made 3 years ago :). No callers of this function (other than a single test in xExchangeHelper.tests.ps1) look for or expect N/A, so returning $null would probably be cleaner. @gborus, up to you if you want to change this or not. If you do, make sure to update the Unit test (for function Test-ExchangePresent) that looks for N/A currently.
|
@mhendric , I'm glad to hear that the upgrade is functional :) at least that was my goal :) Guys do you use anything special to spot these formatting issues. If you do, let me know, I can do that as well, which would be quicker. thanks |
I'll let @johlju comment too, but I think the number one thing is to just read through and become familiar with the DSC Style Guidelines (https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md). I'm not sure if there's any scripts or modules right now that can read a script and identify code that isn't following the DSC style guidelines (something like PSScriptAnalyzer but specifically for DSC). I've been thinking that that would be a useful tool though if it did exist. I'd be willing to contribute time to creating that if it's warranted. |
|
@gborus , everything looks good to me. Great job on this! I'll wait for @johlju to review your changes as well, then hopefully we can get this checked in. BTW, if you happen to do any upgrade testing, you may notice the following message with a bunch of 1's. This is due to a bug in my last code check in. I have a fix for this ready, but am waiting to push it up so that you don't have to do yet another rebase. VERBOSE: Waiting up to 111111111111111111111111111111111111111111111111111111111111 seconds before exiting to give time for ExSetup to start |
There was a problem hiding this comment.
All my review comments have been resolved. Leave it to @mhendric to make the final approval.
Reviewed 2 of 2 files at r14.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @gborus)
Modules/xExchangeHelper.psm1, line 212 at r14 (raw file):
0 { $version = '2013' }
We should actually have them like
0
{
$version = '2013'
}But let's leave it as-is for now, unless you want to resolve that. :) Non-blocking.
For now it is only looking at the changes (in for example Reviewable - big purple button in the PR description) and knowing the style guideline. 🙂 The custom PSSA rules that exist today will run for each resource, see for example line 1003 in the test run for example. The custom rules are also reported in VS Code (if the DscResource.Tests has been cloned into the repo folder, which a unit test does automatically when run). |
johlju
added
needs review
|
The community call is on Wednesday, so normally a week after that. But I think it will be released somewhere mid October. |
|
@johlju , great, thanks! |
|
@gborus Realized we are already in mid October, so probably somewhere at the end of October instead. 🙂 |
Pull Request (PR) description
Adding support of Exchange CU installation to xExchInstall resource.
Added new function Get-ExchangeDisplayVersion to xExchangeHelper.psm1.
Modified Get-ExchangeInstallStatus function in xExchangeHelper.psm1 in order to check for CU install / update scenario.
This Pull Request (PR) fixes the following issues
None.
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is