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

Rename fileService to fileStorage, queueService to queueStorage. #4571

Closed
wants to merge 17 commits into from

Conversation

sima-zhu
Copy link
Contributor

FileService->FileStorage
QueueService -> QueueStorage

Replace file playback json files

gapra-msft and others added 11 commits July 12, 2019 11:57
* SAS implementation

* Fixed some minor formatting issues

* Fixed checkstyle problems and test issue
Removes RawClients from Storage Blobs
…to ContainerClient (Azure#4376)

* Removed raw clients

* Added deleteContainer to StorageClient

* Added getAppendBlob with snapshot to ContainerClient
* Finished the restructure, refactor builder. Added sleep in record feature. Linting
Merges AppendBlobClientBuilder, BlobClientBuilder, BlockBlobClientBuilder, and PageBlobClientBuilder into a single builder class BlobClientBuilder. Additionally, JavaDoc comments for the other builder classes, ContainerClientBuilder and StorageAccountClientBuilder, were cleaned up and the way the endpoint is handled in builders was changed.
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

READMEs and some JavaDocs need cleanup

storage/client/queue/README.md Outdated Show resolved Hide resolved
storage/client/queue/README.md Outdated Show resolved Hide resolved
storage/client/queue/README.md Outdated Show resolved Hide resolved
storage/client/queue/README.md Show resolved Hide resolved
storage/client/queue/README.md Outdated Show resolved Hide resolved
storage/client/queue/README.md Outdated Show resolved Hide resolved
@@ -1,42 +1,42 @@
{
"networkCallRecords" : [ {
"Method" : "PUT",
"Uri" : "https://sima.file.core.windows.net/dirsharename/directory863334?restype=directory",
"Uri" : "https://sima.file.core.windows.net/dirsharename/directory06962b?restype=directory",
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have checked-in tests that reference a specific dev's account. Can you create a tracking work item to move these Uri's to a team account versus an individual account?
/cc @weshaggard

Copy link
Member

Choose a reason for hiding this comment

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

Yes we should be setting up these resources more generally in our test resources subscription. @danieljurek I believe you have some docs talking about that correct?

Copy link
Contributor Author

@sima-zhu sima-zhu Jul 25, 2019

Choose a reason for hiding this comment

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

This would be an easy change. We used azstoragesdkaccount for testing. Will use it for now.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have switched the azstoragesdkaccount team account under Azure SDK Test Resource subscription already.

@joshfree joshfree added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jul 25, 2019
@sima-zhu sima-zhu changed the base branch from storage-post-review1-dev to master July 26, 2019 18:54
@sima-zhu sima-zhu requested a review from alzimmermsft July 26, 2019 23:41
@@ -1,42 +1,42 @@
{
"networkCallRecords" : [ {
"Method" : "PUT",
"Uri" : "https://sima.file.core.windows.net/dirsharename/directory863334?restype=directory",
"Uri" : "https://azstoragesdkaccount.file.core.windows.net/dirsharename/directory149240?restype=directory",
Copy link
Member

Choose a reason for hiding this comment

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

My comment came in rather late so sorry to keep harping on this if you've already fixed it and just haven't pushed yet :P

I looked up the azstoragesdkaccount a few minutes ago, it's not listed in the test resources subscription. Please use a storage account in the test resources subscription.

More info here: https://dev.azure.com/azure-sdk/internal/_wiki/wikis/internal.wiki?pagePath=%2FData%20Plane%20Quality%20Program%2FTesting%20Guidelines&pageId=51&wikiVersion=GBwikiMaster&anchor=best-practices

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 am switching the account to Azure SDK Test Resources. This is the one created by our team and only used for java storage testing.

Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

LGTM, just need to fix that merge conflixt

@@ -441,6 +441,8 @@
<Class name="com.azure.storage.blob.BlobInputStream"/>
<Class name="com.azure.storage.blob.BlobOutputStream"/>
<Class name="com.azure.storage.queue.QueueServiceClient"/>
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Missed a merged conflict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with the fix. Thanks!

Copy link
Member

@jaschrep-msft jaschrep-msft left a comment

Choose a reason for hiding this comment

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

Caught a naming concern but good otherwise.

@@ -137,11 +137,11 @@ public ShareClient getShareClient(String shareName) {
*
* <p>Clear CORS in the File service</p>
*
* {@codesnippet com.azure.storage.file.fileServiceClient.setProperties#fileServiceProperties.clearCORS}
* {@codesnippet com.azure.storage.file.fileStorageClient.setProperties#fileServiceProperties.clearCORS}
Copy link
Member

Choose a reason for hiding this comment

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

Should be fileStorageProperties

Fix elsewhere as needed.

Copy link
Member

Choose a reason for hiding this comment

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

I made this comment a while ago before finishing the review. The more I look at it, the more I'm not sure what the actual name should be. @rickle-msft do you have ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is auto-generated. To rename it, we need change swagger.

@sima-zhu
Copy link
Contributor Author

Split the switch account stuff to another PR #4710.

This is blocked by the board decision about naming.

@sima-zhu
Copy link
Contributor Author

Closed as it is not top priority for preview 3

@sima-zhu sima-zhu closed this Aug 14, 2019
@sima-zhu sima-zhu deleted the storage-service-rename branch February 23, 2021 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants