feat: add ability to upload entire directory, add samples for upload … #2118

ddelgrosso1 · 2022-12-13T19:40:40Z

…directory / download folder

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #2114 🦕

…directory / download folder

snippet-bot · 2022-12-13T19:40:45Z

Here is the summary of changes.

You are about to add 2 region tags.

samples/downloadFolderWithTransferManager.js:24, tag storage_download_folder_transfer_manager
samples/uploadDirectoryWithTransferManager.js:24, tag storage_upload_directory_transfer_manager

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

shaffeeullah · 2022-12-14T16:14:31Z

samples/downloadFolderWithTransferManager.js

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * @experimental


is this where this tag belongs?

Looking at https://sapui5.hana.ondemand.com/sdk/docs/topics/eeaa5de14e5f4fc1ac796bc0c1ada5fb.html it only notes Classifies an entity that is not ready for production use yet, but available for testing purposes. I stuck it in this same location for all other transfer manager samples.

shaffeeullah · 2022-12-14T16:16:28Z

samples/downloadFolderWithTransferManager.js

+ * @experimental
+ */
+
+// sample-metadata:


does this comment belong here? it's outside the region tags and im not entirely sure the purpose. do we have a similar comment in other samples? (i only checked 2 and I didnt see it)

I noticed this in some of the newer samples that had been written, for example createBucketWithDualRegion.

shaffeeullah · 2022-12-14T16:18:00Z

samples/uploadDirectoryWithTransferManager.js

+ * @experimental
+ */
+
+// sample-metadata:


same questions here

shaffeeullah · 2022-12-14T16:18:37Z

samples/uploadDirectoryWithTransferManager.js

+  // Creates a client
+  const storage = new Storage();
+
+  // Creates a transfer manager instance


should we use the word client instead of instance here to be consistent?

I can change this for consistency as I think most samples use the word client for a new storage instance. I was just trying to be more 'technically' accurate.

danielbankhead

Looks good to me, very clean. One minor, optional feature request. I also like the async generators too.

danielbankhead · 2022-12-14T20:48:13Z

samples/uploadDirectoryWithTransferManager.js

+  const storage = new Storage();
+
+  // Creates a transfer manager instance
+  const transferManager = new TransferManager(storage.bucket(bucketName));


What are your thoughts on accepting a string in TransferManager's constructor and creating a bucket from it?

I like this idea. Let me tackle it in a separate PR as to not muddy the waters on this one.

danielduhh · 2022-12-14T21:24:21Z

README.md

