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
1 Answer 1
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.
-
\$\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– Ericrs2017年02月13日 17:14:07 +00:00Commented Feb 13, 2017 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– Matt2017年02月13日 17:18:26 +00:00Commented Feb 13, 2017 at 17:18
$DConfigs
should be null on every pass.$connectionstring1
and 2 are not quoted properly. There is some more as well. \$\endgroup\$