Users/pragupta/azurepwrshelltask #3175
Conversation
| $scriptPath = [System.IO.Path]::Combine([System.IO.Path]::GetTempPath(), $tempFileName); | ||
| ($scriptInline | Out-File $scriptPath) | ||
|
|
||
| Write-Host "tempFile= $scriptPath" |
There was a problem hiding this comment.
this looks like debug messaging to me. consider the write-verbose instead.
| @@ -2,13 +2,17 @@ Trace-VstsEnteringInvocation $MyInvocation | |||
| Import-VstsLocStrings "$PSScriptRoot\Task.json" | |||
|
|
|||
| # Get inputs. | |||
| $scriptPath = Get-VstsInput -Name ScriptPath -Require | |||
| $scriptType = Get-VstsInput -Name ScriptType -Require | |||
There was a problem hiding this comment.
when script type is filepath, then shouldn't ScriptPath be required? i.e. Get-VstsInput -Name ScriptPath -Require
similar thing for inline scenario
| If ($scriptType -eq "InlineScript") { | ||
| $tempFileName = [guid]::NewGuid().ToString() + ".ps1"; | ||
| $scriptPath = [System.IO.Path]::Combine([System.IO.Path]::GetTempPath(), $tempFileName); | ||
| ($scriptInline | Out-File $scriptPath) |
There was a problem hiding this comment.
you should try/finally and best-effort cleanup this temp file. here is what the agent does for the powershell task. it creates a warning issue if the file fails to delete for some reason.
| "visibleRule": "ScriptType = FilePath" | ||
| }, | ||
| { | ||
| "name": "AzurePowerShellInline", |
There was a problem hiding this comment.
nit, "AzurePowerShell" in the input name seems redunant. it's the Azure PowerShell task.
| "helpMarkDown": "Type of the script: File Path or Inline Script", | ||
| "defaultValue": "FilePath", | ||
| "options": { | ||
| "FilePath": "AzurePowerShell Script File Path", |
There was a problem hiding this comment.
nit, "AzurePowerShell" in the dropdown items seems redundant to me. it's the Azure PowerShell task, so this is implied already.
There was a problem hiding this comment.
...nit continued :)
also, i would even say having "AzurePowerShell" in the dropdown item text may be confusing. the task initializes the Azure module and then runs the PowerShell script. other than the name of the task, there is no such thing as "Azure PowerShell"
|
i believe @kmkumaran 's team owns this task. i'm happy to provide a review, but signoff should come from the owners of this task. |
|
nit, also i noticed the commits contains a merge commit. rebase is nicer for viewing history. sometimes merges are convenient, but for a small changes generally rebase is better |
|
@ericsciple - sure. My team will own the final sign-off for this task. |
No description provided.