My first Advanced Function

This topic contains 2 replies, has 2 voices, and was last updated by Profile photo of Ondrej Zilinec Ondrej Zilinec 2 years, 2 months ago.

  • Author
    Posts
  • #19448
    Profile photo of Ondrej Zilinec
    Ondrej Zilinec
    Participant

    Hello dudes

    I've created my advanced function and I want to know your remarks and notes. This function should read computer names from AD an invoke scriptblocks on them:

    
    Function Invoke-CommandOnADComputers
    {
        [CmdletBinding(SupportsShouldProcess=$True,ConfirmImpact='Low')]
        Param
        (
            # This is Active Directory Search Base to limit selection for computer accounts in domain.
            # It can be for example "OU=Computers,OU=Company Name,DC=domain,DC=local"
            [parameter(Mandatory=$true)]
            [string]
            $SearchBase,
    
            # Active Directory filter to merge your computer selection in to the detail.
            # It can be for example 'Name -like "Desktop*"'
            [string]
            $Filter = "*",
    
            # This is scriptblock which should be run on every computer.
            # For example { Restart-Service W32Time; }
            [parameter(Mandatory=$true)]
            [scriptblock]
            $ScriptBlock
        )
        Begin
        {
            #
            # Get list of computer accounts
            #
            Write-Debug "Getting list of computer from $ADSear"
            try
            {
                $ADComputersList = Get-ADComputer -SearchBase $SearchBase -Filter $Filter -ErrorAction Stop
            }
            catch
            {
                Write-Error -Message "Couldn't search in $SearchBase" -ErrorAction Stop
            }
            #
            # Write number of found computers
            #
            Write-Output "Found $($ADComputersList.Count) computers"
            #
            # If in debug, write list of computers
            #
            Write-Debug "List of machines:"
            If (!$PSDebugContext)
            {
                foreach ($item in $ADComputersList)
                {
                    Write-Debug " $($item.Name)"
                }
            }
            Write-Debug "Done with domain computer list"
        }
        Process
        {
            #
            # Let's invoke command on remote computer
            #
            foreach ($ADComputer in $ADComputersList)
            {
                Write-Output $ADComputer.Name
                    try
                    {
                        Write-Debug "Invoking scriptblock on computer"
                        Invoke-Command -ComputerName $ADComputer.Name -ScriptBlock { $ScriptBlock } -ErrorAction Stop
                        Write-Output " Scriptblock invoked successful."
                    }
                    catch 
                    {
                        Write-Output " Scriptblock invoked UNSUCCESSFUL."
                    }
            }
        }
    }
    
  • #19449
    Profile photo of Daniel Krebs
    Daniel Krebs
    Participant

    Hi Ondrej,

    Thanks for sharing your function with us.

    My thoughts:
    1. I would not execute the AD search in your function. Get-ADComputer has some other parameters like -Server or -Credential which I'm unable to use now because you're function does not allow them. My approach would be to have the function take any array of computer names and work with it. It does not matter if the computer names came from AD or another database.
    2. It looks like you're using Write-Output instead of Write-Host to output to the host but don't output proper objects into the pipeline. My recommendation is to use Write-Verbose instead and to think about a [pscustomobject] which you can output to the pipeline with the results of the Invoke-Command to digest later.
    3. I think your usage of Write-Debug is not correct because with debug enabled your Write-Debug statement act like breakpoints as far as I know. I would change to Write-Verbose and ask users to specify -Verbose if they want to see what is happening.
    4. You have 2 foreach loops going over ADComputerList and you output the computer name anyway in your main foreach without debug enabled. I would remove the 1st foreach and use Write-Verbose instead of Write-Output in the main foreach loop.
    5. You're not grabbing the results of Invoke-Command. I would grab the results and return them in a [pscustomobject] into the pipeline to make your functions more useful.

    Best,
    Daniel

  • #19459
    Profile photo of Ondrej Zilinec
    Ondrej Zilinec
    Participant

    Thank you for your remarks. I will think of them and maybe post changed function 🙂

You must be logged in to reply to this topic.