Enable download blob and zip within vscode for persicope uploaded logs to storage. #75
Conversation
| @@ -214,6 +216,105 @@ export async function generateDownloadableLinks( | |||
| } | |||
| } | |||
|
|
|||
| export async function downloadableLinks( | |||
There was a problem hiding this comment.
This is a big function and the name and type don't really help me understand it. Does it return a list of links from which we will later download? Consider typing it as returning Promise<string[]> (and renaming it getDownloadLinks). Does it perform a download itself? Consider naming it downloadWhateverItDownloads (and returning Promise<void> or Promise<Errorable<null>>. Does it do something else? Find a name that expresses that, and consider what it is that it returns.
| ); | ||
|
|
||
| // Hide this all under single pupose funct which lik elike : getZipDir or get me dirname | ||
| const containerClient = blobServiceClient.getContainerClient(containerName); |
There was a problem hiding this comment.
The comment above this line is a good one. I don't think anything from this half of the function is used in the second half of the function. So move everything above here to its own function called e.g. getZipContainer or whatever expresses the intent.
| if (item.kind === "prefix") { | ||
| listCurrentUploadedFolders.push(item.name); | ||
| } | ||
| } |
There was a problem hiding this comment.
You may be able to replace this loop with something like const currentFolders = (await containerClient.list(...)).filter(item => item.kind === prefix).map(item => item.name) but I am not sure if that enumeration is async - in that case filter and map may not be available.
| } | ||
|
|
||
| // Sort and get the latest uploaded folder under the container used by periscope. | ||
| // const periscopeHtmlInterfaceList = []; |
There was a problem hiding this comment.
Tidy commented-out code here and throughout.
|
|
||
| // Sort and get the latest uploaded folder under the container used by periscope. | ||
| // const periscopeHtmlInterfaceList = []; | ||
| // jszip for zipping |
There was a problem hiding this comment.
Does this comment add anything?
| // and extract *.zip files which are individual node logs. | ||
| const latestBlobUploadedByPeriscope = blob.name.indexOf(listCurrentUploadedFolders.sort().reverse()[0]); | ||
|
|
||
| if (latestBlobUploadedByPeriscope !== -1 && path.extname(blob.name) !== '.zip' && blob.name.indexOf(nodeName) !== -1) { |
There was a problem hiding this comment.
Again consider includes instead of indexOf(...) === -1.
| if (latestBlobUploadedByPeriscope !== -1 && path.extname(blob.name) !== '.zip' && blob.name.indexOf(nodeName) !== -1) { | ||
| // Get a block blob client | ||
| // https://docs.microsoft.com/en-us/azure/storage/blobs/storage-quickstart-blobs-nodejs-legacy#download-a-blob | ||
| const blockBlobClient = containerClient.getBlockBlobClient(blob.name); |
There was a problem hiding this comment.
Could everything from here to the bottom of the loop be a well-named helper function? It seems like a downloadBlobToFile kind of chunk.
There was a problem hiding this comment.
Oh looking below maybe this isn't downloading the blob to a file.
| // Get blob content from position 0 to the end | ||
| // In Node.js, get downloaded data by accessing downloadBlockBlobResponse.readableStreamBody | ||
| const downloadBlockBlobResponse = await blockBlobClient.download(0); | ||
| console.log('\nDownloaded blob content...'); |
There was a problem hiding this comment.
Users won't see console.log. Consider using a progress notification if this is going to take a while.
|
|
||
| function writeZipToDownloadLocation(nodeName: string, zip: JSZip) { | ||
| const downloadsFolder = require('downloads-folder'); | ||
| console.log('\nDownloads folder is...'); |
| if (message.command === "downloadLink") { | ||
| // Generate link mechanism is in place due to current behaviour of the aks-periscope tool. (which seems by design for now) | ||
| // more detail here: https://github.com/Azure/aks-periscope/issues/30 | ||
| await longRunning(`Downloading ${message.text} logs in download location.`, |
There was a problem hiding this comment.
What happens when they are downloaded? Do you open the folder for the user?
|
Did you consider simply dropping support for zipping the outputs in VSCode? This is a convenience function that could be left up to user to do afterwards if they wished - and as you mention az-cli doesn't make use of the .zip, so there isn't a conventional periscope experience to uphold here or anything |
Yep, we considered all options but as vscode user-experience the current U/X experience is the best solution for vscode ecosystem for the use of this tool. For The If we drop support for |
|
💡 Thank you so much for discussion, this is no longer necessary, further discussion and whole update reside here: #72 , periscope tool kept the zip as part of their output contract. Thanks 🙏 |
This PR caters need to handle the download and zip the dowloaded folder into the download folder location.
Good amount of detail here: #72
Currently, the download is achieved by user clicking on hyperlink which redirect user to the browser based experienced, but in the new way the experience the user will never need to leave the vscode eco-system.
These changes also are needed because the periscope clean-up PR which will result in clearing this
*.zipfile because it is not formed correctly and not needed. I've checked theaz-cliimplementation and essentially theaz-clialso don't use the*.zipfrom periscope either, so this implementation makes more aligned to other tools as well.This will also decouples the dependency on the
*.ziplink we generate which is uploaded by the periscope tool.Along with various advantages we do have much better control over the zip structure and hierarchy.
Update: 22nd June:
Please refer more as to how thought process for this work evolved: #72
Introducing these changes in vscode is not that straight forward as it sounded: some key complications:
link to browserwill be gone, and replaced with only message driven approach. ( this is something which seems inevitable)vscodechanges need to go first before any tool related changes to removezipupload is removed. (This will take time to re-group with team - and is it worth it to break the tool to vscode contract effort.)