Skip to content

gdrive: add gdrive_user_credentials_file description#1192

Merged
shcheklein merged 5 commits into
masterfrom
gdrive-multi-remote
Apr 28, 2020
Merged

gdrive: add gdrive_user_credentials_file description#1192
shcheklein merged 5 commits into
masterfrom
gdrive-multi-remote

Conversation

@shcheklein
Copy link
Copy Markdown
Contributor

Corresponding core DVC PR treeverse/dvc#3686


You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please chose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 27, 2020 01:15 Inactive
@shcheklein shcheklein requested a review from jorgeorpinel April 27, 2020 01:15
@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 27, 2020 01:17 Inactive
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Suggestions and questions

Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/user-guide/setup-google-drive-remote.md
Comment on lines +202 to +203
`.dvc/tmp/gdrive-user-credentials.json` and they will be used automatically next
time you run DVC.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
`.dvc/tmp/gdrive-user-credentials.json` and they will be used automatically next
time you run DVC.
`.dvc/tmp/gdrive-user-credentials.json` by default, and they will be loaded
automatically next time you use the same GDrive remote.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is not exactly true, they can be used for multiple remotes if you don't specify an option gdrive_user_credentials_file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it. But "run DVC" doesn't sound exact either. How about

Suggested change
`.dvc/tmp/gdrive-user-credentials.json` and they will be used automatically next
time you run DVC.
`.dvc/tmp/gdrive-user-credentials.json` by default, and they will be loaded
automatically next time you use this or other GDrive remotes.

Maybe even continue this paragraph with the explanation about how to overwrite it, instead of a separate paragraph that repeats the file name.

Comment thread content/docs/user-guide/setup-google-drive-remote.md Outdated
Comment thread content/docs/user-guide/setup-google-drive-remote.md
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@shcheklein shcheklein requested a review from jorgeorpinel April 28, 2020 02:58
@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 28, 2020 02:58 Inactive
Comment on lines +209 to +210
If you use multiple GDrive remotes, by default they will be sharing the same
`.dvc/tmp/gdrive-user-credentials.json` credentials file. It can be overridden
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we don't need to repeat the file name since it's mentioned in in the previous paragraph (before the warning).

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

I just thing the part around the credentials file name and how to overwrite it could be merged into a single paragraph or something like that to avoid repetition and going back and forth on the same idea.

@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 28, 2020 22:10 Inactive
@shcheklein shcheklein merged commit cba33a3 into master Apr 28, 2020
@jorgeorpinel jorgeorpinel deleted the gdrive-multi-remote branch May 5, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants