QA script

This topic contains 6 replies, has 2 voices, and was last updated by Profile photo of Graham Beer Graham Beer 1 year, 7 months ago.

  • Author
    Posts
  • #26409
    Profile photo of Graham Beer
    Graham Beer
    Participant

    Hi,

    I've had to write a script for my company to do a few things before launching the application. I wrote the script but was directed on the requirements. I'm actually quite proud of what i've achieved by myself.
    I was wondering if anyone would be kind enough to QA the script, make sure the correct commands and best practice has been followed, would be much appropriated.

    # Script_Name : AAHelpLauncher_User_VR2.ps1
    # Description : Script to set and launch AA Help
    # Version : 0.1
    # Date Created : 13/06/2015
    # Created by : Graham Beer
    #
    ###################################################################
    
    [cmdletbinding()] #required to get -Verbose functionality
    param()
    
    #Set Filename and path variables
    $Updatefile = "C:\AAHELP\SCCM\PIXPAAHELP1.exe"
    $Shortcut = "C:\AAHELP\SCCM\AAHelpUpdater.lnk"
    
    #If exists update files
    	if (Test-Path $Updatefile) 
        	{
    		Write-Verbose "Updating...................." -Verbose
    		$process = (Start-Process -FilePath $Shortcut -PassThru)
    		$process.WaitForExit()
       }
    else {
    write-verbose "The file $($updatefile) does not exist. Process will quit" -Verbose
    break
    }
    
    #Set process Variable
    $Proc = 'PIXPAAHELP1.exe'
    
    Try {
    # Check if process is running
    		$tries = 0
    do {
        if ($tries -lt 10) {
           
           $Service = Get-CimInstance -Namespace root\CIMV2 -ClassName Win32_process | where name -eq $Proc | select -ExpandProperty name
          
            if ([boolean]$Service -eq $true) {
                Write-Warning "Service still running. Sleeping for 10 seconds."
                Start-Sleep -Seconds 5
                $tries++
            }
        }
         else {
               catch
        }
    } 
     while ([boolean]$Service -eq $true)
    }
     Catch {
            write-verbose "Process $Service issue. Script terminated!" -Verbose
            break
         }
    
    #Confirm process exits with a succesful code of 0
    If ($process.ExitCode -eq $null)
    		{
    		Write-verbose "Not Updated cancelled by user" -Verbose
    			}
    		else
    		{
    		Write-Verbose "Update complete" -Verbose
    				}
    
    #Check to see if file exists and remove
    if (test-path $shortcut) {
      Remove-Item $shortcut -force 
      Write-Verbose "Removal worked" -Verbose
         }
     Else {
      Write-Verbose "$($shortcut) does not exist" -Verbose
     }
       	
    
    #Launch AAHELP
    write-verbose "Will now launch AA Help.." -Verbose
    
    try {
    Invoke-WMIMethod -Class Win32_Process -Name Create -ArgumentList "C:\AAHELP\system\lib\AAStart.exe AAHELP"
    }
    catch {
    Write-verbose "failed to launch AA Help!" -Verbose
    }
    
  • #26421
    Profile photo of Richard Diphoorn
    Richard Diphoorn
    Participant

    Hi Graham, great script! First of all, I like to dig into other people's scripts, because it forces me to revisit some topics and knowledge. I always make use of the Community Book of PowerShell.org Best Practices when I write my scripts.

    My take on this script:

    You start your script with comments/help:

    # Script_Name : AAHelpLauncher_User_VR2.ps1
    # Description : Script to set and launch AA Help
    # Version : 0.1
    # Date Created : 13/06/2015
    # Created by : Graham Beer
    #
    ################################################################### 
    

    It is a recommended practice to start a script with this (I'm putting in a link to the relevant resource, because the website here strips away help/comment code blocks):

    DOC-01 Write comment-based help
    The reason for this is that the ### stuff does not do anything in your script; it just adds more bytes into it. What I put has functionality in it; you can use Get-Help, and it would respond with that block of code. That said, the extra comments you put do not contribute to the understanding of the script. Only but comment based help if you want to explain some really difficult stuff. As said, they just take up space and take away the readability of the script.

    You use the cmdletbinding attribute to get in the verbosity. That's ok, but then you put this code:

    [cmdletbinding()] #required to get -Verbose functionality
    param()
    
    #Set Filename and path variables
    $Updatefile = "C:\AAHELP\SCCM\PIXPAAHELP1.exe"
    $Shortcut = "C:\AAHELP\SCCM\AAHelpUpdater.lnk"
    

    I would separate the logic from the code. What I mean with that is that I separate it into a tool script and a controller script. The tool script does the actual work, the controller script utilizes the tool script. This makes the tool script reusable and transferable. For this you have to parameterize the tool script.

    I like to put my 'tools' in functions; this will make a tool script modular. Ultimately, you can put the tool script in a module, but for simplicity I'm not explaining that here.

    Furthermore, the script, as I can see, is following this logic:

      Test if a file exists
      If the file does not exist, stop the script
      In case of that the file exists, start the update process
      In meanwhile, test is the update process is running, with a maximum of 100 seconds. If the maximum has been reached, stop the script.
      If the update process has finished, remove the shortcut file.
      Start the newly updated program.

    This part of your code:

     [cmdletbinding()] #required to get -Verbose functionality
    param()
    
    #Set Filename and path variables
    $Updatefile = "C:\AAHELP\SCCM\PIXPAAHELP1.exe"
    $Shortcut = "C:\AAHELP\SCCM\AAHelpUpdater.lnk"
    
    #If exists update files
    	if (Test-Path $Updatefile) 
        	{
    		Write-Verbose "Updating...................." -Verbose
    		$process = (Start-Process -FilePath $Shortcut -PassThru)
    		$process.WaitForExit()
       }
    else {
    write-verbose "The file $($updatefile) does not exist. Process will quit" -Verbose
    break
    }
    
    #Set process Variable
    $Proc = "PIXPAAHELP1.exe"
    
    Try {
    # Check if process is running
    		$tries = 0
    do {
        if ($tries -lt 10) {
           
           $Service = Get-CimInstance -Namespace root\CIMV2 -ClassName Win32_process | where name -eq $Proc | select -ExpandProperty name
          
            if ([boolean]$Service -eq $true) {
                Write-Warning "Service still running. Sleeping for 10 seconds."
                Start-Sleep -Seconds 5
                $tries++
            }
        }
         else {
               catch
        }
    } 
     while ([boolean]$Service -eq $true)
    }
     Catch {
            write-verbose "Process $Service issue. Script terminated!" -Verbose
            break
         }
    
    #Confirm process exits with a succesful code of 0
    If ($process.ExitCode -eq $null)
    		{
    		Write-verbose "Not Updated cancelled by user" -Verbose
    			}
    		else
    		{
    		Write-Verbose "Update complete" -Verbose
    				} 
    
    #Check to see if file exists and remove
    if (test-path $shortcut) {
      Remove-Item $shortcut -force 
      Write-Verbose "Removal worked" -Verbose
         }
     Else {
      Write-Verbose "$($shortcut) does not exist" -Verbose
     } 
    

    I would transform into a function, like this:

     function Update-AAHelpFile 
    {
        [CmdletBinding()]
        param (
            [Parameter(Position = 0,Mandatory = $true)]
            [string]$UpdateFile,
    
            [Parameter(Position = 1,Mandatory = $true)]
            [string]$ShortcutFile
        )
        
        if (Test-Path("$UpdateFile"))
        {
            try 
            {
                Write-Verbose -Message "The file $UpdateFile exists, trying to update it."
                Start-Process -FilePath $ShortcutFile -PassThru | Wait-Process -Timeout 100 -ErrorAction Stop
            }
            catch
            {
                Write-Warning $_.Exception.Message
            }
            finally
            {
                Write-Verbose -Message 'Update Completed.'
                Remove-Item $ShortcutFile -Force -Confirm:$false
            }
      
        }
        else 
        {
            Write-Warning -Message "The file $($UpdateFile) does not exist. Process will quit" -Verbose
            Break
        }
    } 
    

    I make use of parameterization, so I can reuse the function in other controller scripts. Then I start the update proces, and keep waiting for the process to finish with a maximum of 100 seconds. (You could build in some code to stop the process also).

    Then you don't need to look up specific running processes, because you make use of the builtin cmdlets. The cmdlet Wait-Process does all the hard work for you, to wait for a successful exit of the previous process. Also I utilize the try/catch/finally block, to combine the code for updating and cleaning up after the updating.

    Leaves us with starting the program. You start it with this code:

    #Launch AAHELP
    write-verbose "Will now launch AA Help.." -Verbose
    
    try {
    Invoke-WMIMethod -Class Win32_Process -Name Create -ArgumentList "C:\AAHELP\system\lib\AAStart.exe AAHELP"
    }
    catch {
    Write-verbose "failed to launch AA Help!" -Verbose
    }
    

    I'm not sure why you would use Invoke-WMIMethod. I would use Start-Process instead, and put it into a function, like this:

     Function Start-AAHelp 
    {
        [CmdletBinding()]
        param (
            [Parameter(Position = 0,Mandatory = $true)]
            [string]$Program,
            
            [Parameter(Position = 1,Mandatory = $false)]
            [string[]]$Arguments
        )
    
        try 
        {
    Write-Verbose -Message 'Will now launch AA Help..' -Verbose
            Start-Process -FilePath $Program -ArgumentList $Arguments -ErrorAction Stop
        }
        catch 
        {
            throw $_.Exception.Message
        }
    } 
    

    In this way you can easily pass parameters to the program, and thus reuse this function again.

    This is the tool script. My controller script would look like this:

     #requires -Version 2
     
    
    . C:\Temp\AAHelp-Functions.ps1
    
    Try 
    {
        Update-AAHelpFile -UpdateFile 'C:\AAHELP\SCCM\PIXPAAHELP1.exe' -ShortcutFile 'C:\AAHELP\SCCM\AAHelpUpdater.lnk' -Verbose
    }
    Catch 
    {
        throw $_
    }
    Finally
    {
        Start-AAHelp -Program 'C:\AAHELP\system\lib\AAStart.exe' -Arguments 'AAHELP' -Verbose
    } 
    

    I hope that I gave you some inspiration. For the complete scripts, please look here:

    Controller Script
    Functions Script

  • #26424
    Profile photo of Graham Beer
    Graham Beer
    Participant

    Hi Richard,

    Thank you for taking the time and giving your feedback. Some great points and the guides will be read !
    In new to powershell, couple of months in, can you explain what is happening here please ?

    Function Start-AAHelp
    {
    [CmdletBinding()]
    param (
    [Parameter(Position = 0,Mandatory = $true)]
    [string]$Program,

    [Parameter(Position = 1,Mandatory = $false)]
    [string[]]$Arguments
    )

  • #26431
    Profile photo of Richard Diphoorn
    Richard Diphoorn
    Participant

    Yes, the function takes in two parameters, Program and Arguments. With Position you're saying in which order the parameters has to appear when you type the function. With mandatory you're saying if you have to use the parameter or not. The parameters are defined as strings. The parameter Program takes in a single string, the parameter Arguments takes in multiple strings. But if I think of that, that's not necessary actually because you submit a complete string of arguments to the program. 🙂

    Does this answer your question?

  • #26435
    Profile photo of Graham Beer
    Graham Beer
    Participant

    Kind of. Some of this stuff is new to me, so it's understanding and getting used to it. Be great to have your knowledge 🙂
    Can you readd the function script ? Link not working, keen to have a look 🙂

  • #26436
    Profile photo of Richard Diphoorn
    Richard Diphoorn
    Participant

    Graham, I've updated the link.

    An advice, this is a great start to build up your knowledge about functions and stuff: Learn PowerShell Toolmaking in a Month of Lunches

  • #26440
    Profile photo of Graham Beer
    Graham Beer
    Participant

    Thank you again Richard. Funny enough that was in my list ! I have learn powershell in a month of lunches, deep dives and in depth. All great books. Also signed up for a pluralsight subscription and half way through Jeff Hicks v3/v4 videos, which are superb. Just getting my hands dirty now ! Plus this forum is full of really nice and helpful people 😊

You must be logged in to reply to this topic.