patternshellgitMinor
Automating the download of a GitHub repo
Viewed 0 times
thegithubautomatingdownloadrepo
Problem
This script is designed to locate and download the first repository in your sidebar on the GitHub repository page. It downloads the .zip file for that repo and saves it to the default location.
I built this script as a test to determine what it would take to perform this type of operation since I needed to use it on something else, but this script itself is the actual script I use (very) infrequently.
Basically, any/all PowerShell idioms would be awesome to have feedback on. The comments are there for the person I was writing the script as an example for, but again this is the script I'm worried about. I want to continually improve it.
```
# This method will finish an IE download by forcing the 'ALT+S' key several times to ensure that the "Save" button gets
# hit. It then waits a specified amount of time before returning so that the user can allow the file download to be
# expected to have finished.
Function Finish-Download([__ComObject]$ie, [int]$iterations, [int]$sleep) {
Write-Verbose "Finish-Download: Entered 'Finish-Download ie iterations'"
Write-Debug "Finish-Download: 'iterations': $iterations"
Write-Debug ("Finish-Download: 'ie.HWND': " + $ie.HWND)
Write-Warning "Please do not make any mouse or keyboard interactions for this next step."
Write-Verbose "Finish-Download: Locating 'ie' process"
$ieProc = Get-Process | Where-Object { $_.Name -eq "iexplore" -and $_.MainWindowHandle -eq $ie.HWND }
Write-Verbose "Finish-Download: Attaching 'WScript.Shell' to 'ieProc'"
Write-Debug ("Finish-Download: 'ieProc.Id': " + $ieProc.Id)
$wScript = New-Object -ComObject WScript.Shell
if (-not $wScript.AppActivate($ieProc.Id)) {
Write-Error "Failed to attach WScript.Shell to appropriate process."
Return $false
}
# For SOME reason, WScript takes a moment to activate the process. So we need to sleep. During this part it's
# important that you don't make any mouse/keyboard movements.
# Another hiccup: I
I built this script as a test to determine what it would take to perform this type of operation since I needed to use it on something else, but this script itself is the actual script I use (very) infrequently.
Basically, any/all PowerShell idioms would be awesome to have feedback on. The comments are there for the person I was writing the script as an example for, but again this is the script I'm worried about. I want to continually improve it.
```
# This method will finish an IE download by forcing the 'ALT+S' key several times to ensure that the "Save" button gets
# hit. It then waits a specified amount of time before returning so that the user can allow the file download to be
# expected to have finished.
Function Finish-Download([__ComObject]$ie, [int]$iterations, [int]$sleep) {
Write-Verbose "Finish-Download: Entered 'Finish-Download ie iterations'"
Write-Debug "Finish-Download: 'iterations': $iterations"
Write-Debug ("Finish-Download: 'ie.HWND': " + $ie.HWND)
Write-Warning "Please do not make any mouse or keyboard interactions for this next step."
Write-Verbose "Finish-Download: Locating 'ie' process"
$ieProc = Get-Process | Where-Object { $_.Name -eq "iexplore" -and $_.MainWindowHandle -eq $ie.HWND }
Write-Verbose "Finish-Download: Attaching 'WScript.Shell' to 'ieProc'"
Write-Debug ("Finish-Download: 'ieProc.Id': " + $ieProc.Id)
$wScript = New-Object -ComObject WScript.Shell
if (-not $wScript.AppActivate($ieProc.Id)) {
Write-Error "Failed to attach WScript.Shell to appropriate process."
Return $false
}
# For SOME reason, WScript takes a moment to activate the process. So we need to sleep. During this part it's
# important that you don't make any mouse/keyboard movements.
# Another hiccup: I
Solution
I hope to have more for you but a couple of things to get started.
Verbose, Debug, Information
I am very happy to see you using these cmdlets for different levels of information output to the user. Its way better then a bunch of
You left a small configuration section for the end user to change the preference variables governing the use of the Write- cmdlets in your script. The better way to handle this is to convert your script so that it accepts the common cmdlet parameters e.g.
You can now call this script optionally with
This does not really apply with your script since you
Credentials
Blah blah WHY YOU SAVE PLAIN TEXT CREDENTIALS IN FILE. Ok, now that is out of the way. There are a couple of suggestions here.
-
Prompt the end user for credentials
You can use the
$userSuppliedCredentials = Get-Credential -Message "Type in your github credentials please"
"Username is : $($userSuppliedCredentials.UserName)"
"Password is : $($userSuppliedCredentials.GetNetworkCredential().Password)"
-
Store the credentials, as a securestring, in a file
You can store secure strings in a file for repeated use. They are designed to be decrypted by the user and computer that created them. Prompting a user for this information might be annoying but arguably more secure.
Nesting function calls
Obviously this is allowed since your code runs but you should at least move function declarations to the top of their respective scopes. That is common practice so it will make them easier to find. I personally would not nest declarations and would declare them in the same scope however the is nothing wrong with that approach in general.
IE automation
It usually is a source of heartache and people would be best to avoid it. There are some cases where you just can't get around it.
I do not have enough experience with this but could you not just use the GitHub API for the better portion of this? I know that you can easily query for repos and I would think you should be able to download the one you need using the release API documentation? I made a quick GitHub account and queried for the one repo (empty) I had. If you had multiple it should be easy to isolate one. Where to go from here is beyond my knowledge as of this writing though.
Verbose, Debug, Information
I am very happy to see you using these cmdlets for different levels of information output to the user. Its way better then a bunch of
Write-Host statements.You left a small configuration section for the end user to change the preference variables governing the use of the Write- cmdlets in your script. The better way to handle this is to convert your script so that it accepts the common cmdlet parameters e.g.
-Verbose, -WhatIf, etc. Have a look at this small example[CmdletBinding()]
param()
Write-Information "Good Morning Dave"
Write-Verbose "Start of script"
function Get-IdealSpeed{
Write-Verbose "Beware the implications of driving this fast"
return "88mph"
}
Function Get-Swear{
Write-Debug "Writing something naughty. Hide the children"
Write-Output "Great Scott!"
}
Write-Output "*shrug scripting shoulders*"
Get-IdealSpeed
Get-SwearYou can now call this script optionally with
-Verbose and -Debug without having to change preference variables.This does not really apply with your script since you
exit at the end but be careful about changing these session variables as they might have been set to something different before you set them. Some people set these in their profile scripts. Mostly just to make you aware. So depending on how people execute this it might be wise to get the current state of those variables to reset them when you are done with their use. Supporting the common cmdlet parameters seems a better approach though.Credentials
Blah blah WHY YOU SAVE PLAIN TEXT CREDENTIALS IN FILE. Ok, now that is out of the way. There are a couple of suggestions here.
-
Prompt the end user for credentials
You can use the
Get-Credential cmdlet to prompt the running user for a username and masked password. This is still technically stored in memory and accessible but better then just leaving it in plain text.$userSuppliedCredentials = Get-Credential -Message "Type in your github credentials please"
"Username is : $($userSuppliedCredentials.UserName)"
"Password is : $($userSuppliedCredentials.GetNetworkCredential().Password)"
-
Store the credentials, as a securestring, in a file
You can store secure strings in a file for repeated use. They are designed to be decrypted by the user and computer that created them. Prompting a user for this information might be annoying but arguably more secure.
Nesting function calls
Obviously this is allowed since your code runs but you should at least move function declarations to the top of their respective scopes. That is common practice so it will make them easier to find. I personally would not nest declarations and would declare them in the same scope however the is nothing wrong with that approach in general.
IE automation
It usually is a source of heartache and people would be best to avoid it. There are some cases where you just can't get around it.
I do not have enough experience with this but could you not just use the GitHub API for the better portion of this? I know that you can easily query for repos and I would think you should be able to download the one you need using the release API documentation? I made a quick GitHub account and queried for the one repo (empty) I had. If you had multiple it should be easy to isolate one. Where to go from here is beyond my knowledge as of this writing though.
$gitHubAPIURL = "https://api.github.com"
Invoke-RestMethod -Method Get -Uri "$gitHubAPIURL/user/repos?access_token=my_api_key" | Select -First 1Code Snippets
[CmdletBinding()]
param()
Write-Information "Good Morning Dave"
Write-Verbose "Start of script"
function Get-IdealSpeed{
Write-Verbose "Beware the implications of driving this fast"
return "88mph"
}
Function Get-Swear{
Write-Debug "Writing something naughty. Hide the children"
Write-Output "Great Scott!"
}
Write-Output "*shrug scripting shoulders*"
Get-IdealSpeed
Get-Swear$gitHubAPIURL = "https://api.github.com"
Invoke-RestMethod -Method Get -Uri "$gitHubAPIURL/user/repos?access_token=my_api_key" | Select -First 1Context
StackExchange Code Review Q#153172, answer score: 2
Revisions (0)
No revisions yet.