xExchMaintenanceMode: Fix maintenance mode issues #272
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #272 +/- ##
========================================
+ Coverage 0.95% 0.95% +<.01%
========================================
Files 36 36
Lines 3996 3991 -5
========================================
Hits 38 38
+ Misses 3958 3953 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @mhendric)
DSCResources/MSFT_xExchMaintenanceMode/MSFT_xExchMaintenanceMode.psm1, line 129 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/MSFT_xExchMaintenanceMode.psm1, line 286 at r1 (raw file):
"pauseClusterNode"
We should use single quotes where applicable. Throughout (see two more below)
DSCResources/MSFT_xExchMaintenanceMode/MSFT_xExchMaintenanceMode.psm1, line 1098 at r1 (raw file):
$DomainController, [Parameter()]
We should make these parameters mandatory , or set these to the default values as well?
DSCResources/MSFT_xExchMaintenanceMode/MSFT_xExchMaintenanceMode.psm1, line 1460 at r1 (raw file):
$DomainController, [Parameter()]
We should make these parameters mandatory , or set these to the default values as well?
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 44 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 109 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 290 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 390 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 438 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 735 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 767 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 943 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 1109 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 1131 at r1 (raw file):
$PSCmdlet.ShouldProcess
This should be change to Write-Verbose instead of ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 1349 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 1410 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 2526 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 2545 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 2567 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 2583 at r1 (raw file):
$PSCmdlet.ShouldProcess
This should be change to Write-Verbose instead of ShouldProcess.
DSCResources/MSFT_xExchMaintenanceMode/TransportMaintenance.psm1, line 2676 at r1 (raw file):
SupportsShouldProcess=$True
We should not use this, as DSC cannot use ShouldProcess.
johlju
added
waiting for code fix
|
Thanks for the review, I'll get this updated. Regarding the DomainController parameters, I don't think those should be mandatory. The consumer has the option of specifying a DomainController when using the resource, but it's absolutely not required. |
|
Update pushed. Note that for all the previously added SupportsShouldProcess sections, I just reverted to the original code, and then added a suppress statement for that rule. Hopefully that is acceptable. |
mhendric
added
needs review
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mhendric)
DSCResources/MSFT_xExchMaintenanceMode/MSFT_xExchMaintenanceMode.psm1, line 129 at r2 (raw file):
function Set-TargetResource { [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSUseShouldProcessForStateChangingFunctions", "")]
Curious. There should be no need for a suppress statement, since the test framework ignores that rule, see https://github.com/PowerShell/DscResources/blob/master/PSSARuleSeverities.md#ignore.
Where are you seeing this being reported by script analyzer?
|
I'm seeing that when manually running PSScriptAnalyzer (1.17.1) offline on my dev machine. |
|
@mhendric ah, I'm using VS Code and it is not enabled there so I'm not seeing this. I personally think it would be better to exclude the rule when running |
|
@johlju , do you think I should remove these new suppress statements then, or should I just not worry about adding them in the future? |
|
You are the maintainer of this resource module, so in the end you decide :) I just thought there are unnecessary code to add. You don't have to remove them. Please look at PR PowerShell/DscResources#424, it adds a settings file that can be used to |
|
Ok, I'm going to leave these suppression statements in, but will avoid unnecessary ones in the future. I'll make sure to run Invoke-ScriptAnalyzer with the proper ignore switches going forward. |
Pull Request (PR) description
Fixes multiple issues in xExchMaintenanceMode, including those described in #209. Also fixes almost all issues identified by the PS Script Analyzer.
This Pull Request (PR) fixes the following issues
Fixes #209
Task list
should say what was changed, and how that affects users (if applicable).
and comment-based help?
See DSC Resource Testing Guidelines.
See DSC Resource Testing Guidelines.
DSC Resource Style Guidelines
and Best Practices?
Integration test results
Exchange 2016 CU10 on Windows 2016 (PS5)
Exchange 2013 CU21 on Windows 2012 R2 (PS4)
This change is