Some improvements - Githubissues

Rapidhands commented 4 years ago

Hi, Great job. but, could I suggest some omprovements to do 1) It will be better if all functions have block-comment (synopsis, parameters, examples, ...) Edit : Ok, you have some external help in .md files (probably generated automatically with PlatyPS module :-) ) but not complete. No example of use.

2) Better displayed code : avoid Oneliner command (cmdlet1 |cmdlet2 |cmdlet3) Prefer this Cmdlet1 | Cmdlet2 | Cmdlet3 Easy to realize this with VSCode

3) Pass PSScriptAnalyzer module on the code to respect Best-Practices.

4) Put all functions used in module in differents .ps1 files : Easier to read, to debug for you ... The .psm1 file could easily find them if there are all n a subfoder (Public / Private). No change to do in you psm1 file when you'll add a new function :-) Edit 2 : It's done on Github but not on the 0.41 version in PS Gallery.

5 Use and abuse Splat More readable code i.e. : $ForestInformation = Get-WinADForestDetails -Forest $Forest -IncludeDomains $IncludeDomains -ExcludeDomains $ExcludeDomains -ExcludeDomainControllers $ExcludeDomainControllers -IncludeDomainControllers $IncludeDomainControllers -SkipRODC:$SkipRODC very long line

Simple suggest, of course Regards Olivier

PrzemyslawKlys commented 4 years ago

Hello,

Thank you for opening this issue with suggestions:

Unfortunately, I tend to neglect help. I do build it sometimes in vs. code and generate that in PlatyPS. When I remember to do that, I do it. If not, I do hope someone will help me out. I have 50 PowerShell modules and spend a lot of time writing blog posts describing use cases - if it's not on GitHub in Get-Help you should try https://evotec.xyz/hub and see if I wrote about it. Feel free to help with updates to the code on GitHub, write examples or extend the help in the functions. It would be much appreciated.
I have a 4k monitor. I usually split it into two and work that way. It usually fits me. It's just easier for me to read and understand what is happening. If it gets a very long line, I do split it. Having said what you see in the PSM1 file is automated work and not me doing it. It happens during the removal of comments + merging process. See point 4. Take a look at original code:

https://github.com/EvotecIT/ADEssentials/blob/master/Public/Get-WinADGPOMissingPermissions.ps1

vs

