9
\$\begingroup\$

Based on this with a couple of changes.

Any issues you can point out would be great.

param([string]$targetServer, [string]$user, [string]$pass)

function Get-Service(
    [string]$serviceName = $(throw "serviceName is required"), 
    [string]$targetServer = $(throw "targetServer is required"))
{
    $service = Get-WmiObject  -Class Win32_Service `
        -ComputerName $targetServer -Filter "Name='$serviceName'" -Impersonation 3    
    return $service
}

Function Copy-Files {
param(
        [Parameter(Mandatory=$true,ValueFromPipeline=$True)]
        [string]$source,
        [Parameter(Mandatory=$true,ValueFromPipeline=$True)]
        [string]$targetServer
        )

$destination = "\\$targetServer\C$\APP\"

"Creating network share on $targetServer"        
$share = Get-WmiObject Win32_Share -List -ComputerName $targetServer
$share.create("C:\share","autoShare", 0)        

#create backup        
Move-Item -path $destination  -destination "$destination\backup"

#copy new files
Copy-Item -path $source  -destination $destination -recurse


"Removing network share on $targetServer"    
if ($s = Get-WmiObject -Class Win32_Share -ComputerName $targetServer -Filter "Name='autoShare'") `
  { $s.delete() }    

}

function Start-Service(
    [string]$serviceName = $(throw "serviceName is required"), 
    [string]$targetServer = $(throw "targetServer is required"))
{
    "Getting service $serviceName on server $targetServer"
    $service = Get-Service $serviceName $targetServer
    if (!($service.Started))
    {
        "Starting service $serviceName on server $targetServer"
        $result = $service.StartService()
        Test-ServiceResult -operation "Starting service $serviceName on $targetServer" -result $result   
    }
}

function Uninstall-Service(
    [string]$serviceName = $(throw "serviceName is required"), 
    [string]$targetServer = $(throw "targetServer is required"))
{
    $service = Get-Service  $serviceName   $targetServer

    if (!($service))
    { 
        Write-Warning "Failed to find service $serviceName on $targetServer. Nothing to uninstall."
        return
    }

    "Found service $serviceName on $targetServer; checking status"

    if ($service.Started)
    {
        "Stopping service $serviceName on $targetServer"
        #could also use Set-Service, net stop, SC, psservice, psexec etc.
        $result = $service.StopService()
        Test-ServiceResult -operation "Stop service $serviceName on $targetServer" -result $result
    }

    "Attempting to uninstall service $serviceName on $targetServer"
    $result = $service.Delete()
    Test-ServiceResult -operation "Delete service $serviceName on $targetServer" -result $result   
}

function Test-ServiceResult(
    [string]$operation = $(throw "operation is required"), 
    [object]$result = $(throw "result is required"), 
    [switch]$continueOnError = $false)
{
    $retVal = -1
    if ($result.GetType().Name -eq "UInt32") { $retVal = $result } else {$retVal = $result.ReturnValue}

    if ($retVal -eq 0) {return}

    $errorcode = 'Success,Not Supported,Access Denied,Dependent Services Running,Invalid Service Control'
    $errorcode += ',Service Cannot Accept Control, Service Not Active, Service Request Timeout'
    $errorcode += ',Unknown Failure, Path Not Found, Service Already Running, Service Database Locked'
    $errorcode += ',Service Dependency Deleted, Service Dependency Failure, Service Disabled'
    $errorcode += ',Service Logon Failure, Service Marked for Deletion, Service No Thread'
    $errorcode += ',Status Circular Dependency, Status Duplicate Name, Status Invalid Name'
    $errorcode += ',Status Invalid Parameter, Status Invalid Service Account, Status Service Exists'
    $errorcode += ',Service Already Paused'
    $desc = $errorcode.Split(',')[$retVal]

    $msg = ("{0} failed with code {1}:{2}" -f $operation, $retVal, $desc)

    if (!$continueOnError) { Write-Error $msg } else { Write-Warning $msg }        
}

