-
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
Users/arjgupta/af cv2 use response file #7506
Conversation
Tasks/AzureFileCopyV2/Utility.ps1
Outdated
@@ -294,6 +305,9 @@ function Upload-FilesToAzureContainer | |||
finally | |||
{ | |||
Handle-AzCopyLogs -isLogsPresent $useDefaultArguments -logsFilePath $azCopyLogFilePath -ErrorAction SilentlyContinue | |||
if ((Test-Path -Path $responseFile -PathType Leaf) -eq $true) { | |||
Remove-Item -Path $responseFile -Force | |||
} |
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.
Should we just do:
Remove-Item -Path $responseFile -Force -ErrorAction SilentlyContinue
instead of If block?
Will prevent task failure if for some reason Remove-Item fails.
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.
yes. makes more sense.
Tasks/AzureFileCopyV2/task.json
Outdated
@@ -13,7 +13,7 @@ | |||
"version": { | |||
"Major": 2, | |||
"Minor": 0, | |||
"Patch": 2 | |||
"Patch": 7 |
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.
3
Tasks/AzureFileCopyV2/task.loc.json
Outdated
@@ -13,7 +13,7 @@ | |||
"version": { | |||
"Major": 2, | |||
"Minor": 0, | |||
"Patch": 2 | |||
"Patch": 7 |
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.
3
@@ -0,0 +1,99 @@ | |||
{ | |||
"name": "AzureFileCopyV2", |
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.
Are these locked versions, IP scanned versions or are they the latest that were pulled during your build?
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.
all these are versions that were pulled in from the azure-arm-rest module.
} | ||
} | ||
|
||
run() |
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.
await?
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 do not think await is required. the process does not exit until the event loop is empty.
function getResourceGroupNameFromUri(resourceUri: string): string { | ||
if (isNonEmpty(resourceUri)) { | ||
resourceUri = resourceUri.toLowerCase(); | ||
return resourceUri.substring(resourceUri.indexOf("resourcegroups/") + "resourcegroups/".length, resourceUri.indexOf("/providers")); |
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.
if you don't find a match for /prodivers, your substring construction is likely misbehave. Can you add UT around this with boundary conditions and make this string extraction more defensive.
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.
Any reason you did not use match() with regex to extract resourcename between known strings "resourcegroups/" and "/providers"?
|
||
async function run(): Promise<void> { | ||
let tempDirectory: string = tl.getVariable('Agent.TempDirectory'); | ||
let fileName: string = Math.random().toString(36).replace('0.', ''); |
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.
why can't we use the build or release id iteself for unique file name?
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.
Because there can be multiple tasks in the same definition.
let storageAccountName = tl.getInput('StorageAccountRM', true); | ||
var azureEndpoint: AzureEndpoint = await new AzureRMEndpoint(connectedServiceName).getEndpoint(); | ||
const storageArmClient = new armStorage.StorageManagementClient(azureEndpoint.applicationTokenCredentials, azureEndpoint.subscriptionID); | ||
let storageAccount: StorageAccount = await storageArmClient.storageAccounts.get(storageAccountName); |
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.
Test with MSI based endpoint as well.
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.
done
No description provided.