function Get-WinADGPOMissingPermissions { 
    <#
    .SYNOPSIS
    Short description
    .DESCRIPTION
    Long description
    .PARAMETER Domain
    Parameter description
    .EXAMPLE
    An example
    .NOTES
    Based on https://secureinfra.blog/2018/12/31/most-common-mistakes-in-active-directory-and-domain-services-part-1/
    #>
    [cmdletBinding()]
    param([alias('ForestName')][string] $Forest,
        [string[]] $ExcludeDomains,
        [alias('Domain', 'Domains')][string[]] $IncludeDomains,
        [switch] $SkipRODC,
        [System.Collections.IDictionary] $ExtendedForestInformation,
        [validateset('AuthenticatedUsers', 'DomainComputers', 'Either')][string] $Mode = 'Either',
        [switch] $Extended)
    if (-not $ExtendedForestInformation) { $ForestInformation = Get-WinADForestDetails -Forest $Forest -IncludeDomains $IncludeDomains -ExcludeDomains $ExcludeDomains -ExcludeDomainControllers $ExcludeDomainControllers -IncludeDomainControllers $IncludeDomainControllers -SkipRODC:$SkipRODC } else { $ForestInformation = $ExtendedForestInformation }
    foreach ($Domain in $ForestInformation.Domains) {
        $QueryServer = $ForestInformation['QueryServers']["$Domain"].HostName[0]
        if ($Extended) {
            $GPOs = Get-GPO -All -Domain $Domain -Server $QueryServer | ForEach-Object { [xml] $XMLContent = Get-GPOReport -ID $_.ID.Guid -ReportType XML -Server $ForestInformation.QueryServers[$Domain].HostName[0] -Domain $Domain
                Add-Member -InputObject $_ -MemberType NoteProperty -Name 'LinksTo' -Value $XMLContent.GPO.LinksTo
                if ($XMLContent.GPO.LinksTo) { Add-Member -InputObject $_ -MemberType NoteProperty -Name 'Linked' -Value $true } else { Add-Member -InputObject $_ -MemberType NoteProperty -Name 'Linked' -Value $false }
                $_ | Select-Object -Property Id, DisplayName, DomainName, Owner, Linked, GpoStatus, CreationTime, ModificationTime,
                @{label        = 'UserVersion'
                    expression = { "AD Version: $($_.User.DSVersion), SysVol Version: $($_.User.SysvolVersion)" }
                },
                @{label        = 'ComputerVersion'
                    expression = { "AD Version: $($_.Computer.DSVersion), SysVol Version: $($_.Computer.SysvolVersion)" }
                }, WmiFilter, Description, User, Computer, LinksTo }
        } else { $GPOs = Get-GPO -All -Domain $Domain -Server $QueryServer }
        $DomainInformation = Get-ADDomain -Server $QueryServer
        $DomainComputersSID = $('{0}-515' -f $DomainInformation.DomainSID.Value)
        $MissingPermissions = @(foreach ($GPO in $GPOs) {
                $Permissions = Get-GPPermission -Guid $GPO.Id -All -Server $QueryServer -DomainName $Domain | Select-Object -ExpandProperty Trustee
                if ($Mode -eq 'Either' -or $Mode -eq 'AuthenticatedUsers') { $GPOPermissionForAuthUsers = $Permissions | Where-Object { $_.Sid.Value -eq 'S-1-5-11' } }
                if ($Mode -eq 'Either' -or $Mode -eq 'DomainComputers') { $GPOPermissionForDomainComputers = $Permissions | Where-Object { $_.Sid.Value -eq $DomainComputersSID } }
                if ($Mode -eq 'Either') { If (-not $GPOPermissionForAuthUsers -and -not $GPOPermissionForDomainComputers) { $GPO } } elseif ($Mode -eq 'AuthenticatedUsers') { If (-not $GPOPermissionForAuthUsers) { $GPO } } elseif ($Mode -eq 'DomainComputers') { If (-not $GPOPermissionForDomainComputers) { $GPO } }
            })
        $MissingPermissions
    }

Eighty-nine lines vs. 50 lines. This is intentional because it happens automatically. Check 4 to be sure to understand my use case.

I do have PSScriptAnalyzer running. Not sure what else could I fix with it? I also have it in my build script. Feel free to suggest options I could use.

FormatCodePSM1 = @{
                Enabled           = $true
                RemoveComments    = $true
                FormatterSettings = @{
                    IncludeRules = @(
                        'PSPlaceOpenBrace',
                        'PSPlaceCloseBrace',
                        'PSUseConsistentWhitespace',
                        'PSUseConsistentIndentation',
                        'PSAlignAssignmentStatement',
                        'PSUseCorrectCasing'
                    )

                    Rules        = @{
                        PSPlaceOpenBrace           = @{
                            Enable             = $true
                            OnSameLine         = $true
                            NewLineAfter       = $true
                            IgnoreOneLineBlock = $true
                        }

                        PSPlaceCloseBrace          = @{
                            Enable             = $true
                            NewLineAfter       = $false
                            IgnoreOneLineBlock = $true
                            NoEmptyLineBefore  = $false
                        }

                        PSUseConsistentIndentation = @{
                            Enable              = $true
                            Kind                = 'space'
                            PipelineIndentation = 'IncreaseIndentationAfterEveryPipeline'
                            IndentationSize     = 4
                        }

                        PSUseConsistentWhitespace  = @{
                            Enable          = $true
                            CheckInnerBrace = $true
                            CheckOpenBrace  = $true
                            CheckOpenParen  = $true
                            CheckOperator   = $true
                            CheckPipe       = $true
                            CheckSeparator  = $true
                        }

                        PSAlignAssignmentStatement = @{
                            Enable         = $true
                            CheckHashtable = $true
                        }

                        PSUseCorrectCasing         = @{
                            Enable = $true
                        }
                    }
                }
            }

It's already that way. GitHub uses this Private/Public (but also other folders if you check different modules). During publishing to PSGallery, I use a process of merging, cleanup of inside comments, and rerun PSscriptAnalyzer to do some last-minute formatting.

This is done on purpose because of: https://evotec.xyz/powershell-single-psm1-file-versus-multi-file-modules/

There is a considerable speed difference between single PSM1 and multi-file solution. I also use a lot of PowerShell modules as a dependency. For example:

https://github.com/EvotecIT/ADEssentials/blob/51baf4345d2406d304aa88b555eccb1b3c7baad5/ADEssentials.psd1#L53-L54

And in case of PowerShellGallery, you only see PSEventViewer. This is because of I have fully automated building process in place using my PSPublishModule PowerShell Module which is able to scan the code, find functions from other modules and if it's approved module for merging it doesn't bring the whole module but only puts some of the needed commands as Private.

Here's how the build process looks like:

It changes all that GitHub stuff you see, does all it's magic, and puts a single PSM1/PSD1 file automatically generated with improvements for speed. If I need to debug something, I either do it with PSM1 and search in code or do it locally on my dev version if I can reproduce it. Again it's a choice rather than me not understanding how to do it :-)

That's one of the things I do, but not that very often. I even have installed EditorServicesCommandSuite which gives me the ability to automatically convert to splat

I mostly avoid it because of no IntelliSense. My code changes a lot and until PowerShell team adds IntelliSense to splats it's hard for me to start using it on a permanent basis.

If you see improvements you can do in any of my 50 modules - feel free to help. I'm open for PR's :-)

Hope this helps a bit and explains the process

Rapidhands commented 4 years ago

Sorry for the convenients and myt language (not my mother tongue) Last Day, I've downloaded ADEssentials from PS Gallery. At this time PSScript Analyzer displayed lot of errors (Var not called, Set-... cmdlet but noShoudl Process.

At this time, with the current version available on Github, PSScritpAnalyzer return few Warnings and Informations

