-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Use agent's temp dir if available #6673
Conversation
Param () | ||
|
||
$agentVersion = Get-VstsTaskVariable -Name 'agent.version' | ||
if (!$agentVersion -or (([version]'2.115.0').CompareTo([version]$agentVersion) -ge 1)) |
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 check just make sure the agent version is greater than 2.115.0 and the variable Agent.TempDirectory is defined since then. However, don't we need to check if Agent.TempDirectory is shorter than env:Temp for real?
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.
My assumption is that agent directory will mostly be smaller than temp directory. However, better to not make any assumption. Updated code.
{ | ||
return Get-VstsTaskVariable -Name 'Agent.TempDirectory' | ||
$agentTemp = Get-VstsTaskVariable -Name 'Agent.TempDirectory' | ||
if ($agentTemp.Length -le $envTemp) |
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.
$envTemp.Length?
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.
fixed and added L0 tests as well
@@ -204,4 +205,27 @@ function Merge-HashTables | |||
} | |||
$HashTableNew = $HashTableOld + $HashTableNew | |||
return $HashTableNew | |||
} | |||
|
|||
function Get-TempDirectoryPath |
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.
Consider:
The Service Fabric Update Manifests task also uses a temp directory (although it uses $ENV:AGENT_TEMPDIRECTORY) Probably would be prudent to put this in the Service Fabric common scripts and use it in both places.
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.
#fixed
|
||
$agentVersion = Get-VstsTaskVariable -Name 'agent.version' | ||
$envTemp = $env:Temp | ||
if ($agentVersion -and (([version]'2.115.0').CompareTo([version]$agentVersion) -lt 1)) |
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.
Consider the ifs can be combined using:
$envTemp = $env:TEMP
$agentTemp = $env:AGENT_TEMPDIRECTORY
if ($agentTemp -and $agentTemp.Length -lt $tmp.Length)
{
return $agentTemp
}
return $envTemp
If the agent doesn't support AGENT_TEMPDIRECTORY it will not be defined
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 have removed check for agent version. Still using Get-VstsTaskVariable to get agent temp dir rather than depend on $env
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.
Looks like the tests for Invoke-ActionWithRetries is now failing
Fixes #6466