function Install-Service(
    [string]$serviceName = $(throw "serviceName is required"), 
    [string]$targetServer = $(throw "targetServer is required"),
    [string]$displayName = $(throw "displayName is required"),
    [string]$physicalPath = $(throw "physicalPath is required"),
    [string]$userName = $(throw "userName is required"),
    [string]$password = $pass,
    [string]$startMode = "Automatic",
    [string]$description = "",
    [bool]$interactWithDesktop = $false
)
{
    # can't use installutil; only for installing services locally
    #[wmiclass]"Win32_Service" | Get-Member -memberType Method | format-list -property:*    
    #[wmiclass]"Win32_Service"::Create( ... )        

    # todo: cleanup this section 
    $serviceType = 16          # OwnProcess
    $serviceErrorControl = 1   # UserNotified
    $loadOrderGroup = $null
    $loadOrderGroupDepend = $null
    $dependencies = $null

    # description?
    $params = `
        $serviceName, `
        $displayName, `
        $physicalPath, `
        $serviceType, `
        $serviceErrorControl, `
        $startMode, `
        $interactWithDesktop, `
        $userName, `
        $password, `
        $loadOrderGroup, `
        $loadOrderGroupDepend, `
        $dependencies `

    "Username: $username  Password: $password"

    $scope = new-object System.Management.ManagementScope("\\$targetServer\root\cimv2", `
        (new-object System.Management.ConnectionOptions))
    "Connecting to $targetServer"
    $scope.Connect()
    $mgt = new-object System.Management.ManagementClass($scope, `
        (new-object System.Management.ManagementPath("Win32_Service")), `
        (new-object System.Management.ObjectGetOptions))

    $op = "service $serviceName ($physicalPath) on $targetServer"    
    "Installing $op"
    $result = $mgt.InvokeMethod("Create", $params)    
    Test-ServiceResult -operation "Install $op" -result $result
    "Installed $op"

    "Setting $serviceName description to '$description'"
    Set-Service -ComputerName $targetServer -Name $serviceName -Description $description
    "Service install complete"
}

function Publish-Service
{    
param(
        [Parameter(Mandatory=$true,ValueFromPipeline=$True)]
        [string]$targetServer
        )

    $serviceName = "APPSyncronizationService"     
    Uninstall-Service $serviceName $targetServer

    "Pausing to avoid potential temporary access denied"
    Start-Sleep -s 5 # Yeah I know, don't beat me up over this

    Copy-Files -source "C:\src\app\APP\APP.SyncService\bin\Debug\" -targetServer $targetServer               

    Install-Service `
    -ServiceName $serviceName `
    -TargetServer $targetServer `
    -DisplayName "APP Test Syncronization Service" `
    -PhysicalPath "C:\APP\APP.SyncService.exe" `
    -Username $user `
    -Description "Description"    

    Start-Service $serviceName $targetServer       
}

Publish-Service -targetServer $targetServer
\$\endgroup\$

1 Answer 1

9
+200
\$\begingroup\$

Your code and overall logic look good. In general the areas of improvement I see are

  • There are some things you are inconsistent about, e.g., function parameters
  • You are sending notification information down the pipeline.
  • Some of the parameter declaration could be improved.
  • I see that your are trying to make the code fit within a certain amount of characters on each line. While that is perfectly fine the are some features of PowerShell I could show that would improve code functionality and still give that same visual effect.

Param Block

Mandatory Parameters

In your first function you have the param $serviceName throw an error if it is not represent.

[string]$serviceName = $(throw "serviceName is required")

In another function you get the same thing but leveraging advanced parameters.

[Parameter(Mandatory=$true,ValueFromPipeline=$True)]
[string]$source,

So as you can see there is no reason for throw. The Mandatory flag set to true accomplishes the almost exact same thing. Calling the function without passing the parameter will make PowerShell ask for it.

cmdlet Get-Service at command pipeline position 1
Supply values for the following parameters:
serviceName: _ <-- Pretend that underscore is blinking

Perhaps this has occurred to you and you prefer the messages from throw. Setting the parameter will mitigate errors. If nothing else just take it as an FYI.

Watch out for scope

The function Install-Service has a parameter defined $password with a default of $pass. While that code will still function is can be misleading as $pass is a variable defined in a parent scope. Consider making that one mandatory as well and just use $pass when you are calling the function.

Error Checking

You make several calls to WMI but there is not guarantee they are going to work. Consider using a try/catch block or -SilentlyContinue and validating the result. Examples of both are below. These are not production examples but samples to show functionality.

# Try/Catch Example
try{
    $result = Get-WmiObject -Class Win32_Volume -ComputerName doesnotexist
} catch {
    "There was and error: $($_.Exception)"
}

