Update ⬆️ to newest Azure Storage SDK for blobs 🗄 #513

wwlorey · 2019-11-18T23:59:16Z

Fixes partially: #328

ejizba · 2019-11-20T19:18:22Z

src/azureStorageExplorer/blobContainers/BlobDirectoryTreeItem.ts

@@ -1,218 +0,0 @@
-/*---------------------------------------------------------------------------------------------


This file was one of the few files that is named correctly (the name should match the class). Also ideally file name changes happen in their own PR so that it doesn't mess with the diff

Should I wait to rename those files in a separate PR?

Yes please :) Also feel free to bother someone to review that PR as soon as you submit it since it should have no functional changes (aka easy to review) and so that we can avoid a ton of merge conflicts

ejizba · 2019-11-20T19:21:08Z

src/azureStorageExplorer/blobContainers/blobFileHandler.ts

-            });
-
+        const blockBlobClient = treeItem.root.createBlockBlobClient(treeItem.container.name, treeItem.fullPath);
+        await blockBlobClient.downloadToFile(filePath);


Can we keep the logic that displays progress?

ejizba · 2019-11-20T19:41:44Z

package.json

@@ -195,7 +195,7 @@
 			},
 			{
 				"command": "azureStorage.createBlockTextBlob",
-				"title": "Create Empty Text Blob...",


I snuck in the fix for text/plain content type defaults for blobs. Now it guesses the content type when creating/updating and respects the existing content type if it exists. I'll address the fix for file shares separately.

Can we unsneak the fix and do it separately? 🙃

ejizba · 2019-11-20T19:43:12Z

src/azureStorageExplorer/blobContainers/blobContainerGroupNode.ts

+        let response = blobServiceClient.listContainers().byPage({ continuationToken, maxPageSize });
+
+        // tslint:disable-next-line: no-unsafe-any
+        return (await response.next()).value;


What's the "unsafe-any" here? Is the package missing correct types?

As far as I can tell, types aren't provided here :(

For some context, here's an updated version with more typing:

let response: AsyncIterableIterator<azureStorageBlob.ServiceListContainersSegmentResponse> = blobServiceClient.listContainers().byPage({ continuationToken, maxPageSize }); // tslint:disable-next-line:no-any let next: IteratorResult<azureStorageBlob.ServiceListContainersSegmentResponse, any> = await response.next(); // tslint:disable-next-line: no-unsafe-any return next.value;

ejizba · 2019-11-20T19:47:40Z

src/azureStorageExplorer/blobContainers/blobUtils.ts

+        return {
+            metadata: existingProperties.metadata,
+            blobHTTPHeaders: {
+                blobCacheControl: existingProperties.cacheControl,


Not a huge fan of copying out each property here. Seems like we could get into problems if they ever add properties. Can we just return existingProperties as is? And only modify blobContentMD5?

Yeah this isn't ideal. The reason I enumerated everything is because the property names don't match up. Do you know if there's a cleaner way to take care of this?

Per offline conversation: reading properties from some resource returns a lot of info we don't explicitly need when setting new properties. Also the BlobGetPropertiesResponse object structure is different from BlockBlobUploadOptions needed when uploading resources

ejizba · 2019-11-20T19:49:16Z

src/azureStorageExplorer/storageAccounts/storageAccountNode.ts

+
+    private createBlobServiceClient(): azureStorageBlob.BlobServiceClient {
+        const credential = new azureStorageBlob.StorageSharedKeyCredential(this.storageAccount.name, this.key.value);
+        return new azureStorageBlob.BlobServiceClient(`https://${this.storageAccount.name}.blob.core.windows.net`, credential);


I don't think this'll work with sovereign accounts because the url will be different. Check out this.storageAccount.primaryEndpoints.blob which should have the url you need without hard-coding anything

ejizba · 2019-11-20T19:49:36Z

src/azureStorageExplorer/storageAccounts/storageAccountNode.ts

+            createBlockBlobClient: (containerName: string, blobName: string) => {
+                const blobContainerClient = this.createBlobContainerClient(containerName);
+                return blobContainerClient.getBlockBlobClient(blobName);
+            },
            createBlobService: () => {


Can we get rid of the old createBlobService now?

ejizba · 2019-11-20T19:50:54Z

src/azureStorageExplorer/storageAccounts/storageAccountNode.ts

+            createBlockBlobClient: (containerName: string, blobName: string) => {
+                const blobContainerClient = this.createBlobContainerClient(containerName);
+                return blobContainerClient.getBlockBlobClient(blobName);
+            },
            createBlobService: () => {
                return azureStorage.createBlobService(this.storageAccount.name, this.key.value, this.storageAccount.primaryEndpoints.blob).withFilter(new azureStorage.ExponentialRetryPolicyFilter());
            },


Is there an analagous ExponentialRetryPolicyFilter for the new sdk? This was a pretty important fix: #380

That retry behavior appears to be built in. The default is an exponential retry policy. Those config settings are under retryOptions of type StorageRetryOptions

nturinski · 2019-11-20T19:44:05Z

src/azureStorageExplorer/IStorageRoot.ts

 import * as azureStorage from "azure-storage";
 import { ISubscriptionContext } from "vscode-azureextensionui";
 import { StorageAccountWrapper } from "../components/storageWrappers";

 export interface IStorageRoot extends ISubscriptionContext {
    storageAccount: StorageAccountWrapper;
-    createBlobService(): azureStorage.BlobService;
+    createBlobServiceClient(): azureStorageBlob.BlobServiceClient;
+    createBlobContainerClient(containerName: string): azureStorageBlob.ContainerClient;


These functions seem like they shouldn't be here (except createBlobServiceClient) They all require specific information regarding the container and blob names, but this seems more like a general-purpose class for just leveraging the subscription information.

Would it be better to move them to blobUtils?

Yeah, I think so. Eventually we'll have to have similar functions for File, Queue, and Table as well and this file seems like it'd get kind of messy in functionality.

nturinski · 2019-11-20T23:43:35Z

src/azureStorageExplorer/blobContainers/blobContainerGroupNode.ts

+
+        let containersResponse = await this.listContainers();
+        let createdContainer: azureStorageBlob.ContainerItem | undefined;
+        for (let container of containersResponse.containerItems) {


Does containerClient.exist() not work for this?

containerClient.exists() only returns a bool and I needed a reference to the container here

nturinski · 2019-11-21T00:23:15Z

src/azureStorageExplorer/blobContainers/blobDirectoryNode.ts

+    public static contextValue: string = 'azureBlobDirectory';
+    public contextValue: string = 'azureBlobDirectory';
+
+    public fullPath: string = path.posix.join(this.parentPath, this.directory.name);


Is the fullPath the path on Azure? I'm wondering if we need to use posix for joining the paths since that would be using the OS of the user, not the OS of the storage account for joining paths.

It's the blob's path on the user OS

nturinski · 2019-11-21T00:32:13Z

src/azureStorageExplorer/blobContainers/blobDirectoryNode.ts

+    public contextValue: string = 'azureBlobDirectory';
+
+    public fullPath: string = path.posix.join(this.parentPath, this.directory.name);
+    public dirPath: string = `${this.fullPath}/`;


So if this is the dirPath on the local machine, then we need to check the OS to know which separator to use. I think there's an os.separator for that.

nturinski · 2019-11-21T00:43:47Z

src/azureStorageExplorer/blobContainers/blobUtils.ts

-    await createBlockBlobFromLocalFile(blobName, containerName, root, filePath, createOptions);
+export async function loadMoreBlobChildren(parent: BlobContainerTreeItem | BlobDirectoryTreeItem, continuationToken?: string): Promise<{ children: AzExtTreeItem[], continuationToken?: string }> {
+    const dirPath = parent instanceof BlobDirectoryTreeItem ? parent.dirPath : '';
+    const listOptions = dirPath ? { prefix: dirPath } : undefined;


Does having { prefix: undefined } break the listOptions?

Prefix is OK to be undefined, I'll simplify that logic

Forgot to do this originally 😰

nturinski · 2019-11-25T17:26:26Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

@@ -452,19 +453,41 @@ export class BlobContainerTreeItem extends AzureParentTreeItem<IStorageRoot> imp
    }

    private async uploadFileToBlockBlob(filePath: string, blobPath: string, suppressLogs: boolean = false): Promise<void> {
-        let blobFriendlyPath = `${this.friendlyContainerName}/${blobPath}`;
+        const blobFriendlyPath = `${this.friendlyContainerName}/${blobPath}`;


Could you add typings wherever you can? I'm sure we'll inevitably turn on that tslint rule eventually and it'd just be easier to do it as we go

nturinski · 2019-11-25T17:26:55Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

+                    blobContentType: mime.getType(blobPath) || undefined
+                },
+                onProgress: (progress: TransferProgressEvent) => {
+                    if (lastUpdated + updateTimerMs < Date.now()) {


Write a comment explaining why you need to debounce the progress.report.

nturinski · 2019-11-25T17:27:42Z

src/azureStorageExplorer/blobContainers/blobFileHandler.ts

+        await window.withProgress({ title: `Downloading ${treeItem.blob.name}`, location: ProgressLocation.Notification }, async (notificationProgress) => {
+            await blockBlobClient.downloadToFile(filePath, undefined, undefined, {
+                onProgress: (progress) => {
+                    if (lastUpdated + updateTimerMs < Date.now()) {


Since you're using this in two different spots, it probably makes sense to extract this to another file and just reuse it.

nturinski · 2019-11-26T19:32:29Z

src/azureStorageExplorer/blobContainers/blobContainerGroupNode.ts

+        }
+
+        if (!createdContainer) {
+            throw new Error(`Could not find container ${name}`);


Rather not could not find, I think the error should indicate that there was an error in the creation itself. I'd assume if we can't find it, it means it wasn't created properly?

nturinski · 2019-11-26T19:38:01Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

    }

    public async loadMoreChildrenImpl(clearCache: boolean): Promise<AzExtTreeItem[]> {
-        const result: AzExtTreeItem[] = [];
+        let result: AzExtTreeItem[] = [];


Why did you change this from const to let?

Not sure why I did that 🤦‍♂️ I'll change it back

nturinski · 2019-11-26T19:41:51Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

-    // tslint:disable-next-line:promise-function-async // Grandfathered in
-    listBlobs(currentToken: azureStorage.common.ContinuationToken, maxResults: number = 50): Promise<azureStorage.BlobService.ListBlobsResult> {
-        return new Promise<azureStorage.BlobService.ListBlobsResult>((resolve, reject) => {
-            console.log(`${new Date().toLocaleTimeString()}: Querying Azure... Method: listBlobsSegmentedWithPrefix blobContainerName: "${this.container.name}" prefix: ""`);


Do you know if you were supposed to remove this output with this PR?

Some SDK calls had those logs and others didn't so I removed them when I consolidated functions. Do you think that would be good to add back in?

I'm not sure how busy the output looks today. When does it output? I don't see it when I expand a blobContainer tree item.

But also, if you do add it back in, you can leverage ext.outputChannel.appendLog() to have the timestamp prefixed to the output message (rather than manually inserting it as it was coded before)

That outputs during a static website deployment when all blobs to delete are found. I can add it back in

nturinski · 2019-11-26T19:44:52Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

-            blobs.push(...result.entries);
-            currentToken = result.continuationToken;
+
+            const containerClient: azureStorageBlob.ContainerClient = createBlobContainerClient(this.root, this.container.name);


I think this can go outside the while loop so you're not creating a new containerClient with each iteration

nturinski · 2019-11-26T19:49:37Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

-        let currentToken: azureStorage.common.ContinuationToken | undefined;
-        let blobs: azureStorage.BlobService.BlobResult[] = [];
+        let currentToken: string | undefined;
+        let blobs: azureStorageBlob.BlobItem[] = [];


nit: This could be const too since it's never reassigned (probably should enable that tslint rule)

nturinski · 2019-11-26T19:51:48Z

src/azureStorageExplorer/blobContainers/blobContainerNode.ts

-                });
-            });
+            const containerClient: azureStorageBlob.ContainerClient = createBlobContainerClient(this.root, this.container.name);
+            await containerClient.delete();
        } else {


This can be removed now. ext.ui.showWarningMessage handles userCancelled errors for us.

https://github.com/microsoft/vscode-azuretools/blob/master/ui/src/AzureUserInput.ts#L86-L87

nturinski · 2019-11-26T22:29:24Z

src/azureStorageExplorer/blobContainers/blobDirectoryNode.ts

+        let errors: boolean = false;
+
+        // tslint:disable-next-line: strict-boolean-expressions
+        while (dirPath) {


I'm not sure if this was like this before or not, but why use a while loop instead of just using a for loop that iterates over dirPaths?

This function wasn't modified by the SDK changes. It looks like they went with a while loop to handle folders & nested folders more easily

Size was being read from a non-existent block blob

nturinski · 2019-12-03T22:21:44Z

src/azureStorageExplorer/blobContainers/blobDirectoryNode.ts

+    private _continuationToken: string | undefined;
+
+    public static contextValue: string = 'azureBlobDirectory';
+    public contextValue: string = 'azureBlobDirectory';


Should be able to just change this to = BlobDirectoryTreeItem.contextValue rather than having repeated strings.

nturinski · 2019-12-04T00:37:43Z

src/azureStorageExplorer/blobContainers/blobUtils.ts

+}
+
+export class TransferProgressState {
+    public readonly updateTimerMs: number = 200;


This should also be a property that you can set in the constructor. I can see instances where people would only want the % to update every second or something.

nturinski · 2019-12-04T00:41:22Z

src/azureStorageExplorer/blobContainers/blobUtils.ts

+    public readonly updateTimerMs: number = 200;
+
+    constructor(
+        public percentage: number = 0,


I feel like that these should all not be in the constructor actually. I'm not sure what use-case you would have where you create a new TransferProgressState with these already set.

I think it might make sense to make handleTransferProgress a function of TransferProgressState and make all of those properties private so that you ensure nothing else is setting the properties except handleTransferProgress.

nturinski · 2019-12-04T00:45:41Z

src/azureStorageExplorer/storageAccounts/storageAccountNode.ts

-            ErrorDocument404Path: errorDocument404Path,
-            IndexDocument: indexDocument
+            // tslint:disable-next-line: strict-boolean-expressions
+        }) || undefined;


Why was this logic added?

Per offline discussion: errorDocument404Path can't be an empty string, so default to undefined. Also value passed into showInputBox must be of type string

William Lorey added 3 commits November 18, 2019 15:26

Update to newest blob SDK

73adf4e

Pull in master changes

4f16d46

2?

5786ce0

wwlorey requested a review from a team as a code owner November 18, 2019 23:59

ejizba reviewed Nov 20, 2019

View reviewed changes

nturinski reviewed Nov 21, 2019

View reviewed changes

William Lorey added 4 commits November 21, 2019 09:57

Unsneak text/plain fix 🤫

1b96af3

Update tests

d0ef7ca

Forgot to do this originally 😰

Add progress logic

9c6108e

Support better blob URL

fac722a

nturinski reviewed Nov 25, 2019

View reviewed changes

William Lorey added 4 commits November 25, 2019 09:56

Add OS specific separator & simplify prefix logic

e3afada

Add handleTransferProgress

70c825a

Remove client functions from IStorageRoot

3d3211a

Type it up

405084b

nturinski reviewed Nov 26, 2019

View reviewed changes

Fix 'create container' error message

4da99e8

nturinski reviewed Nov 26, 2019

View reviewed changes

const

e548f9e

nturinski reviewed Nov 26, 2019

View reviewed changes

William Lorey added 2 commits November 26, 2019 11:51

Update vars in listAllBlobs

f10cd29

Remove UserCancelledError

f3a8525

nturinski reviewed Nov 26, 2019

View reviewed changes

Add console log and fix size check

4bce181

Size was being read from a non-existent block blob

nturinski reviewed Dec 3, 2019

View reviewed changes

azureBlobDirectory

400fa18

nturinski reviewed Dec 4, 2019

View reviewed changes

Restructure TransferProgress

8f774a2

nturinski approved these changes Dec 4, 2019

View reviewed changes

wwlorey merged commit 6021feb into master Dec 4, 2019

wwlorey deleted the wwl/blob-sdk branch December 4, 2019 01:27

wwlorey mentioned this pull request Dec 19, 2019

Don't display progress notifications when uploading blobs #540

Merged

github-actions bot locked and limited conversation to collaborators Feb 10, 2021

		@@ -1,218 +0,0 @@
		/*---------------------------------------------------------------------------------------------

Update ⬆️ to newest Azure Storage SDK for blobs 🗄 #513

Update ⬆️ to newest Azure Storage SDK for blobs 🗄 #513

Conversation

wwlorey commented Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwlorey Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturinski Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwlorey commented Nov 18, 2019 •

edited

Loading

wwlorey Nov 25, 2019 •

edited

Loading

nturinski Dec 4, 2019 •

edited

Loading