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.

Sign up for GitHub

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

Jump to bottom

[Storage] Rename storage-file to storage-file-share #5734

Merged
merged 3 commits into from
Oct 24, 2019
Merged

[Storage] Rename storage-file to storage-file-share #5734

merged 3 commits into from
Oct 24, 2019

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Oct 23, 2019 •
edited
Loading

As per #5705, the storage-file package should be renamed to storage-file-share.

Since this involves a lot of moving parts, this PR only focuses on the rename and not the other API changes listed in #5705. They will follow after this PR is merged

@ramya-rao-a
Copy link
Contributor Author

@KarishmaGhiya Can you review the parts related to CI and let me know if anything else needs to be done to update the existing infrastructure for storage-file to work for the new storage-file-share

@XiaoningLiu XiaoningLiu added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 23, 2019
@jiacfan
Copy link
Member

jiacfan commented Oct 23, 2019

Should we add readme for the package name change?

i.e. storage-file -> storage-file-share,

Considering, this might be a big change user might face.

@XiaoningLiu
Copy link
Member

One of the bigest PR so far. Still checking. File names, class/method/parameter names, JSDoc, documents, samples should update

KarishmaGhiya
KarishmaGhiya approved these changes Oct 23, 2019
View reviewed changes
Copy link
Member

@KarishmaGhiya KarishmaGhiya left a comment

Choose a reason for hiding this comment

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

The CI changes look good to me

HarshaNalluru
HarshaNalluru reviewed Oct 23, 2019
View reviewed changes
sdk/storage/storage-file-share/README.md
@@ -77,27 +77,27 @@ There are differences between Node.js and browsers runtime. When getting started
The preferred way to install the Azure File Storage client library for JavaScript is to use the npm package manager. Simply type the following into a terminal window:
Copy link
Member

Choose a reason for hiding this comment

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

Still Azure File Storage client library ?

@HarshaNalluru
Copy link
Member

One of the bigest PR so far. Still checking. File names, class/method/parameter names, JSDoc, documents, samples should update

@XiaoningLiu, this PR is only to rename storage-file to storage-file-share. Classes and other names will be part of a separate PR.

@ramya-rao-a
Copy link
Contributor Author

@jiacfan

Should we add readme for the package name change?

Do you mean a changelog update?
Or a line in the readme talking about the name change?

@jiacfan
Copy link
Member

jiacfan commented Oct 24, 2019

What about add some words in the readme talking about the name change? Previously we have "@azure/storage-file@next under Client: July 2019 Preview", and may be we need mention the change, besides changing @azure/storage-file@next directly to @azure/storage-file-share, otherwise it might be strange for users use @azure/storage-file@next.

In reply to: 545694280 [](ancestors = 545694280)

@ramya-rao-a
Copy link
Contributor Author

@jiacfan I need to check with Storage team and Scott to figure out the exact messaging we want to have here for the rename. I have logged #5769 to track this and we will get to it in the next few days.

Meanwhile, this PR is the first step towards the move.
It contains folder rename, package rename, CI infrastructure updates, link updates in all .md files.

Once this PR is merged, we will send another PR for the class name changes and then handle #5769

@XiaoningLiu
Copy link
Member

@jiacfan I need to check with Storage team and Scott to figure out the exact messaging we want to have here for the rename. I have logged #5769 to track this and we will get to it in the next few days.

Meanwhile, this PR is the first step towards the move.
It contains folder rename, package rename, CI infrastructure updates, link updates in all .md files.

Once this PR is merged, we will send another PR for the class name changes and then handle #5769

We can add npm package deprecate warning, so every one install @azure/storage-file will get warning to switch to @azure/storage-file-share

XiaoningLiu
XiaoningLiu approved these changes Oct 24, 2019
View reviewed changes
Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

Generally look good. Hard to view every file. Like to further review in following rename steps by Ramya.

@ramya-rao-a
Copy link
Contributor Author

Rebased this branch from master and re-ran all the CI tests
The one for Windows_Node8 got cancelled as it ran past its 60min time slot.
All others are green, so going ahead with the merge.

@ramya-rao-a ramya-rao-a merged commit 9c84d46 into master Oct 24, 2019
@ramya-rao-a ramya-rao-a deleted the file-share branch October 24, 2019 20:46
This was referenced Oct 25, 2019
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HarshaNalluru HarshaNalluru HarshaNalluru left review comments

@XiaoningLiu XiaoningLiu XiaoningLiu approved these changes

@KarishmaGhiya KarishmaGhiya KarishmaGhiya approved these changes

@danieljurek danieljurek Awaiting requested review from danieljurek

@jeremymeng jeremymeng Awaiting requested review from jeremymeng

@jiacfan jiacfan Awaiting requested review from jiacfan

@mikeharder mikeharder Awaiting requested review from mikeharder

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@vinjiang vinjiang Awaiting requested review from vinjiang

@weshaggard weshaggard Awaiting requested review from weshaggard

Assignees
No one assigned
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ramya-rao-a @jiacfan @XiaoningLiu @HarshaNalluru @KarishmaGhiya