# SilentlyContinue Example
$result = Get-WmiObject -Class Win32_Volume -ComputerName doesnotexist -ErrorAction SilentlyContinue
if(!$result){"Something happened"}

Code Clarity

I refereed to this in a bullet above about mak[ing] the code fit within a certain amount of characters on each line. While playing with the backticks works it can be annoying when you have to make changes and forget to add them. Once place in Install-Service where you use them and it is not even required. PowerShell is forgiving for this:

$params = 
    $serviceName, 
    $displayName, 
    $physicalPath, 
    $serviceType, 
    $serviceErrorControl, 
    $startMode, 
    $interactWithDesktop, 
    $userName, 
    $password,
    $loadOrderGroup,
    $loadOrderGroupDepend,
    $dependencies

In other places you cannot get away with that. When you call Install-Service you use backticks again to get each parameter on its own line. That in itself is fine but I wanted to show you splatting. You pass a hashtable of parameter and value pairs and splat them to the cmdlet. Each one is on its own line and can be edited easily in place.

$params = @{
    ServiceName = $serviceName
    TargetServer = $targetServer
    DisplayName = "APP Test Syncronization Service"
    PhysicalPath = "C:\APP\APP.SyncService.exe"
    Username = $user
    Description = "Description"  
}

Install-Service @params

Function names

Get-Service and Start-Service are already cmdlet names. What you are doing is changing the command order precedence. Yours will get called first but you are using them for the same functionality the builtin cmdlets.

# Using the builtin
Get-Service -ComputerName $target -Name Spooler

I would opt for just using the built in ones. They return [System.ServiceProcess.ServiceController] objects that you can reference to check command success.

Be careful with your function output

Again, this is really wrong but something you need to be aware of. You are sending lots of status information down the pipeline. Basic example being

"Pausing to avoid potential temporary access denied"

That line is sent down the output stream. If you are going capture output from function, or use the pipeline like you configured on some of your parameters ValueFromPipeline=$True, you might get unexpected behavior. If this is truly information only then consider Write-host or a separate logging function.

Test-ServiceResult

The way you convert the error code to its friendly text could be shortened. If you have at least PowerShell v5 you can use the enum keyword. If not you could always add a type definition as well. To keep it simple I am just going to improve your error code array.

You can also use the -is operator to test the type of a variable.

if ($result -is [uint32]) {$retVal = $result} else {$retVal = $result.ReturnValue}

if ($retVal -eq 0) {return}

$errorCodes = "Success","Not Supported","Access Denied","Dependent Services Running",
    "Invalid Service Control","Service Cannot Accept Control","Service Not Active","Service Request Timeout",
    "Unknown Failure","Path Not Found","Service Already Running","Service Database Locked","Service Dependency Deleted",
    "Service Dependency Failure","Service Disabled","Service Logon Failure","Service Marked for Deletion","Service No Thread",
    "Status Circular Dependency","Status Duplicate Name","Status Invalid Name","Status Invalid Parameter","Status Invalid Service Account",
    "Status Service Exists","Service Already Paused"

$msg = ("{0} failed with code {1}:{2}" -f $operation, $retVal, $errorCodes[$retVal]) 
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Great work on an awesome answer! Looking forward to seeing more! :-) \$\endgroup\$
    – Quill
    Commented Jan 13, 2016 at 17:36
  • 1
    \$\begingroup\$ "Remeber that you would need to use a" ...a what? \$\endgroup\$ Commented Jan 13, 2016 at 17:37
  • 1
    \$\begingroup\$ @Mat'sMug That was a thought that needed to be removed. I thought you needed something else but you don't. Also... are you my mug? \$\endgroup\$
    – Matt
    Commented Jan 13, 2016 at 17:44
  • 1
    \$\begingroup\$ Nope. Just one "t" here :-) ..BTW you've just earned a revival and necromancer badge for this answer, congrats - keep it up! \$\endgroup\$ Commented Jan 13, 2016 at 17:48
  • 5
    \$\begingroup\$ @Mat'sMug says we are supposed to lure you into the 2nd Monitor: chat.stackexchange.com/rooms/8595/the-2nd-monitor \$\endgroup\$
    – user34073
    Commented Jan 13, 2016 at 17:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.