DownloadBuildArtifact: FileContainer #4521
Conversation
| "version": { | ||
| "Major": 0, | ||
| "Minor": 1, | ||
| "Patch": 48 |
There was a problem hiding this comment.
should we start from 0?
There was a problem hiding this comment.
yes, but why patch level 48?
There was a problem hiding this comment.
I was iterating. Does it matter where we start it? I can bump it down to 0.0.1 if it's really important.
| "name": "downloadPath", | ||
| "type": "string", | ||
| "label": "Destination directory", | ||
| "defaultValue": "$(System.ArtifactsDirectory)", |
There was a problem hiding this comment.
RM extension in the agent doesn't set this variable
There was a problem hiding this comment.
That seems like a bug. It is a System.* variable. It also seems problematic to hope all extensions set something like that. Shouldn't the agent just set it since it's a system level thing? The agent should define the a dir then RM extension should respect and use it.
There was a problem hiding this comment.
we should definately default to this though
| "version": { | ||
| "Major": 0, | ||
| "Minor": 1, | ||
| "Patch": 47 |
There was a problem hiding this comment.
why this one is 47 the task.json has 48. :D
| }); | ||
| } | ||
| catch (err) { | ||
| downloadReject(err); |
There was a problem hiding this comment.
do we need to know which file failed?
| // pipe the content to the target | ||
| contentStream.pipe(outputStream); | ||
| contentStream.on('end', () => { | ||
| tl.debug(`Downloaded ${item.relativePath} to ${outputFilename}`); |
There was a problem hiding this comment.
consider: removing outputfilename from this statment might reduce noise.
| createdFolders[folder] = true; | ||
| } | ||
|
|
||
| tl.debug(`Downloading ${item.relativePath} to ${outputFilename}`); |
There was a problem hiding this comment.
Instead of debug, this should probably localized and console.log so there feedback to the web console.
Also consider wrapping the path single quotes to enable easier diagnosing for weirdo trailing space issue or something like that.
There was a problem hiding this comment.
note, i didnt see any non-verbose output anywhere else, but i could have missed something.
There was a problem hiding this comment.
artifact and file level should be info. we might also want to info out course grained % - like every 10% - but probably only if file count > 10?? Granular file level details should be debug.
|
|
||
| // ignore folders | ||
| items = items.filter(item => item.itemType === ContainerItemType.File); | ||
| tl.debug(`Found ${items.length} File items in #/${containerId}/${containerPath}`); |
There was a problem hiding this comment.
File is capitalized (intentional?)
There was a problem hiding this comment.
Yes, intentional, as I'm referring to the itemType value. Items are "File" or "Folder", IIRC. Although I suppose that value isn't relevant to your average build watcher...
| resolve(itemResponse.result); | ||
| } | ||
| else { | ||
| // TODO: decide whether to retry or bail |
There was a problem hiding this comment.
forget about this todo? or will this land in a future PR?
There was a problem hiding this comment.
Future PR. I need to evaluate the different ways things might fail, and a retry strategy.
Tasks/DownloadBuildArtifact/main.ts
Outdated
| } | ||
|
|
||
| main() | ||
| .then((result) => tl.setResult(tl.TaskResult.Succeeded, "")) |
Tasks/DownloadBuildArtifact/main.ts
Outdated
|
|
||
| // get the artifact metadata | ||
| let buildApi = vssConnection.getBuildApi(); | ||
| let artifact = await buildApi.getArtifact(buildId, artifactName, projectId); |
There was a problem hiding this comment.
nice, it looks like we will be able to plug in fileshare easily.
i'm curious what your thoughts are about setting variables. can discuss offline.
| "description": "Download a build artifact.", | ||
| "helpMarkDown": "", | ||
| "category": "Utility", | ||
| "author": "Microsoft Corporation", |
| ], | ||
| "dataSourceBindings": [ | ||
| ], | ||
| "instanceNameFormat": "Download Build Artifact", |
There was a problem hiding this comment.
shouldn't artifact name get formatted into this?
Tasks/DownloadBuildArtifact/main.ts
Outdated
| } | ||
|
|
||
| main() | ||
| .then((result) => tl.setResult(tl.TaskResult.Succeeded, "")) |
There was a problem hiding this comment.
why not wrap main function in a try / catch or wrap main() call in try catch? I'm trying to push a linear C# coding paradigm and avoid .then code unless we have to.
There was a problem hiding this comment.
Copypasta. Sure.
Tasks/DownloadBuildArtifact/main.ts
Outdated
|
|
||
| async function main(): Promise<void> { | ||
| let projectId = tl.getVariable('System.TeamProjectId'); | ||
| let buildId = parseInt(tl.getInput("buildId")); |
There was a problem hiding this comment.
does user get a nice actionable message if not an int?
There was a problem hiding this comment.
of course if not supplied no message but if supplied and I give it string is it silently ignored or does user get actionable error and failure?
Tasks/DownloadBuildArtifact/main.ts
Outdated
| let downloadPath = tl.getPathInput("downloadPath"); | ||
|
|
||
| let accessToken = getAuthToken(); | ||
| let credentialHandler = getBearerHandler(accessToken); |
There was a problem hiding this comment.
Can you run this task locally interactively to test / debug?
In the step by step I encourage that:
https://github.com/Microsoft/vsts-task-lib/blob/master/node/docs/stepbystep.md
In order to pull it off, I have to support a pat (interactive) or bearer (run in a job).
See here. Let me know if you have any better ideas.
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "resolution": "main", | |||
There was a problem hiding this comment.
Ahhh - we want to get away from typings. Can you do npm install @types with this task? Please set the right pattern going forward. We already npm install if package.json exists so I think it should just work.
There was a problem hiding this comment.
it's deprecated. I have an action item to back through our tasks 1:1. After I do that, I want to send as a pattern to other teams to do the same...
|
before coming out of preview we should add UNC support (robocopy). Perhaps Eric should add that in since he's familiar with it on the publish side ... |
| let contentStream = await downloader(item); | ||
|
|
||
| // create the target stream | ||
| let outputStream = fs.createWriteStream(outputFilename); |
There was a problem hiding this comment.
what happens if the file exists?
what if the path exists as a directory, will this produce a weird non-intuitive error?
There was a problem hiding this comment.
if the file exists, it is overwritten. if it's a folder and we're trying to write a file to it, on windows we get EISDIR: illegal operation on a directory, open 'e:\test\downloadbuildartifact\drop\ConsoleApplication1\bin\Release\ConsoleApplication1.pdb'
scottdallamura
force-pushed
the
users/scdallam/downloadbuildartifact
branch
from
e8b0215 to
35a3481
Compare
Tasks/DownloadBuildArtifact/main.ts
Outdated
| let artifact = await buildApi.getArtifact(buildId, artifactName, projectId); | ||
|
|
||
| let providers: ArtifactProvider[] = [ | ||
| new FileContainerProvider() |
There was a problem hiding this comment.
I want a fileshare provider that throws in the downloadArtifact method. I want an clear message that "File share artifacts are not yet supported in the early preview. Coming soon.". I don't want it to fall through and tell the customer file shares aren't supported (confusing and mixed message.
No description provided.