Skip to content
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

Update ⬆️ to newest Azure Storage SDK for file shares 🗂 #558

Merged
merged 5 commits into from
Jan 11, 2020

Conversation

wwlorey
Copy link
Contributor

@wwlorey wwlorey commented Jan 8, 2020

Fixes #328

@wwlorey wwlorey requested a review from a team as a code owner January 8, 2020 23:30
src/tree/StorageAccountTreeItem.ts Outdated Show resolved Hide resolved
src/tree/fileShare/DirectoryTreeItem.ts Outdated Show resolved Hide resolved
src/tree/fileShare/FileShareGroupTreeItem.ts Outdated Show resolved Hide resolved
src/tree/fileShare/FileShareGroupTreeItem.ts Outdated Show resolved Hide resolved
src/tree/fileShare/FileShareGroupTreeItem.ts Outdated Show resolved Hide resolved
test/storageAccountActions.test.ts Outdated Show resolved Hide resolved
const fileClient: azureStorageShare.ShareFileClient = createFileClient(root, shareName, directoryPath, name);
let propertiesResult: azureStorageShare.FileGetPropertiesResponse = await fileClient.getProperties();
let options: azureStorageShare.FileCreateOptions = {};
options.fileHttpHeaders = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I missed the conversation when I was OOF on Thanksgiving on why we're doing it like this. If it's a limitation of the new sdk, I think we should file an issue because the old sdk handled this much better. Does the Storage Explorer team use this version of the sdk and do they do the same thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd like to know what is missing from the new sdk. /cc @XiaoningLiu

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremymeng It would be easier to update a file's contents while maintaining its metadata & properties if FileCreateOptions and FileGetPropertiesResponse had the same object structure. Either that or an update function that uploaded a file's new contents while preserving properties.

Today we have to read the existing properties/metadata and include them when updating a file via uploadFile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wwlorey agree. Thanks for the feedback! I logged an issue at Azure/azure-sdk-for-js#7097

src/utils/fileUtils.ts Outdated Show resolved Hide resolved
src/utils/directoryUtils.ts Outdated Show resolved Hide resolved
@wwlorey wwlorey mentioned this pull request Jan 10, 2020
});
});
// tslint:disable-next-line: no-unsafe-any
let responseValue: azureStorageShare.DirectoryListFilesAndDirectoriesSegmentResponse = (await response.next()).value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Not sure I see the point in returning { files: azureStorageShare.FileItem[], directories: azureStorageShare.DirectoryItem[], continuationToken: string } when you could just return DirectoryListFilesAndDirectoriesSegmentResponse directly and it has all the same information

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just liked saving segment.fileItems as files, etc. It seemed cleaner for functions that call listFilesInDirectory to deal with

@wwlorey wwlorey merged commit 28e52c5 into master Jan 11, 2020
@wwlorey wwlorey deleted the wwl/new-file-share-sdk branch January 11, 2020 00:32
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch over to azure-sdk-for-js
3 participants