xActiveDirectory: Fix Script Analyzer Rule Failures #303

X-Guardian · 2019-05-29T21:25:53Z

Pull Request (PR) description

This PR fixes all the current Script Analyzer rule failures that are occurring during the AppVeyor build and opts in to the following tests for future builds:

Common Tests - Custom Script Analyzer Rules
Common Tests - Required Script Analyzer Rules
Common Tests - Flagged Script Analyzer Rules

A summary of the changes that have been made are as follows:

xADDomainDefaultPasswordPolicy
- Custom Script Analyzer rule fix
xADDomainTrust
- Required Script Analyzer rule fix
- Flagged Script Analyzer rule fix
- Custom Script Analyzer rule fix
- Move message localization to external file
- Suppress PSDSCUseVerboseMessageInDSCResource due to verbose messages being in helper functions
xADOrganizationalUnit
- Custom Script Analyzer rule fix
xADObjectPermissionEntry
- Flagged Script Analyzer rule fix (Further verbose message improvement and localization in PR xADObjectPermissionEntry: Fix failure when applied in the same configuration as xADDomain #299)
xADRecycleBin
- Flagged Script Analyzer rule fix
- Custom Script Analyzer rule fix
- Message localization
xADReplicationSite
- Flagged Script Analyzer rule fix
- Message localization
xADReplicationSubnet
- Flagged Script Analyzer rule fix
- Message localization
xADServicePrincipalName
- Flagged Script Analyzer rule fix
- Message localization
xWaitForADDomain
- Flagged Script Analyzer rule fix
- Custom Script Analyzer rule fix
- Message localization

New tests have also been added for the following resources:

xADRecycleBin

This Pull Request (PR) fixes the following issues

None

Task list

Added an entry under the Unreleased section in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

- Script Analyzer fix - Message localization - Add missing verbose messages

- Message localization - Add missing verbose messages

- Script Analyzer fix - Message localization - Add missing verbose messages

Merge Dev

codecov-io · 2019-05-29T22:02:37Z

Codecov Report

Merging #303 into dev will increase coverage by 2%.
The diff coverage is 76%.

@@         Coverage Diff          @@
##            dev   #303    +/-   ##
====================================
+ Coverage    90%    92%    +2%     
====================================
  Files        20     20            
  Lines      2241   2299    +58     
  Branches     10     10            
====================================
+ Hits       2029   2135   +106     
+ Misses      202    154    -48     
  Partials     10     10

- Move message localization to external file - Suppress PSDSCUseVerboseMessageInDSCResource - Script Analyzer fix

- Fix Should Throw message

X-Guardian · 2019-05-31T12:39:57Z

Hi @johlju, I can't improve the codecov/patch percentage anymore, as the main culprit is the xADDomainTrust resource, which uses object method calls which can't currently be mocked.

Can you add this to your list of PRs to review? Thanks.

johlju

Thank you for all this work! 😃 A few review comments.

Reviewed 9 of 15 files at r3, 9 of 9 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @X-Guardian)

CHANGELOG.md, line 54 at r5 (raw file):

Quoted 5 lines of code…

- Changes to xActiveDirectory
  - Fix Script Analyzer rule failures
  - Opt-in to "Common Tests - Custom Script Analyzer Rules"
  - Opt-in to "Common Tests - Required Script Analyzer Rules"
  - Opt-in to "Common Tests - Flagged Script Analyzer Rules"

Let's move this entries to the Changes to xActiveDirectory at the top.

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 101 at r5 (raw file):