C:\Users\Olivier\Desktop\ADEssentials-master\ADEssentials-master\Public

RuleName Severity ScriptName Line Message

PSUseSingularNouns Warning Get-WinADD 1 The cmdlet 'Get-WinADDiagnostics' uses a plural noun. A
iagnostics singular noun should be used instead. .ps1 PSAvoidUsingEmptyCatchBlock Warning Get-WinADF 36 Empty catch block is used. Please use Write-Error or throw
orestObjec statements in catch blocks. tsConflict .ps1 PSAvoidUsingEmptyCatchBlock Warning Get-WinADF 47 Empty catch block is used. Please use Write-Error or throw
orestObjec statements in catch blocks. tsConflict .ps1 PSUseSingularNouns Warning Get-WinADF 1 The cmdlet 'Get-WinADForestRoles' uses a plural noun. A
orestRoles singular noun should be used instead. .ps1 PSUseOutputTypeCorrectly Information Get-WinADG 87 The cmdlet 'Get-WinADGPOMissingPermissions' returns an
POMissingP object of type 'System.Object[]' but this type is not
ermissions declared in the OutputType attribute. .ps1 PSUseSingularNouns Warning Get-WinADG 1 The cmdlet 'Get-WinADGPOMissingPermissions' uses a plural
POMissingP noun. A singular noun should be used instead. ermissions .ps1 PSUseSingularNouns Warning Get-WinADG 1 The cmdlet 'Get-WinADGPOSysvolFolders' uses a plural noun.
POSysvolFo A singular noun should be used instead. lders.ps1 PSUseSingularNouns Warning Get-WinADL 1 The cmdlet 'Get-WinADLMSettings' uses a plural noun. A
MSettings. singular noun should be used instead. ps1 PSUseSingularNouns Warning Get-WinADP 1 The cmdlet 'Get-WinADPriviligedObjects' uses a plural noun. riviligedO A singular noun should be used instead. bjects.ps1 PSUseOutputTypeCorrectly Information Get-WinADP 111 The cmdlet 'Get-WinADPriviligedObjects' returns an object riviligedO of type 'System.Object[]' but this type is not declared in
bjects.ps1 the OutputType attribute. PSUseSingularNouns Warning Get-WinADP 1 The cmdlet 'Get-WinADProxyAddresses' uses a plural noun. A
roxyAddres singular noun should be used instead. ses.ps1 PSUseSingularNouns Warning Get-WinADS 1 The cmdlet 'Get-WinADSiteConnections' uses a plural noun. A iteConnect singular noun should be used instead. ions.ps1 PSUseSingularNouns Warning Get-WinADS 1 The cmdlet 'Get-WinADSiteLinks' uses a plural noun. A
iteLinks.p singular noun should be used instead. s1 PSUseSingularNouns Warning Get-WinADT 1 The cmdlet 'Get-WinADTrusts' uses a plural noun. A singular rusts.ps1 noun should be used instead. PSUseSingularNouns Warning Set-WinADD 1 The cmdlet 'Set-WinADDiagnostics' uses a plural noun. A
iagnostics singular noun should be used instead. .ps1 PSUseShouldProcessForStateChangingF Warning Set-WinADD 1 Function 'Set-WinADDiagnostics' has verb that could change unctions iagnostics system state. Therefore, the function has to support .ps1 'ShouldProcess'. PSUseShouldProcessForStateChangingF Warning Set-WinADR 1 Function 'Set-WinADReplication' has verb that could change unctions eplication system state. Therefore, the function has to support .ps1 'ShouldProcess'. PSUseSingularNouns Warning Set-WinADR 1 The cmdlet 'Set-WinADReplicationConnections' uses a plural eplication noun. A singular noun should be used instead. Connection s.ps1 PSUseShouldProcessForStateChangingF Warning Set-WinADR 1 Function 'Set-WinADReplicationConnections' has verb that unctions eplication could change system state. Therefore, the function has to Connection support 'ShouldProcess'. s.ps1 PSUseShouldProcessForStateChangingF Warning Set-WinADT 1 Function 'Set-WinADTombstoneLifetime' has verb that could unctions ombstoneLi change system state. Therefore, the function has to support fetime.ps1 'ShouldProcess'. PSUseSingularNouns Warning Test-ADSit 1 The cmdlet 'Test-ADSiteLinks' uses a plural noun. A eLinks.ps1 singular noun should be used instead. PSUseOutputTypeCorrectly Information Test-ADSit 21 The cmdlet 'Test-ADSiteLinks' returns an object of type eLinks.ps1 'ordered' but this type is not declared in the OutputType attribute. PSUseSingularNouns Warning Test-DNSNa 1 The cmdlet 'Test-DNSNameServers' uses a plural noun. A meServers. singular noun should be used instead. ps1

Great Job ! I really appreciate your job ... and you know what ? Most of your modules should be included by default as posh Modules. :-)

PrzemyslawKlys commented 4 years ago

Can you send me the rules you're using for PSScriptAnalyzer? Not sure if I will use it now, but maybe in future I'll do updates. Slowly - day by day.

EvotecIT / ADEssentials

Some improvements #2