-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated Install-omsAgent for issue #2 #5
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Monterey,
Thanks for the PR. Sorry about taking so long to review.
Just added a couple of my thoughts as comments.
Thanks
Ben
@@ -96,10 +105,10 @@ function Install-OmsAgent | |||
$path | |||
} | |||
|
|||
if($PSBoundParameters.sourcePath -eq $true) | |||
if($PSBoundParameters.sourcePath) # Check for source path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment as not needed
@@ -26,27 +26,36 @@ function Install-OmsAgent | |||
[Alias('IPAddress', 'Name')] | |||
[string[]] | |||
$computerName = $env:COMPUTERNAME, | |||
[Parameter(Mandatory=$true, ParameterSetName='workSpaceClearText')] | |||
[Parameter(Mandatory=$true, ParameterSetName='localOms-workSpaceClearText')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove '-' out of all Parameter Set names to keep in keeping with camel case
.Synopsis | ||
Update the OMS agent on remote computers. | ||
.DESCRIPTION | ||
Either downloads the installer from a URL or copies the installer via the powershell session. Can detected if a previous version is installed and skip if so. If allready installed WorkSpaceId and WorkSpaceKey added to previous install. Doesn't detect invalid workspace IDs or Keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the description be updated so it is easier to understand exactly what the CMDLet does.
[Parameter(Mandatory=$true, ParameterSetName='downloadOms')] | ||
[ValidateNotNullOrEmpty()] | ||
[string] | ||
$downloadURL = 'http://download.microsoft.com/download/0/C/0/0C072D6E-F418-4AD4-BCB2-A362624F400A/MMASetup-AMD64.exe', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure about specifying a download link here as it may not be a current one. My feeling is to make the user specify there own DL link so they can make sure it is current, incase the default link is a old version. Thought?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree however it is very difficult to find the download link at least in experience. I will test the link and see what version is being downloaded or see if there is an aka link out there
} | ||
|
||
|
||
Write-Verbose "$computer - Trying to Update OMS..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add [$(Get-Date -Format G)] to verbose output to keep consistant.
Get-service HealthService|Start-Service | ||
} -ErrorAction Stop | ||
Write-Verbose "$computer - Restarting HealthService" | ||
Write-Error "$computer - OMS didn't update correctly based on the exit code" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add [$(Get-Date -Format G)] to verbose output to keep consistant.
} | ||
else | ||
{ | ||
if((Get-omsAgentInternal -computerName $computer -session $psSession).displayverison -gt $orginalAgent.displayverison) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add logic so if "(Get-omsAgentInternal -computerName $computer -session $psSession).displayverison -eq $orginalAgent.displayverison)" the service is started. Might make it a little more robust if something stopped the agent installing but didn't change the original install.
$installString = $path + ' /Q:A /R:N /C:"setup.exe /qn AcceptEndUserLicenseAgreement=1"' | ||
|
||
$installSuccess = Invoke-Command -Session $psSession -ScriptBlock { | ||
Get-service HealthService|Stop-Service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add spaces around the pipe for readability.
Write-Verbose "$computer - OMS updated correctly based on the exit code" | ||
Write-Verbose "$computer - Restarting HealthService" | ||
Invoke-Command -Session $psSession -ScriptBlock { | ||
Get-service HealthService|Start-Service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add spaces around the pipe for readability.
[ValidateNotNullOrEmpty()] | ||
[string] | ||
$workspacekey, | ||
[Parameter(Mandatory=$true, ParameterSetName='workSpaceEncrypt')] | ||
[Parameter(Mandatory=$true, ParameterSetName='localOms-workSpaceEncrypt')] | ||
[Parameter(Mandatory=$true, ParameterSetName='downloadOms-workSpaceEncrypt')] | ||
[System.Management.Automation.PSCredential] | ||
[System.Management.Automation.Credential()] | ||
$workSpace, | ||
[Parameter(ParameterSetName='downloadOMS')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter set name needs changing,
Default parameter set in Install-omsAgent needs changing. |
There is no pester test with Update-omsAgent |
any update on this? |
Hi, I have some other changes and updated tests to push. Will add the changes from this PR. I will try and do it this week. Thanks |
Thanks Ben
On Mon, Jan 29, 2018 at 12:46 PM Ben Taylor ***@***.***> wrote:
Hi,
I have some other changes and updated tests to push. Will add the changes
from this PR.
I will try and do it this week.
Thanks
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIvmUAfUuuNK2z-ePHkousiHPnOXTy-Aks5tPgPmgaJpZM4Os_IW>
.
--
Carlos Conrado
|
Updated Install-omsAgent to resolve issue #2