4
\$\begingroup\$

I was tasked with this assignment with almost no time and I cannot stand up a new environment to test this. The risk is pretty high with what is the request is for so I'm asking for as much peer and code review as I can get.

I'm tasked to check all the web.configs that have a certain DB Server and change it to a new value.

This will happen across about 250 servers with about an hour maintenance window.

My first pass I want it to find the configs and place it in a folder on my local machine for me to review the change with C:\NewConfigs\FullPathofConfigHere

Second pass I will actually set-content or create a new config which (this is commented out for now) to the configs in their current location. All of which is on either a D: or E: Drive.

$servers = Get-Content "servers.txt"
$WebConfigFile = "web.config"
$connectionstring1 = "DBstring1.local.domain"
$connectionstring2 = "DBstring2.local.domain"
$to = "C:\ConfigFinder\BackupConfigs\"
$NewFolder = "C:\ConfigFinder\NewConfigs\"

Function Backup {
    foreach ($computer in $servers) {

        Get-ChildItem -Recurse -Force \\$computer\d$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
        Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
        Select-Object FullName |
        #This makes a backup copy before doing any rewrite
        % {
            $newpath = join-path $To $_.FullName.ToLower()
            md $newpath
            Copy-Item $_.FullName.ToLower() -destination $newpath -verbose
        }

        Get-ChildItem -Recurse -Force \\$computer\e$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
        Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
        Select-Object FullName |
        #Select-String $connectionstring1 |
        #This makes a backup copy before doing any rewrite             
        % {
            $newpath = join-path $To $_.FullName
            md $newpath
            Copy-Item $_.FullName.ToLower() -destination $newpath -verbose
        }

    }
}
Function CreateLocal {
    foreach ($computer in $servers) {

        Get-ChildItem -Recurse -Force \\$computer\d$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
        Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
        Select-Object FullName |
        #Select-String $connectionstring1 | 
        #This makes a backup copy before doing any rewrite
        % {
            $ConfigName = "Web.qa.Config"
            $newpath = join-path $NewFolder $_.FullName.Replace("Web.config","")
            md $newpath
            $finaldestination = $newpath + $ConfigName
            (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Out-File $finaldestination
        }
        Get-ChildItem -Recurse -Force \\$computer\e$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
        Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
        Select-Object FullName |
        #This makes a backup copy before doing any rewrite             
        % {
            $ConfigName = "Web.qa.Config"
            $newpath = join-path $NewFolder $_.FullName.Replace("Web.config","")
            md $newpath
            $finaldestination = $newpath + $ConfigName
            (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Out-File $finaldestination
        }

    }
}
Function ConfigonServer {
    Write-Host "CAUTION YOU ARE ABOUT TO WRITE NEW CONFIGS ON THE SERVERS"
    $resp = Read-Host " Are you SURE you want to continue? (Y/[N])"
    if ($Resp.ToUpper() -eq "N") {
        Write-Host "Taking you back to Safety"
        sleep 3
        Menu 
    }
    if ($Resp.ToUpper() -eq "Y") {
        foreach ($computer in $servers) {

            Get-ChildItem -Recurse -Force \\$computer\d$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
            Where-Object {$_.FullName -notlike "*Recycle.bin*"}  |
            Select-Object FullName |
            #Select-String $connectionstring1 | 
            #This makes a backup copy before doing any rewrite
            % {
                $ConfigName = "Web.qa.Config"
                $finaldestination = $_.FullName.replace("Web.config","") + $ConfigName
                (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Out-File $finaldestination -encoding "UTF8"
            }
            Get-ChildItem -Recurse -Force \\$computer\e$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
            Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
            Select-Object FullName |
            #Select-String $connectionstring1 |
            #This makes a backup copy before doing any rewrite             
            % {
                $ConfigName = "Web.qa.Config"
                $finaldestination = $_.FullName.replace("Web.config","") + $ConfigName
                (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Out-File $finaldestination -encoding "UTF8"
            }
        }

    }
}
Function HoldMyBeer {
    Write-Host "CAUTION YOU ARE ABOUT TO RE-WRITE ALL THE CONFIGS"
    $resp = Read-Host " Are you SURE you want to continue? (Y/[N])"
    if ($Resp.ToUpper() -eq "N") {
        Write-Host "Taking you back to Safety"
        sleep 3
        Menu 
    }
    if ($Resp.ToUpper() -eq "Y") {

        foreach ($computer in $servers) {

            Get-ChildItem -Recurse -Force \\$computer\d$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
            Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
            Select-Object FullName |
            #Select-String $connectionstring1 | 
            #This makes a backup copy before doing any rewrite
            % {
                (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Set-Content $_.FullName -encoding "UTF8"
            }
            Get-ChildItem -Recurse -Force \\$computer\e$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
            Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
            Select-Object FullName |
            #Select-String $connectionstring1 |
            #This makes a backup copy before doing any rewrite          
            % {
                (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Set-Content $_.FullName -encoding "UTF8"
            }
        }
    }
}
Function Menu      {
    Do {
        Write-Host ""
        Write-Host ""
        Write-Host     "===================================================="
        Write-Host "What would you like to do Today?"
        Write-Host     "===================================================="
        Write-Host ""
        Write-Host "1. Backup to Local Disk" -foregroundcolor green
        Write-Host "2. Create New Strings to Local Disk" -foregroundcolor cyan
        Write-Host "3. Create New Configs on The Server List" -foregroundcolor yellow
        Write-Host "4. Re-write the Files on the servers)" -foregroundcolor magenta
        Write-Host "5. Exit"

        Write-Host ""
        Write-Host ""
        Write-Host $errout
        $Choice = Read-Host '(1-5)'

        switch ($Choice) {
            1 {
                Backup; break
            }
            2 {
                CreateLocal; break
            }
            3 {
                ConfigonServer; break
            }
            4 {
                HoldMyBeer; break
            }
            5 {
                Exit;exit
            }
            default {
                $errout = "No, try again........Try 1-5 only"
            }

        }

    }
    until ($Choice -ne "")
}
Menu
\$\endgroup\$
  • 2
    \$\begingroup\$ Does this code actually worK? I ask because I see several points of failure here. $DConfigs should be null on every pass. $connectionstring1 and 2 are not quoted properly. There is some more as well. \$\endgroup\$ – Matt Feb 10 '17 at 4:19
  • \$\begingroup\$ I don't know. to be honest. It's too risky to run. SO Closed my thread there and told me to post here. I've made a few changes \$\endgroup\$ – Ericrs Feb 10 '17 at 17:06
  • \$\begingroup\$ @Ericrs One of the most important rules of Code Review is all code must work as intended. If you don't know, that's a problem. Please find a way to find out and update your code if necessary (or post a new question if an answer has been provided in the meantime). Please take a look at our help center. \$\endgroup\$ – Mast Feb 10 '17 at 20:41
  • \$\begingroup\$ @Ericrs I am going to check your SO post. perhaps it needs some rephrasing. People won't be able to debug for you so I might understand why it was closed. You need to make a test environment with copies of your files. I would never trust code from people on the internet to be useful without testing. Regardless of the source. \$\endgroup\$ – Matt Feb 10 '17 at 21:07
  • \$\begingroup\$ I have it working now. I've updated it. \$\endgroup\$ – Ericrs Feb 10 '17 at 23:11
3
\$\begingroup\$

Operator Case Sensitivity

By default most (all?) of the comparison operators, like -eq and -like, are case-insensitive by default. What that means is that code like

if ($Resp.ToUpper() -eq "N")

is redundant and functionally the same as

if ($Resp -eq "N")

While on the topic of case sensitivity .ToLower() serves no purpose here

Copy-Item $_.FullName.ToLower()

Verb-Noun Naming Convention

PowerShell function naming recommendations are Verb-Noun(s). The action you are performing and the object of your action. You see this is all stock cmdlets like Get-Item. MSDN has an extensive but simple to follow list of recommendations. Trying and align the name as best as possible with what the cmdlet/function/code is doing. If you are having an issue figuring out a name it is possible that you need to break up that code into separate pieces.

Not to overly criticize HoldMyBeer. At least Hold-Beer would be better :)

The Choice Menu System

PowerShell has a great way of creating menus guiding user input and acting on the results. It is not too hard to get a grasp at first glance so I am going to include the code snippet from the from the front of that article.

$title = "Delete Files"
$message = "Do you want to delete the remaining files in the folder?"

$yes = New-Object System.Management.Automation.Host.ChoiceDescription "&Yes", `
"Deletes all the files in the folder."

$no = New-Object System.Management.Automation.Host.ChoiceDescription "&No", `
"Retains all the files in the folder."

$options = [System.Management.Automation.Host.ChoiceDescription[]]($yes, $no)

$result = $host.ui.PromptForChoice($title, $message, $options, 0)

switch ($result)
{
0 {"You selected Yes."}
1 {"You selected No."}
}

This would have more functionality and less worrying about code and enduser selections. This way you know the enduser can only select a valid option.

Select-Object -ExpandProperty

When you are using select-object you are getting an object array containing the properties you requested. This is the case even if you select one property.

To make subsequent use of that single property you are return just that property as an array as supposed to an object array with that property. Consider the following code:

Get-ChildItem -Recurse -Force \\$computer\e$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
      Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
      Select-Object FullName | # ....

This could be simplified

Get-ChildItem -Recurse -Force "\\$computer\e$" -ErrorAction SilentlyContinue -Include $WebConfigFile | 
    Select-Object -ExpandProperty FullName | 
    Where-Object {$_ -notlike "*Recycle.bin*"} | # ....

This will also see a positive effect in other parts of your code as you refer to fullname frequently.

Code Repetition

If you find yourself repeating the same code over and over again you should be asking if there is another way. Functions and better use of cmdlet parameters here would make some headway for you.

You are gathering files rather frequently. Albeit the code is used only once per menu selection. However if you had to make a logic change you would have to ensure your doing it in around 8 places. That is a huge margin for error.

Heck you could even consolidate both these blocks since Get-ChildItem supports arrays for -Path

    Get-ChildItem -Recurse -Force \\$computer\d$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
    Where-Object {$_.FullName -notlike "*Recycle.bin*"}  |
    Select-Object FullName |
    #Select-String $connectionstring1 | 
    #This makes a backup copy before doing any rewrite
    % {
        $ConfigName = "Web.qa.Config"
        $finaldestination = $_.FullName.replace("Web.config","") + $ConfigName
        (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Out-File $finaldestination -encoding "UTF8"
    }
    Get-ChildItem -Recurse -Force \\$computer\e$ -ErrorAction SilentlyContinue -Include $WebConfigFile | 
    Where-Object {$_.FullName -notlike "*Recycle.bin*"} |
    Select-Object FullName |
    #Select-String $connectionstring1 |
    #This makes a backup copy before doing any rewrite             
    % {
        $ConfigName = "Web.qa.Config"
        $finaldestination = $_.FullName.replace("Web.config","") + $ConfigName
        (Get-Content $_.FullName).replace($connectionstring1, $connectionstring2) | Out-File $finaldestination -encoding "UTF8"
    }

The first line would work with this small change.

Get-ChildItem -Recurse -Force "\\$computer\d$", "\\$computer\e$" -ErrorAction SilentlyContinue

That cuts out the second block entirely since they both appear to do the same thing.

Filter > Include

Since you are only trying to find files with a certain name and are searching recursively through drive you will find that -Filter will out perform -Include since it functions at the provider level. From Get-ChildItem on MSDN

Filters are more efficient than other parameters, because the provider applies them when retrieving the objects, rather than having Windows PowerShell filter the objects after they are retrieved from the provider.

Consistency

I see that you are using both Out-File and Set-Content. Pick one and stick with it. If someone else is reading your code they may have to spend time wondering why you chose this. Set-Content should perform better between the two.

Trepidation of running this code

If you are at all concerned about running this code it would be a trivial exercise to set up a test environment to ensure that your code only does what you expect it to.


There are some other areas that could be addressed but the ones above would be the ones I would actually focus on first.

\$\endgroup\$
  • \$\begingroup\$ I'll check these out. As far as setup a test environment the problem isn't me not wanting to. it's others higher up not wanting to spend the time and the money to do so. \$\endgroup\$ – Ericrs Feb 13 '17 at 17:14
  • \$\begingroup\$ @Ericrs All I am suggesting is getting some files, copying them to a neutral location, and test your script. Takes only a few minutes. You want to be sure it does what you think it does. \$\endgroup\$ – Matt Feb 13 '17 at 17:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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