[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSDSCUseVerboseMessageInDSCResource", "")]

Instead of this, please add at least one Write-Verbose message in this function. I rather we add verbose messages than overriding this. If we want to pass the common test then please add at least one verbose message.

Throughout where this override exist. Sorry, I know it will be a bit of additional work, but it helps debugging in the future.

I'm okay by removing the overrides and opt-out from that particular common test if there are more places that has this override.

I rather see verbose logging is improved than overridden. 🙂

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 133 at r5 (raw file):

[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSDSCUseVerboseMessageInDSCResource", "")]

See previous comment.

johlju · 2019-05-31T14:48:12Z

Awesome improvement on the coverage percentage! 😃

Merge from Dev

X-Guardian

Reviewable status: 0 of 18 files reviewed, 3 unresolved discussions (waiting on @johlju)

CHANGELOG.md, line 54 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

- Changes to xActiveDirectory
  - Fix Script Analyzer rule failures
  - Opt-in to "Common Tests - Custom Script Analyzer Rules"
  - Opt-in to "Common Tests - Required Script Analyzer Rules"
  - Opt-in to "Common Tests - Flagged Script Analyzer Rules"

Let's move this entries to the Changes to xActiveDirectory at the top.

Done.

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 101 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSDSCUseVerboseMessageInDSCResource", "")]
Instead of this, please add at least one Write-Verbose message in this function. I rather we add verbose messages than overriding this. If we want to pass the common test then please add at least one verbose message.

Throughout where this override exist. Sorry, I know it will be a bit of additional work, but it helps debugging in the future.

I'm okay by removing the overrides and opt-out from that particular common test if there are more places that has this override.

I rather see verbose logging is improved than overridden. 🙂

Are you sure? There is plenty of verbose logging in the Confirm-ResourceProperties helper function which is all that this function calls. Adding another verbose message here is then just noise. I can add a justification attribute saying 'Verbose messaging in helper function' if that would help.

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 133 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSDSCUseVerboseMessageInDSCResource", "")]

See previous comment.

Same response as previous comment.

johlju

Reviewed 18 of 19 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @X-Guardian)

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 101 at r5 (raw file):

Previously, X-Guardian (Simon Heather) wrote…

Are you sure? There is plenty of verbose logging in the Confirm-ResourceProperties helper function which is all that this function calls. Adding another verbose message here is then just noise. I can add a justification attribute saying 'Verbose messaging in helper function' if that would help.

Ah okay! Did not realizes that. Yes, please add a comment to the override. Thanks for questioning my comment! 🙂

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 133 at r5 (raw file):

Previously, X-Guardian (Simon Heather) wrote…

Same response as previous comment.

Ah okay! Did not realizes that. Yes, please add a comment to the override. Thanks for questioning my comment! 🙂

X-Guardian

Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 101 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah okay! Did not realizes that. Yes, please add a comment to the override. Thanks for questioning my comment! 🙂

Done.

DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1, line 133 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah okay! Did not realizes that. Yes, please add a comment to the override. Thanks for questioning my comment! 🙂

Done.

johlju

Reviewed 1 of 1 files at r8, 1 of 1 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved

johlju · 2019-06-01T09:31:51Z

Thank you for this @X-Guardian! 🙇

X-Guardian and others added 13 commits May 29, 2019 21:53

xADDomainDefaultPasswordPolicy Script Analyzer fix

db3bb29

xADDomainTrust Script Analyzer fix

f20abd4

xADOrganizationalUnit Script Analyzer fix

ab4e1c0

xADRecycleBin Changes:

5664a85

- Script Analyzer fix - Message localization - Add missing verbose messages

xADReplicationSite changes:

9e63ce9

- Message localization - Add missing verbose messages

xADReplicationSubnet changes:

8d06728

- Message localization - Add missing verbose messages

xADServicePrincipalName changes:

35200c8

- Message localization - Add missing verbose messages

xWaitForADDomain changes:

c7985f4

- Script Analyzer fix - Message localization - Add missing verbose messages

Add Script Analyzer opt-ins

830852a

Update CHANGELOG

23407c1

Merge pull request #1 from PowerShell/dev

23b4a07

Merge Dev

Merge Dev branch and fix conflicts

1f4d94a

Fix xWaitForADDomain resource name

c682221

X-Guardian added 8 commits May 29, 2019 23:14

xADOrganizationalUnit Script Analyzer fix

72af166

xADRecycleBin Script Analyzer fix

639d2ff

xADDomainTrust Script Analyzer fix

2171ca7

xADReplicationSubnet fix Write-Verbose parameter

45946b7

xWaitForADDomain Script Analyzer fixes

ddb6343

xADDomainTrust changes:

74cb154

- Move message localization to external file - Suppress PSDSCUseVerboseMessageInDSCResource - Script Analyzer fix

xADServicePrincipalName.Tests changes:

dce7ef0

- Fix Should Throw message

xADObjectPermissionEntry Add Verbose Messages

cf4d78d

X-Guardian marked this pull request as ready for review May 29, 2019 23:16

X-Guardian added 2 commits May 30, 2019 22:32

New xADRecycleBin tests

9688e1b

Fix xADRecycleBin LocalizedData resource name

53d7533

Merge branch 'dev' into ScriptAnalyzerRules-Fix

3f05254

johlju added the needs review The pull request needs a code review. label May 31, 2019

johlju requested changes May 31, 2019

View reviewed changes

johlju removed the needs review The pull request needs a code review. label May 31, 2019

johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label May 31, 2019

X-Guardian and others added 3 commits May 31, 2019 16:45

Merge pull request #2 from PowerShell/dev

f7de60d

Merge from Dev

Merge from Dev

925abe0

Move entries in CHANGELOG

54fe08f

X-Guardian commented May 31, 2019

View reviewed changes

johlju requested changes Jun 1, 2019

View reviewed changes

X-Guardian and others added 2 commits June 1, 2019 09:56

Add SuppressMessageAttribute justification

74ed799

Merge branch 'dev' into ScriptAnalyzerRules-Fix

14e9235

X-Guardian commented Jun 1, 2019

View reviewed changes

X-Guardian mentioned this pull request Jun 1, 2019

xActiveDirectory: Opt-In to DSC Resource Common Meta Tests #306

Merged

9 tasks

johlju approved these changes Jun 1, 2019

View reviewed changes

johlju merged commit d8d71e1 into dsccommunity:dev Jun 1, 2019

johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jun 1, 2019

X-Guardian deleted the ScriptAnalyzerRules-Fix branch June 1, 2019 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

xActiveDirectory: Fix Script Analyzer Rule Failures #303

xActiveDirectory: Fix Script Analyzer Rule Failures #303

Uh oh!

X-Guardian commented May 29, 2019 •

edited

Loading

Uh oh!

codecov-io commented May 29, 2019 •

edited

Loading

Uh oh!

X-Guardian commented May 31, 2019

Uh oh!

johlju left a comment

Uh oh!

johlju commented May 31, 2019

Uh oh!

X-Guardian left a comment

Uh oh!

johlju left a comment

Uh oh!

X-Guardian left a comment

Uh oh!

johlju left a comment

Uh oh!

johlju commented Jun 1, 2019

Uh oh!

Uh oh!

xActiveDirectory: Fix Script Analyzer Rule Failures #303

xActiveDirectory: Fix Script Analyzer Rule Failures #303

Uh oh!

Conversation

X-Guardian commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

Uh oh!

codecov-io commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

X-Guardian commented May 31, 2019

Uh oh!

johlju left a comment

Choose a reason for hiding this comment

Uh oh!

johlju commented May 31, 2019

Uh oh!

X-Guardian left a comment

Choose a reason for hiding this comment

Uh oh!

johlju left a comment

Choose a reason for hiding this comment

Uh oh!

X-Guardian left a comment

Choose a reason for hiding this comment

Uh oh!

johlju left a comment

Choose a reason for hiding this comment

Uh oh!

johlju commented Jun 1, 2019

Uh oh!

Uh oh!

X-Guardian commented May 29, 2019 •

edited

Loading

codecov-io commented May 29, 2019 •

edited

Loading