@@ -133,6 +133,7 @@ Samples are in the [`samples/`](https://github.com/googleapis/nodejs-storage/tre
 | Download File | [source code](https://github.com/googleapis/nodejs-storage/blob/main/samples/downloadFile.js) | [![Open in Cloud Shell][shell_img]](https://console.cloud.google.com/cloudshell/open?git_repo=https://github.com/googleapis/nodejs-storage&page=editor&open_in_editor=samples/downloadFile.js,samples/README.md) |
 | Download a File in Chunks Utilzing Transfer Manager | [source code](https://github.com/googleapis/nodejs-storage/blob/main/samples/downloadFileInChunksWithTransferManager.js) | [![Open in Cloud Shell][shell_img]](https://console.cloud.google.com/cloudshell/open?git_repo=https://github.com/googleapis/nodejs-storage&page=editor&open_in_editor=samples/downloadFileInChunksWithTransferManager.js,samples/README.md) |
 | Download File Using Requester Pays | [source code](https://github.com/googleapis/nodejs-storage/blob/main/samples/downloadFileUsingRequesterPays.js) | [![Open in Cloud Shell][shell_img]](https://console.cloud.google.com/cloudshell/open?git_repo=https://github.com/googleapis/nodejs-storage&page=editor&open_in_editor=samples/downloadFileUsingRequesterPays.js,samples/README.md) |
+| Download Folder With Transfer Manager | [source code](https://github.com/googleapis/nodejs-storage/blob/main/samples/downloadFolderWithTransferManager.js) | [![Open in Cloud Shell][shell_img]](https://console.cloud.google.com/cloudshell/open?git_repo=https://github.com/googleapis/nodejs-storage&page=editor&open_in_editor=samples/downloadFolderWithTransferManager.js,samples/README.md) |


Sort of a nit, but both directory and folder are used interchangeably throughout the PR. Given the updated param name uses directory, should we stick with that verbiage?

So I used folder when downloading as GCS does not call them directories because technically they aren't. I used directory for uploading to represent a local disk directory. Do you think it is better to just stick with directory throughout? My concern was this may cause future confusion with upcoming features.

That distinction makes sense. The only thing that needs to be updated is the readmes: Upload Folder With Transfer Manager

frankyn

Overall LGTM, just had one nit

frankyn · 2022-12-21T18:08:09Z

samples/downloadFolderWithTransferManager.js

+  // const bucketName = 'your-unique-bucket-name';
+
+  // The ID of the GCS folder to download
+  // const folderName = 'your-folder-name';


does this support both relative and absolute?
If it does can you add it to the comment

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

shaffeeullah

thanks, denis!!

feat: add ability to upload entire directory, add samples for upload …

bfde215

…directory / download folder

ddelgrosso1 requested review from a team as code owners December 13, 2022 19:40

product-auto-label bot added the size: m Pull request size is medium. label Dec 13, 2022

product-auto-label bot added api: storage Issues related to the googleapis/nodejs-storage API. samples Issues that are directly related to samples. labels Dec 13, 2022

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 13, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 13, 2022

linter fix

e22f5f5

shaffeeullah reviewed Dec 14, 2022

View reviewed changes

danielbankhead approved these changes Dec 14, 2022

View reviewed changes

danielduhh reviewed Dec 14, 2022

View reviewed changes

update phrasing of comments

201a126

ddelgrosso1 force-pushed the tm-dir-samples branch from dec9dc0 to 201a126 Compare December 21, 2022 17:58

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 21, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 21, 2022

frankyn reviewed Dec 21, 2022

View reviewed changes

update comment in downloadFolderWithTransferManager sample

063aaff

ddelgrosso1 force-pushed the tm-dir-samples branch from 7c7c4fc to 063aaff Compare December 21, 2022 18:45

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 21, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 21, 2022

🦉 Updates from OwlBot post-processor

48c4919

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

shaffeeullah self-requested a review December 27, 2022 18:24

shaffeeullah approved these changes Dec 27, 2022

View reviewed changes

frankyn approved these changes Jan 3, 2023

View reviewed changes

ddelgrosso1 merged commit b0f32ce into googleapis:main Jan 4, 2023

ddelgrosso1 deleted the tm-dir-samples branch January 4, 2023 14:59

release-please bot mentioned this pull request Jan 4, 2023

chore(main): release 6.9.0 #2126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to upload entire directory, add samples for upload … #2118

feat: add ability to upload entire directory, add samples for upload … #2118

ddelgrosso1 commented Dec 13, 2022 •

edited

Loading

snippet-bot bot commented Dec 13, 2022 •

edited

Loading

shaffeeullah Dec 14, 2022

ddelgrosso1 Dec 14, 2022

shaffeeullah Dec 14, 2022

ddelgrosso1 Dec 14, 2022 •

edited

Loading

shaffeeullah Dec 14, 2022

shaffeeullah Dec 14, 2022

ddelgrosso1 Dec 14, 2022

danielbankhead left a comment

danielbankhead Dec 14, 2022

ddelgrosso1 Dec 21, 2022

danielduhh Dec 14, 2022

ddelgrosso1 Dec 15, 2022

danielduhh Dec 19, 2022

frankyn left a comment

frankyn Dec 21, 2022

shaffeeullah left a comment

feat: add ability to upload entire directory, add samples for upload … #2118

feat: add ability to upload entire directory, add samples for upload … #2118

Conversation

ddelgrosso1 commented Dec 13, 2022 • edited Loading

snippet-bot bot commented Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddelgrosso1 Dec 14, 2022 • 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

danielbankhead left a 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

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaffeeullah left a comment

Choose a reason for hiding this comment

ddelgrosso1 commented Dec 13, 2022 •

edited

Loading

snippet-bot bot commented Dec 13, 2022 •

edited

Loading

ddelgrosso1 Dec 14, 2022 •

edited

Loading